On Mon, 24 Nov 2008, Mike McGrath wrote: Ugh, inciteful? really? You'd think I'd spell check these things by now :) -Mike

On Mon November 24 2008, Toshio Kuratomi wrote: From the proposal: | make a GET request that can change state on the server It is recommended to not use GET requests to change state on the server, therefore it would be probably better to change these GET requests to POST requests. Regards, Till

On Tue November 25 2008, Mike McGrath wrote: There may be also some other issues, e.g. when GET requests are used to submit confidential data, because then they may also be stored in the browsers history. But my concern was not about security issues. It would not be necessarily be serious damage, but the browser's session management could show annoying beheaviour, because then some requests could be made everytime a user restores are browser session. Regards, Till

On Tue November 25 2008, Toshio Kuratomi wrote: I guess it would be enough only to check whether the request is a POST-request without checking where the variables come from. This is maybe available in this variable: cherrypy.request.method This is also needed, if you check for the request method, because otherwise you would have broken links. Regards, Till

Till Maas wrote: The proposal doesn't specifically mention POST there as well but it should to make things clearer: "Every time we submit a form or make a GET request that can change state on the server" s/submit/POST/ /me changes that now. The reasons the proposal is explicit about GET are: 1) We'd have to constantly audit code for places where GET is being used to alter state and change that. This is doable if the app authors are aware of this but not so scalable if it's me going through and making those changes. 2) It is hard in most web frameworks to separate GET and POST requests. The framework takes the request and abstracts whether a GET or POST was used to send the variables and just send the variables themselves into the methods. The methods that receive the calls would have to jump through hoops to tell where the variables came from. So... if the proposal is secure for both POST and GET, I'd rather do that. If you can show where GET cannot be made secure (which is possible, I tried to examine the possibilities there and only came up with logging as a problem area but more eyes on this, the better) then we'll have to add a section for how to separate POST and GET requests in TurboGears and do periodic auditing to make sure methods which aren't marked POST-only aren't allowing changes to be made. -Toshio

Till Maas wrote: To be easy to code, require the token for every request of an authenticated user. -Toshio

Till Maas wrote: You are correct. This is mentioned in a different way in the "Other Notes" section: "We can make all links between Fedora Services carry the special session information. That will allow a link from the pkgdb to bodhi to stay logged in, for example, but it will not allow a user to open up a second tab and be logged in there. So this destroys some of the single-sign-on ability that we have now." I've now updated this to be clearer that this affects bookmarks and links in messages sent by the system. If the proposal was to manually mark which methods make changes and only those, then we'd be able to do this instead: User can bookmark links to pages which present choices to users (like a form). The form will be submitted with the double submit method using the values of the current cookie. The con of this is that the consequences of not marking a method are a security hole this way. One that we would have to audit the code to discover. I'm not a big fan of this tradeoff but I'm open to comments -- is this something that's so big a regression that we should bite the bullet and do it? Can we come up with another method that protects the users from failure by the programs author to properly mark the methods? So this is interesting. This is true since we wanted to remove the need to hash the tg-visit in javascript and thus the token is 100% statically derived. We could work around this in several ways: 1) In addition to checking the hash against the tg-visit, check against any non-expired session. If we did this check in the FAS server (all authentication goes back to the FAS server already) then I think that would work. So we'd send the FAS server the tg-visit cookie and the token. Then the FAS server would do the comparison of the token to the visit cookie; comparison to the visit to the current active sessions, and finally, if the token did not match the cookie, between the token and other non-expired sessions owned by the user. This seems like a relatively simple change to the proposal (change the server in one place to do a transparent check for the rest of the code). The only question would be how easy it is to send the token to the FAS server via the TurboGears identity methods... from a brief refresher look at jsonfas I think this is doable but it would be the hardest part. 2) Change to have javascript hash the cookie dynamically. This brings in two dependencies: javascript and a hashing library. The cookie that's sent to the web site will be updated when the user logs in on the second tab. If we then switch back to the first tab and the javascript there reads the cookie value and hashes it, we will have a valid token when that's submitted. In case javascript is turned off we'd still need to include the cookie information statically, this just would be replaced in normal circumstances. I've added #1 to the proposal for now. -Toshio

On Wed, Nov 26, 2008 at 04:53:00PM +0100, Till Maas wrote: Has anyone taken a look at PubCookie? It sounds like we are trying to re-invent the wheel here, which is probably not a good idea when it comes to security-related infrastructure. http://www.pubcookie.org/

Till Maas wrote: Pretty much agreed on this analysis. My one note is that in my usage, at least, I already have to login most of the time when clicking on a link in bugzilla or email due to my session having expired already. I thought of doing this but it still allows things to be insecure if what a method does gets changed and the whitelisting isn't updated. With my proposal, the identity and the token are tied together. So you can't have an identity unless you A) have a username and password or B) have both a token and some form of alternate id (SSL Cert or cookie). Since it's natural to protect a resource by looking for a specific group, username, or just not being anonymous, you are protected from CSRF at the same time. OTOH, if a whitelist of methods isn't updated when a form goes from merely showing information to changing data, you lose the CSRF protection. Not changing the token can have security ramifications as it allows the browser to specify what the visit key will be once the user is authenticated. I can't think of any way for javascript to manipulate this ATM but there could be something that I'm not thinking of or browser security holes could introduce something in this area. It not only makes it easier on the application author, it is secure if the application author does something wrong. This is the main advantage of this proposal. This is the one I don't know about. It will change my usage a bit since I'll need to start clicking on links to the other apps in present app pages to open new tabs. But when I usually open links from external sources I'm already used to having to re-login due to the session expiring. So we need feedback here, do you often click on multiple things in close proximity and wouldn't be able to change to clicking within the app? Are you able to This one's untrue as seen in the revised proposal. This one is probably not correct either. If the token and the tg-visit match, there's no extra request. If the token and tg-visit do not match, there's one extra request from FAS to the database. There are no extra json calls. This is true. But weighed against it is that usability takes a backseat to security. So is the usability loss high enough that we just have to be more vigilant in making sure that our list of secure/non-secure methods is kept up-to-date or is the security risk of people trying to hack us low enough that we can deal with the occasional bug that opens up a security vulnerability (which we won't find without auditing the code)? Or best of all, can we add some other check that allows us to preserve the present usability and still refuse state-changing events if they haven't been marked as such. -Toshio

Chuck Anderson wrote: We're talking about comments added to bugzilla that link to the Fedora Web Applications (pkgdb, bodhi, etc). Bugzilla has its own cookies and authentication structure that we won't be messing with as part of this. -Toshio

Till Maas wrote: Yes, I'm just wondering whether this is the normal usage because it isn't mine. As noted, I have to relogin nearly everytime I click on an external link because my session expires. (note: links inside of Fedora Apps and between Fedora Apps would carry the token so they wouldn't be subject to a relogin). This is different. It's very natural to think about user permissions when making a change. Just like Unix filesystem permissions, we're asking, does this user have permission to do this? Protecting against CSRF is different. It's asking whether the user really made the request or if it was only the user's browser making the request. That's not a natural thing to check for. I don't know the answer to how often but the actions do change sometimes. For instance, a URL can be used to display a comment form on an update. When you submit a comment you are brought back to the comment form with the new comment added. So there's two ways to write this: Either the action could submit to a new URL and the URL redirects back to the comment form URL once the request is processed or the action could submit to the same URL with the comment form's inputs as optional arguments. In Bodhi the first method is presently chosen. If it is recoded to the second method, marking for the purposes of CSRF protection would need to be updated. This is addressed above. This is something I thought for a bit on. However I'm not 100% satisfied with what can be done here. In the present pkgdb, session.flush() must be called anytime data is changed in the database. So we could override that method to add this protection. In FAS, adding a new certificate is not recorded in the database so overriding flush() is not enough to protect that. In bodhi we're still using SQLObject so we'd have to override the equivalent of flush() there as well. Additionally, there's autoflush methods that can be turned on in the database adapters. If a TG app uses autoflush, then we won't have a hook to override in this manner. I'm thinking of vulnerabilities when we add SSL Certs here instead of username/password. Sending session + username/password tells us that the user is in charge of the request. Sending session + token tells us that the browser was able to read information from the response so the same-origin-policy should protect us. Sending session + SSL Cert does not tell us anything as the browser uses both cookies and SSL Certs when making a call to a different domain at the behest of a web page. Keeping the same session id in this case could be dangerous if there are flaws in the browser or the server code that allow the malicious web page to set the session cookie. But this isn't my job. It's a hypothetical. <nod> So would it be better to fix the web app's UI? True. But the current situation is less secure than the proposal. So we need to understand the relative value of each rather than aiming for something that is as usable as currently. This might be true. Although better UI could resolve that. The server already stores these so there's not more information here. Rather than impossible, I think that catching things at the session.flush() call is a step towards this. But there's a sacrifice in flexibility in doing things here that I'm not certain we can enforce. -Toshio