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)
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)
15.96 KB,
patch
|
dkl
:
review+
|
Details | Diff | Splinter Review |
15.33 KB,
patch
|
LpSolit
:
review+
|
Details | Diff | 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)
Reporter | ||
Comment 1•12 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•12 years ago
|
||
Except I just found a bug. :/
Reporter | ||
Comment 3•12 years ago
|
||
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•12 years ago
|
||
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•12 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•12 years ago
|
||
Oh, forgot to mention that all tests pass (from runtests.pl output).
Assignee | ||
Updated•12 years ago
|
Attachment #585091 -
Flags: review?(LpSolit)
Assignee | ||
Comment 7•12 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•12 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.
Assignee | ||
Comment 9•12 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•12 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
Comment 14•10 years ago
|
||
so sir am o eligible for bounty ? I mailed security@mozilla.org also please be clear
Flags: needinfo?
Updated•10 years ago
|
Flags: needinfo?
Assignee | ||
Comment 15•10 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•10 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•10 years ago
|
||
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•10 years ago
|
||
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)
Comment 19•10 years ago
|
||
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
Comment 20•10 years ago
|
||
(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•10 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.
Comment 22•10 years ago
|
||
(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 23•10 years ago
|
||
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•10 years ago
|
Flags: approval?
Flags: approval4.4?
Comment 24•10 years ago
|
||
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?
Comment 26•10 years ago
|
||
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•10 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•10 years ago
|
Flags: approval4.4?
Comment 28•10 years ago
|
||
Fixed Bugzilla version id POD. Carrying forward r+.
Attachment #8382497 -
Attachment is obsolete: true
Attachment #8383238 -
Flags: review+
Comment 32•10 years ago
|
||
dveditz, we would like to release this soon. Can we get a CVE for this issue? Thanks dkl
Flags: needinfo?(dveditz)
Assignee | ||
Updated•10 years ago
|
Flags: blocking4.4.3+
Updated•10 years ago
|
Alias: CVE-2014-1517
Flags: needinfo?(dveditz)
Comment 33•10 years ago
|
||
Use CVE-2014-1517
Updated•10 years ago
|
Flags: approval?
Flags: approval4.4?
Flags: approval4.4+
Flags: approval+
Comment 34•10 years ago
|
||
(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•10 years ago
|
||
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•10 years ago
|
Attachment #8408330 -
Attachment description: patch, v4.2 → patch for trunk, v4.2
Assignee | ||
Updated•10 years ago
|
Attachment #8383238 -
Attachment description: 713926_44_3.patch → patch for 4.4
Assignee | ||
Comment 36•10 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
Closed: 10 years ago
Resolution: --- → FIXED
Comment 38•10 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•10 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?
Comment 40•10 years ago
|
||
(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•10 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.
You need to log in
before you can comment on or make changes to this bug.
Description
•