Fixing CSRF exploits in Infrastructure

[Toshio Kuratomi]

Till Maas wrote:
> On Wed November 26 2008, Toshio Kuratomi wrote:
>> Till Maas wrote:
> 
>>> How big the regression is if users have to log in for every external link
>>> they click on, depends on how often this happens. I believe that links to
>>> FAS are not exchanged very often, therefore it will not hurt very much. I
>>> guess there is also not so often a need to use FAS with tabs. But maybe
>>> there are people who have to use FAS more often. With Bodhi it is
>>> contrary, because there it is normal to get mails with links if someone
>>> added a comment to a package or for testers to exchange links to Bodhi
>>> updates. Also links to Bodhi updates are used in Bugzilla comments. There
>>> it would have a much bigger impact on the efficiency of testing new
>>> package updates imho.
>> 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.
> 
> But in the future you would have to do this for every link everytime, even if 
> you use many of them in a short timeframe.
> 
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).

>>> Regarding the time needed for auditing applications: There may still be a
>>> lot of other vulnerabilites in these applications which cannot be fixed
>>> automatically. Therefore they still need to be written carefully. But
>>> maybe a compromise would be to require the token for all requests by
>>> default and then whitelist the ones, that are not meant to change state,
>>> e.g. requests like:
>>>
>>> https://admin.fedoraproject.org/updates/pstreams-devel-0.6.0-6.fc10
>> 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.
> 
> What it am apllication writer does not require authentication for some action 
> that should require authentication? With this reasoning you could also 
> require that every request has to be authenticated.

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.

> Also do the actions behind a certain request / URL really change that often?
> 
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.

>> 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.
> 
> If you also do not add proper authentication if a method gets updated to show 
> confidential information, you lose the protection of the confidential 
> information.
> 
This is addressed above.

> Is it maybe possible to modify the functions behind the API that are used to 
> change state to require a valid token? Then the author of the webapplication 
> needs to activly work agains the CSRF protection instead of accidently 
> forgetting it.
> 
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.

>>> Another way would be to not change the session id if a user needs to
>>> supply the username/password again only because the token was missing. It
>>> would probably be enough to ask the user only to click a link that
>>> contains the matching token in case the token is missing.
>> 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.
> 
> How is it different to keep the same session id after sending a valid session 
> id and token to the server than to keep it after sending a valid session and 
> a username / password combination?
> 
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.

>>> On the
>>> downside it has a high impact on usability
>> 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
> 
> If I send you a bunch of links that point to Bodhi updates and your job is to 
> add one comment to each update, then you have to provide your username and 
> password once for each link. Currently you would only login once for the 
> first link and open the others afterwards.

But this isn't my job.  It's a hypothetical.

> I use as often as possible direct 
> links from mails or bookmarks, because clicking through the webapp to get to 
> a certain location takes a lot more time.
> 
<nod>  So would it be better to fix the web app's UI?

>>> or makes the automatic CSRF
>>> protection a lot more complicated.
>> This one's untrue as seen in the revised proposal.
> 
> The revised proposal is not yet as usable as the current situation.
> 
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.

>>> Also securing all requests may cost a lot
>>> of performance, because more requests need to be made.
>> 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.
> 
> If one has to click through the webapp instead of clicking directly on a link, 
> there are also a lot more requests.

This might be true.  Although better UI could resolve that.

> Also if one can have multiple valid 
> session ids, then more need to be stored on the server.
> 
The server already stores these so there's not more information here.

>> 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.
> 
> Imho this is not possible, because to refuse state-changing events, you need 
> to be able to distinguish them from other events. If you have an algorithm 
> for this, you can use it to mark thes events.
> 
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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 197 bytes
Desc: OpenPGP digital signature
Url : http://lists.fedoraproject.org/pipermail/infrastructure/attachments/20081202/41fb5182/attachment.bin