Closed Bug 713926 (CVE-2014-1517) Opened 12 years ago Closed 10 years ago

[SECURITY] Login form lacks CSRF protection

Categories

(Bugzilla :: User Accounts, defect)

4.0.2
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 4.4

People

(Reporter: reed, Assigned: LpSolit)

References

()

Details

(Keywords: sec-low, Whiteboard: [wanted-bmo][infrasec:csrf][ws:low])

Attachments

(2 files, 7 obsolete files)

Attached patch patch - v1 (obsolete) — Splinter Review
[spinning off from bug 471801]

The (CGI-based) login form lacks any type of CSRF protection. This is needed to alleviate the concern specified in section 3 of Adam Barth's CSRF paper (http://seclab.stanford.edu/websec/csrf/csrf.pdf) where an attacker could cause a user to authenticate to Bugzilla using the *attacker's* credentials.

While not optimal, this restricts login form uses to the user's IP plus the time period of MAX_TOKEN_AGE.
Attachment #584620 - Flags: review?(mkanat)
Comment on attachment 584620 [details] [diff] [review]
patch - v1

I've tested this, and it works as expected both with a valid token and without a valid token.
Attachment #584620 - Flags: review?(dkl)
Except I just found a bug. :/
Attached patch patch - v2 (obsolete) — Splinter Review
Previous patch had two bugs
 1) Login page wasn't excluding the 'GoAheadAndLogIn' and 'token' params
 2) If user has valid login but invalid token, clicking "confirm" on the confirm action page will still not log the user in, as Bugzilla_login and Bugzilla_password are not given as hidden fields

I fixed 1), and I tried to fix 2), but I got to thinking that we shouldn't be saving those fields as hidden fields anyway, so the user is just going to have to log in again.

One concern, though: The confirm action page is more for actual changes, rather than logging in. As such, the text is slightly confusing. I could fix this, but I would basically have to duplicate most of confirm-action.html.tmpl into another block. If that's wanted, I'm happy to do that.

The comment move is because I was digging into hidden-fields, and the comment was in the wrong place, which was confusing me.
Attachment #584620 - Attachment is obsolete: true
Attachment #584620 - Flags: review?(mkanat)
Attachment #584620 - Flags: review?(dkl)
Attachment #585083 - Flags: review?(mkanat)
Attachment #585083 - Flags: review?(dkl)
Attached patch patch - v3 (obsolete) — Splinter Review
Actually, we need separate login tokens from normal tokens, as we don't want to wipe out a valid token for some specific action with our trivial login token. This version separates the two ("login_token" vs. "token").

I also separated the text in confirm-action, but I find what I did very messy. I tried a few things, and I didn't like any of them. Thoughts?

How do I get rid of the extra whitespace here?

  <input type="submit" id="confirm" value="      Send me to the login page">
</form>

<p><a href="index.cgi">    Redirect me to the home page.</a></p>
Attachment #585083 - Attachment is obsolete: true
Attachment #585083 - Flags: review?(mkanat)
Attachment #585083 - Flags: review?(dkl)
Attachment #585091 - Flags: review?(mkanat)
Attachment #585091 - Flags: review?(dkl)
(In reply to Reed Loden [:reed] (very busy) from comment #4)
> How do I get rid of the extra whitespace here?
> 
>   <input type="submit" id="confirm" value="      Send me to the login page">
> </form>
> 
> <p><a href="index.cgi">    Redirect me to the home page.</a></p>

Replaced %- with %~... That seemed to help.
Oh, forgot to mention that all tests pass (from runtests.pl output).
Attachment #585091 - Flags: review?(LpSolit)
Comment on attachment 585091 [details] [diff] [review]
patch - v3

>=== modified file 'template/en/default/account/auth/login-small.html.tmpl'

>+    <input type="hidden" id="login_token[% qs_suffix %]" name="login_token" value="[% issue_hash_token(['login']) FILTER html %]">

This line is too long. Also, you are going to generate two login tokens per page. Are you sure your code handles this correctly?


>=== modified file 'template/en/default/account/auth/login.html.tmpl'

>   [% PROCESS "global/hidden-fields.html.tmpl"
>-     exclude="^Bugzilla_(login|password|restrictlogin)$" %]
>+     exclude="^(Bugzilla_(login|password|restrictlogin)|GoAheadAndLogIn|login_token)$" %]

You exclude GoAheadAndLogIn with no explicit reason. Could you explain why you do this and did you make sure this has no side-effect (even in less obvious cases)? I know it's redefined later in this template, but it's just to make sure we are not regressing anything.


>=== modified file 'template/en/default/global/confirm-action.html.tmpl'

>+[% UNLESS cgi.param("GoAheadAndLogIn") %]

You cannot use cgi.param() here. cgi is undefined unless you define it explicitly. Also, I'm not convinced that looking at GoAheadAndLogIn is the right way to go.

Also, I see that you duplicate a lot of text and add a lot of checks about GoAheadAndLogIn. If templates are so different, then a separate template would be better than messing this one.
Attachment #585091 - Flags: review?(mkanat)
Attachment #585091 - Flags: review?(dkl)
Attachment #585091 - Flags: review?(LpSolit)
Attachment #585091 - Flags: review-
(In reply to Frédéric Buclin from comment #7)
> >+    <input type="hidden" id="login_token[% qs_suffix %]" name="login_token" value="[% issue_hash_token(['login']) FILTER html %]">
> 
> This line is too long. Also, you are going to generate two login tokens per
> page. Are you sure your code handles this correctly?

Yes. They are just hash tokens, and based on how they are created, they are generally the same. I will split the line in my next update.

> >=== modified file 'template/en/default/account/auth/login.html.tmpl'
> 
> >   [% PROCESS "global/hidden-fields.html.tmpl"
> >-     exclude="^Bugzilla_(login|password|restrictlogin)$" %]
> >+     exclude="^(Bugzilla_(login|password|restrictlogin)|GoAheadAndLogIn|login_token)$" %]
> 
> You exclude GoAheadAndLogIn with no explicit reason. Could you explain why
> you do this and did you make sure this has no side-effect (even in less
> obvious cases)? I know it's redefined later in this template, but it's just
> to make sure we are not regressing anything.

As I mentioned in comment #3, if I don't exclude GoAheadAndLogIn and login_token, they get copied back in, which will interfere with the the new fields that get added a few lines below that. I've tested a variety of cases, and I haven't seen any problems, but I'm happy to try some other stuff.

> >+[% UNLESS cgi.param("GoAheadAndLogIn") %]
> 
> You cannot use cgi.param() here. cgi is undefined unless you define it
> explicitly. Also, I'm not convinced that looking at GoAheadAndLogIn is the
> right way to go.

Hmm, weird. Seemed to work in my testing (the right text was being displayed), but maybe the undef part was what was causing it to work. If you don't think GoAheadAndLogIn is the right way, what alternatives do you propose? Some variable somehow? I could swap to testing for 'login_token' now that the two are split.

> Also, I see that you duplicate a lot of text and add a lot of checks about
> GoAheadAndLogIn. If templates are so different, then a separate template
> would be better than messing this one.

Yeah, thought about that. Would just mean I would have to modify check_hash_token() somehow (new param?) to make it use the different template. I will see what seems to work best.
Keywords: sec-low
I talked about this issue on IRC with reed yesterday, and I still want a PoC before going further. I don't see how this is exploitable. Being logged in with the attacker's credentials is not a security issue as you cannot do any harm; for instance, you can no longer access security bugs if the attacker couldn't access the bug himself.
Severity: normal → minor
(In reply to Frédéric Buclin from comment #9)
> I talked about this issue on IRC with reed yesterday, and I still want a PoC
> before going further. I don't see how this is exploitable. Being logged in
> with the attacker's credentials is not a security issue as you cannot do any
> harm; for instance, you can no longer access security bugs if the attacker
> couldn't access the bug himself.

Example I mentioned on IRC:
* Attacker causes the victim to login to Bugzilla as the attacker. Victim submits security bug. Attacker immediately has access to bug, as it was filed as the attacker instead of the victim.
Severity: minor → normal
so sir am o eligible for bounty ?
I mailed security@mozilla.org also please be clear
Flags: needinfo?
Flags: needinfo?
(In reply to pbssubhash from comment #14)
> so sir am o eligible for bounty ?

You already got your answer in bug 966183: sec-bounty- means "no", sorry.
Note that Bugzilla::Auth::Login::CGI is also called by the User.login webservice method, and so requiring a token would break WebServices as you would be unable to log in. I did some investigation, and there is no way to distinguish a valid User.login call and an evil one as XMLHttpRequest lets you change the Content-Type header to application/json.
Attached patch patch, v4 (obsolete) — Splinter Review
As mentioned in bug 726696 comment 13

The main login page generates a cookie which is stored in the web browser and a corresponding token is included in the login form. When the user submits his credentials, the token is compared with the cookie to make sure the submission is intentional. If cookies are disabled, it falls back to the Referer header to make sure it's a local submission and not one coming from some external website.

The mini login forms in the page header and footer do not include the token nor do they generate a token as we would have to do it all the time, for all pages being viewed while being logged out. This seems to be overkill. As long as the Referer header is available, this is not a problem, else an error will be thrown asking the user to log in again, this time from the main login page. I cannot automatically generate a token without its cookie counterpart, else the attacker could copy it in his remote attack.

For WebServices, it bypasses this check as Bugzilla::Auth::Persist::Cookie no longer creates cookies for them. It only creates a token which can then be returned by User.login. This also means that an attacker can no longer override your login cookies and so the victim won't do subsequent actions on his behalf.
Assignee: reed → LpSolit
Attachment #585091 - Attachment is obsolete: true
Attachment #8369134 - Flags: review?(dkl)
Attached patch patch, v4.1 (obsolete) — Splinter Review
The mini login form now also includes a token, which must match the cookie to validate the request. If cookies are disabled, we still fall back to the Referer header.

Bugzilla is so impractical with cookies disabled (requires login for each action) that I would be tempted to say that cookies should be mandatory, which would let me remove the Referer header check.
Attachment #8369134 - Attachment is obsolete: true
Attachment #8369134 - Flags: review?(dkl)
Attachment #8369192 - Flags: review?(dkl)
Not going to take this on 4.2 because it requires stuff that we can't backport that far as a prerequisite.  This will go in 4.4 and trunk only. The security advisory will need to point out that it isn't fixed in the older branches because of API breakage, and people who want paranoid security should upgrade.
Target Milestone: Bugzilla 4.2 → Bugzilla 4.4
Depends on: 893195
(In reply to Frédéric Buclin from comment #16)
> there is no way to
> distinguish a valid User.login call and an evil one as XMLHttpRequest lets
> you change the Content-Type header to application/json.

But you can't do cross-site XMLHTTPRequest without a CORS agreement, which Bugzilla will not give.

(Of course, bug 726696, but still.)

I think it may now be reasonable to require cookies for logging in to Bugzilla. Every web client supports cookies, and many have or can have per-site cookie control. The user experience without cookies is pretty miserable. Back in 2000 at Oxford University, they had a weird cookie-stripping firewall so I had to log in for every change. This was so annoying that I downloaded a keystroke scripting app so I could press a single key and it would type my username and password and submit the form...

Gerv
(In reply to Gervase Markham [:gerv] from comment #20)
> But you can't do cross-site XMLHTTPRequest without a CORS agreement, which
> Bugzilla will not give.

You don't need a CORS agreement to call WS methods. You just won't get the answer back, but the damage is already done. So no, it's not OK to accept cookies.
(In reply to Frédéric Buclin from comment #21)
> (In reply to Gervase Markham [:gerv] from comment #20)
> > But you can't do cross-site XMLHTTPRequest without a CORS agreement, which
> > Bugzilla will not give.
> 
> You don't need a CORS agreement to call WS methods. You just won't get the
> answer back, but the damage is already done. So no, it's not OK to accept
> cookies.

This bug is specifically about login. No damage is done if the attacker causes the user's browser to send a HEAD (or is it OPTIONS) request to the login URL, and Bugzilla fails to return the relevant CORS headers so the browser goes no further.

Damage can only be done if either a) the user actually gets logged in, or b) the attacker gets the user's credentials or cookie. And neither can happen in this scenario.

Gerv
Comment on attachment 8369192 [details] [diff] [review]
patch, v4.1

Review of attachment 8369192 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good and works as expected. r=dkl

::: Bugzilla/WebService.pm
@@ +168,4 @@
>  
>  For REST, you may also use the C<username> and C<password> variable
>  names instead of C<Bugzilla_login> and C<Bugzilla_password> as a
>  convenience.

While we are here, lets mention that 'token' can be used as well for convenience.
Attachment #8369192 - Flags: review?(dkl) → review+
Flags: approval?
Flags: approval4.4?
Sigh. LpSolit made a valid point that we need to backport bug 893195 before this can be accepted for 4.4. Removing flag for 4.4 for now.

dkl
Flags: approval4.4?
Attached patch 713926_44_1.patch (obsolete) — Splinter Review
Patch for Bugzilla 4.4
Attachment #8382303 - Flags: review?(LpSolit)
Attached patch 713926_44_2.patch (obsolete) — Splinter Review
POD updated. I reworded it a little differently so let me know if it looks ok.

dkl
Attachment #8382303 - Attachment is obsolete: true
Attachment #8382303 - Flags: review?(LpSolit)
Attachment #8382497 - Flags: review?(LpSolit)
Comment on attachment 8382497 [details] [diff] [review]
713926_44_2.patch

>=== modified file 'Bugzilla/WebService.pm'

>+=item C<Bugzilla_token>
>+
>+B<Added in Bugzilla 5.0 and backported to 4.x>

Shouldn't it be "Added in Bugzilla 4.4.3"? At least "4.x" is clearly wrong as 4.0.x, 4.2.x and 4.4, 4.4.1 and 4.4.2 don't have it.



>=== modified file 'Bugzilla/WebService/User.pm'

>+requests to the webservice, for the duration of the session, i.e. til
>+L<User.logout|/logout> is called.

s/til/till/


Otherwise works fine. r=LpSolit
Attachment #8382497 - Flags: review?(LpSolit) → review+
Flags: approval4.4?
Attached patch patch for 4.4Splinter Review
Fixed Bugzilla version id POD. Carrying forward r+.
Attachment #8382497 - Attachment is obsolete: true
Attachment #8383238 - Flags: review+
dveditz, we would like to release this soon. Can we get a CVE for this issue?

Thanks
dkl
Flags: needinfo?(dveditz)
Flags: blocking4.4.3+
Alias: CVE-2014-1517
Flags: needinfo?(dveditz)
Blocks: 996172
Flags: approval?
Flags: approval4.4?
Flags: approval4.4+
Flags: approval+
(In reply to David Lawrence [:dkl] from comment #23)
> ::: Bugzilla/WebService.pm
> @@ +168,4 @@
> >  
> >  For REST, you may also use the C<username> and C<password> variable
> >  names instead of C<Bugzilla_login> and C<Bugzilla_password> as a
> >  convenience.

This needs to be changed and may as well do it now if the patch is getting revised anyway.

For REST, you may also use the C<login> and C<password> variable
names instead of C<Bugzilla_login> and C<Bugzilla_password> as a
convenience. You may also use C<token> instead of C<Bugzilla_token>.

dkl
Fixed bitrot in admin/sudo.html.tmpl and also fixed the POD in WebService.pm per dkl's comment 34.
Attachment #8369192 - Attachment is obsolete: true
Attachment #8408330 - Flags: review+
Attachment #8408330 - Attachment description: patch, v4.2 → patch for trunk, v4.2
Attachment #8383238 - Attachment description: 713926_44_3.patch → patch for 4.4
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   b639a1a..0e39097  master -> master

To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   8835520..2f385c1  4.4 -> 4.4
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Security advisory sent.
Group: bugzilla-security
This bug is probably the reason why BZ 4.4.3 and later no longer returns cookies when xmlrpc User.login is called.  Can anyone clarify actual risks?  There's some discussion around comment #20, but it does not seem to have enough detail to identify attack vector being blocked.  Thank you!
API documentation got this text as part of this change:

  Support for using login cookies for authentication has been dropped
  for security reasons.

This does not seem accurate based on testing with a BZ instance that already got this fix.  Both cookies obtained before update, as well as new cookies generated by splitting token value are accepted as authentication for xmlrpc requests.

Is that cookie support expected to be removed in future versions?
Blocks: 1001497
(In reply to Frédéric Buclin from comment #36)
> To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
>    b639a1a..0e39097  master -> master
> 
> To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
>    8835520..2f385c1  4.4 -> 4.4

Hi,
what about 4.2? patch from this BZ doesn't apply to 4.2.9.
http://www.bugzilla.org/security/4.0.11/

             Due to changes involved in the Bugzilla API, this fix is
             not backported to the 4.0 and 4.2 branches, meaning that
             Bugzilla 4.0.12 and older, and 4.2.8 and older, will
             remain vulnerable to this issue.
Blocks: 1024987
Blocks: 1132887
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: