Bug 713926 (CVE-2014-1517)

[SECURITY] Login form lacks CSRF protection

RESOLVED FIXED in Bugzilla 4.4

Status

()

Bugzilla
User Accounts
RESOLVED FIXED
6 years ago
13 days ago

People

(Reporter: reed, Assigned: Frédéric Buclin)

Tracking

({sec-low})

4.0.2
Bugzilla 4.4
sec-low
Dependency tree / graph
Bug Flags:
approval +
approval4.4 +
blocking4.4.3 +

Details

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

Attachments

(2 attachments, 7 obsolete attachments)

(Reporter)

Description

6 years ago
Created attachment 584620 [details] [diff] [review]
patch - v1

[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)
(Reporter)

Comment 1

6 years ago
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)
(Reporter)

Comment 2

6 years ago
Except I just found a bug. :/
(Reporter)

Comment 3

6 years ago
Created attachment 585083 [details] [diff] [review]
patch - v2

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)
(Reporter)

Comment 4

6 years ago
Created attachment 585091 [details] [diff] [review]
patch - v3

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)
(Reporter)

Comment 5

6 years ago
(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.
(Reporter)

Comment 6

6 years ago
Oh, forgot to mention that all tests pass (from runtests.pl output).
(Assignee)

Updated

6 years ago
Attachment #585091 - Flags: review?(LpSolit)
(Assignee)

Comment 7

6 years ago
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-
(Reporter)

Comment 8

6 years ago
(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
(Assignee)

Comment 9

5 years ago
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
(Reporter)

Comment 10

5 years ago
(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
(Assignee)

Updated

4 years ago
Duplicate of this bug: 961425
Duplicate of this bug: 963687
(Assignee)

Updated

4 years ago
Duplicate of this bug: 966183

Comment 14

4 years ago
so sir am o eligible for bounty ?
I mailed security@mozilla.org also please be clear
Flags: needinfo?

Updated

4 years ago
Flags: needinfo?
(Assignee)

Comment 15

4 years ago
(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.
(Assignee)

Comment 16

4 years ago
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.
(Assignee)

Comment 17

4 years ago
Created attachment 8369134 [details] [diff] [review]
patch, v4

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)
(Assignee)

Comment 18

4 years ago
Created attachment 8369192 [details] [diff] [review]
patch, v4.1

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
(Assignee)

Comment 21

4 years ago
(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+

Updated

4 years ago
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?
Created attachment 8382303 [details] [diff] [review]
713926_44_1.patch

Patch for Bugzilla 4.4
Attachment #8382303 - Flags: review?(LpSolit)
Created attachment 8382497 [details] [diff] [review]
713926_44_2.patch

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)
(Assignee)

Comment 27

4 years ago
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+
(Assignee)

Updated

4 years ago
Flags: approval4.4?
Created attachment 8383238 [details] [diff] [review]
patch for 4.4

Fixed Bugzilla version id POD. Carrying forward r+.
Attachment #8382497 - Attachment is obsolete: true
Attachment #8383238 - Flags: review+
(Assignee)

Updated

4 years ago
Duplicate of this bug: 981186
Duplicate of this bug: 992094
(Assignee)

Updated

4 years ago
Duplicate of this bug: 994297
dveditz, we would like to release this soon. Can we get a CVE for this issue?

Thanks
dkl
Flags: needinfo?(dveditz)
(Assignee)

Updated

4 years ago
Flags: blocking4.4.3+
Alias: CVE-2014-1517
Flags: needinfo?(dveditz)
Use CVE-2014-1517

Updated

4 years ago
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
(Assignee)

Comment 35

4 years ago
Created attachment 8408330 [details] [diff] [review]
patch for trunk, v4.2

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+
(Assignee)

Updated

4 years ago
Attachment #8408330 - Attachment description: patch, v4.2 → patch for trunk, v4.2
(Assignee)

Updated

4 years ago
Attachment #8383238 - Attachment description: 713926_44_3.patch → patch for 4.4
(Assignee)

Comment 36

4 years ago
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
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Comment 37

4 years ago
Security advisory sent.
Group: bugzilla-security

Comment 38

4 years ago
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!

Comment 39

4 years ago
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?
(Assignee)

Updated

4 years ago
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.

Comment 41

4 years ago
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.
(Assignee)

Updated

3 years ago
Blocks: 1024987
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1087971
Duplicate of this bug: 1094085
(Assignee)

Updated

3 years ago
Blocks: 1132887
You need to log in before you can comment on or make changes to this bug.