670868 - (CVE-2011-2978) [SECURITY] Account preferences page trusts user-modifiable field for obtaining current e-mail address

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, defect)

Description

[Reed Loden [:reed]]

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]]

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

[:glob ✱]

Attachment: 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]]

Also should remove 'old_login' from the account prefs form.

Comment 4

[:glob ✱]

(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]]

What refactoring? Just remove the hidden 'old_email' field from the page.

Comment 6

[Reed Loden [:reed]]

er, 'old_login'.

Comment 7

[Reed Loden [:reed]]

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]

Wow, WTF.

Comment 9

[Max Kanat-Alexander]

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

[:glob ✱]

(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]

(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]]

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]

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]

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

[:glob ✱]

Attachment: patch for 3.4, v1

Comment 16

[Frédéric Buclin]

Comment on attachment 547624 [details] [diff] [review]
patch for 3.4, v1

r=LpSolit

Comment 17

[Daniel Veditz [:dveditz]]

Use CVE-2011-2978 for this bug

Comment 18

[Frédéric Buclin]

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]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified userprefs.cgi
Committed revision 7891.

Committing to: bzr+ssh://[e-mail]/bugzilla/4.0/
modified userprefs.cgi
Committed revision 7637.

Committing to: bzr+ssh://[e-mail]/bugzilla/3.6/
modified userprefs.cgi
Committed revision 7254.

Committing to: bzr+ssh://[e-mail]/bugzilla/3.4/
modified userprefs.cgi
Committed revision 6808.

Comment 20

[Max Kanat-Alexander]

Security advisory sent, unlocking this bug.