Last Comment Bug 670868 - (CVE-2011-2978) [SECURITY] Account preferences page trusts user-modifiable field for obtaining current e-mail address
(CVE-2011-2978)
: [SECURITY] Account preferences page trusts user-modifiable field for obtainin...
Status: RESOLVED FIXED
:
Product: Bugzilla
Classification: Server Software
Component: User Accounts (show other bugs)
: 2.16
: All All
: -- major (vote)
: Bugzilla 3.4
Assigned To: Byron Jones ‹:glob› [PTO until 2017-01-09]
: default-qa
:
Mentors:
Depends on:
Blocks: 660528
  Show dependency treegraph
 
Reported: 2011-07-12 00:29 PDT by Reed Loden [:reed] (use needinfo?)
Modified: 2011-08-05 17:33 PDT (History)
6 users (show)
LpSolit: approval+
mkanat: blocking4.2+
LpSolit: approval4.0+
mkanat: blocking4.0.2+
LpSolit: approval3.6+
mkanat: blocking3.6.6+
LpSolit: approval3.4+
LpSolit: blocking3.4.12+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch for 3.6 and higher, v1 (440 bytes, patch)
2011-07-12 01:38 PDT, Byron Jones ‹:glob› [PTO until 2017-01-09]
LpSolit: review+
Details | Diff | Splinter Review
patch for 3.4, v1 (468 bytes, patch)
2011-07-22 01:02 PDT, Byron Jones ‹:glob› [PTO until 2017-01-09]
LpSolit: review+
Details | Diff | Splinter Review

Description Reed Loden [:reed] (use needinfo?) 2011-07-12 00:29:25 PDT
I noticed this while investigating bug 670669. The 'account' tab under user prefs contains a hidden 'old_email' field. This field can be easily modified by a malicious user to contain some other address besides what is actually used. This prevents the real user from receiving an e-mail address confirmation message, so he/she may not notice that somebody has stolen his/her account.

In userprefs.cgi : SaveAccount(),
88     my $old_login_name = $cgi->param('old_login');
  ^^ pulled from user content

136             Bugzilla::Token::IssueEmailChangeToken($user, $old_login_name,
137                                                    $new_login_name);

  ^^ trusted as actual previous e-mail address

Instead of trusting user content, should just pull the e-mail address from the user object.
Comment 1 Reed Loden [:reed] (use needinfo?) 2011-07-12 00:33:20 PDT
Note that because of bug 670669, I don't even need to know the actual user password in order to do this. All it takes is to get a hold of a logincookie somehow.
Comment 2 Byron Jones ‹:glob› [PTO until 2017-01-09] 2011-07-12 01:38:57 PDT
Created attachment 545336 [details] [diff] [review]
patch for 3.6 and higher, v1

the fix is pretty simple; with it you can no longer redirect the confirmation email.
Comment 3 Reed Loden [:reed] (use needinfo?) 2011-07-12 08:48:33 PDT
Also should remove 'old_login' from the account prefs form.
Comment 4 Byron Jones ‹:glob› [PTO until 2017-01-09] 2011-07-12 09:53:36 PDT
(In reply to comment #3)
> Also should remove 'old_login' from the account prefs form.

i wanted to do that, but it's being used as part of authentication.  i didn't think the refactoring was worth the risk.
Comment 5 Reed Loden [:reed] (use needinfo?) 2011-07-12 10:10:54 PDT
What refactoring? Just remove the hidden 'old_email' field from the page.
Comment 6 Reed Loden [:reed] (use needinfo?) 2011-07-12 10:12:04 PDT
er, 'old_login'.
Comment 7 Reed Loden [:reed] (use needinfo?) 2011-07-12 10:14:46 PDT
Ah, I see what you mean... Sorry, I was accidentally grepping for 'old_email' and didn't see a problem. ;)

if (!Bugzilla->user->id) {
    # Use credentials given in the form if login cookies are not available.
    $cgi->param('Bugzilla_login', $cgi->param('old_login'));
    $cgi->param('Bugzilla_password', $cgi->param('old_password'));
}

I'm not sure why this is here... 'old_login' is only available on the account prefs page, and you should be logged-in already to get to that page. LpSolit, thoughts?
Comment 8 Max Kanat-Alexander 2011-07-12 23:30:34 PDT
Wow, WTF.
Comment 9 Max Kanat-Alexander 2011-07-19 15:38:14 PDT
Comment on attachment 545336 [details] [diff] [review]
patch for 3.6 and higher, v1

Why does old_login exist at all? Can we get rid of it in the HTML as well?
Comment 10 Byron Jones ‹:glob› [PTO until 2017-01-09] 2011-07-19 20:05:51 PDT
(In reply to comment #9)
> Why does old_login exist at all? Can we get rid of it in the HTML as well?

--> comment 4 :)
Comment 11 Frédéric Buclin 2011-07-20 02:53:57 PDT
(In reply to comment #9)
> Why does old_login exist at all? Can we get rid of it in the HTML as well?

I will jump in, as I didn't say anything yet. :)

The reason we store the old_login in the HTML page is so that when the user doesn't use cookies in his browser, he doesn't need to enter his (old) password twice when saving data on this page, see bug 45918 (implemented in Bugzilla 2.14). Honestly, such a user doesn't go to this page everyday, and moreover, he already has to enter his credentials every time he accesses a page which requires them. So I would prefer to drop old_login entirely to avoid any abuse and leave the few users who do not want to enable cookies enter their password twice on this page (once in the "Old Password" field, and once again when credentials are asked).

justdave, this means reverting what you did in bug 45918. Are you fine with this?
Comment 12 Dave Miller [:justdave] (justdave@bugzilla.org) 2011-07-20 09:35:03 PDT
Wow, this would be why we were supposed to be validating the old_password before allowing any changes to anything listed on this page.  Which makes this pretty related to bug 670669.

So the two possible fixes here are:
1) once again enforce that old_password check
or
2) remove the old password field entirely and require cookies.

#2 might be palatable these days.  Most people realize that trying to use Bugzilla without cookies is painful.  Most browsers allow you to accept/block cookies per-site now, and people should know to allow Bugzilla.  #1 might also be a valid fix, but probably depends on bug 670669.
Comment 13 Frédéric Buclin 2011-07-21 10:16:56 PDT
Comment on attachment 545336 [details] [diff] [review]
patch for 3.6 and higher, v1

This is indeed the right fix. No need to remove old_login. If login cookies are present, Bugzilla will use them and ignore old_login. If the login cookies are missing, then Bugzilla will use old_login and old_password to authenticate the user. From here, there is no way to abuse Bugzilla with glob's fix.

In a follow-up bug, we should fix Bugzilla::Token::IssueEmailChangeToken() to only get ($user, $new_email) as arguments, and get $old_email from $user->login, instead of passing $old_email as we currently do. This would also prevent this abuse. But this should only be done on trunk (even 4.2), as a security enhancement.

r=LpSolit
Comment 14 Frédéric Buclin 2011-07-21 10:32:23 PDT
A backport is needed for 3.4.11 too. If you clear Bugzilla_login (and let Bugzilla get your credentials from the login cookies), the old recipient gets no email. The same fix will work.
Comment 15 Byron Jones ‹:glob› [PTO until 2017-01-09] 2011-07-22 01:02:49 PDT
Created attachment 547624 [details] [diff] [review]
patch for 3.4, v1
Comment 16 Frédéric Buclin 2011-07-22 03:00:47 PDT
Comment on attachment 547624 [details] [diff] [review]
patch for 3.4, v1

r=LpSolit
Comment 17 Daniel Veditz [:dveditz] 2011-08-01 16:35:00 PDT
Use CVE-2011-2978 for this bug
Comment 18 Frédéric Buclin 2011-08-02 05:51:35 PDT
From what I can see, this problem exists since users can change their email address, which happened in 2.16rc1, see bug 23067.
Comment 19 Frédéric Buclin 2011-08-04 13:50:49 PDT
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/
modified userprefs.cgi
Committed revision 7891.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.0/
modified userprefs.cgi
Committed revision 7637.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/3.6/
modified userprefs.cgi
Committed revision 7254.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/3.4/
modified userprefs.cgi
Committed revision 6808.
Comment 20 Max Kanat-Alexander 2011-08-05 17:33:30 PDT
Security advisory sent, unlocking this bug.

Note You need to log in before you can comment on or make changes to this bug.