706790 - BrowserID: implement CSRF protection

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: Extensions, defect)

Description

[Gervase Markham [:gerv]]

See URL. We should use the token system to implement CSRF protection for BrowserID logins.

Gerv

Comment 1

[Gervase Markham [:gerv]]

This is non-trivial :-| Bugzilla's token-issuing system says:

    # The concatenated string [that makes the token] is of the form
    # token creation time + site-wide secret + user ID + data

The time is used so that tokens can expire. The site-wide secret is a constant. If the user is not logged in, the user ID is 0. The data is any data passed in which stops tokens for one purpose in Bugzilla being used for another purpose - but if the user is not logged in, we haven't got any secret data of that sort either.

The upshot is that if we just ask Bugzilla for a token, then the token returned is usable by anyone, i.e. it's not tied to the user's current session (they don't have one). So we can't use it to prevent Login CSRF - because an attacker can mint a valid token at will just by loading index.html and reading it from the HTML/JS...

Adding a token will mean at least that an attacker's CSRF trap will work for at most MAX_TOKEN_AGE (3) days. But that doesn't help much.

I'm not sure this is fixable...

The issue here is that users could get logged out of their accounts and logged in as someone else, not notice and then input some sensitive data that the attacker could capture or see. I guess the question is: how long will a Mozilla person take to notice that they are logged in as someone else's Bugzilla account?

Gerv

Comment 2

[Reed Loden [:reed]]

(In reply to Gervase Markham [:gerv] from comment #1)
>     # The concatenated string [that makes the token] is of the form
>     # token creation time + site-wide secret + user ID + data
> 
> The time is used so that tokens can expire. The site-wide secret is a
> constant. If the user is not logged in, the user ID is 0.

This changed recently. See bug 705474 (and bug 471801 following). "user ID" is now *either* the user ID if the user exists OR the remote IP of the browser (from remote_ip()).

> Adding a token will mean at least that an attacker's CSRF trap will work for
> at most MAX_TOKEN_AGE (3) days. But that doesn't help much.

Yeah, that's one of my remaining concerns. 3 days is way too long for a token of this type. I'd like to have support for setting different times for certain tokens. If that existed, then I could have another constant that was much more reasonable (say, maybe 2-6 hours or so, or even sooner).

Comment 3

[Gervase Markham [:gerv]]

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/extensions/browserid/trunk/  
modified template/en/default/hook/account/auth/login-small-additional_methods.html.tmpl
modified lib/Login.pm
modified template/en/default/hook/account/auth/login-additional_methods.html.tmpl
Committed revision 7.

OK, I checked in token support. The strength of the protection depends on the strength of the uniqueness of the tokens - so taking reed's patch to use remote_ip() would probably be a good idea.

Gerv