Closed Bug 670868 (CVE-2011-2978) Opened 9 years ago Closed 8 years ago

[SECURITY] Account preferences page trusts user-modifiable field for obtaining current e-mail address

Categories

(Bugzilla :: User Accounts, defect, major)

2.16
defect
Not set
major

Tracking

()

RESOLVED FIXED
Bugzilla 3.4

People

(Reporter: reed, Assigned: glob)

References

Details

Attachments

(2 files)

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.
Flags: blocking4.2?
Flags: blocking4.0.2?
Flags: blocking3.6.6?
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.
the fix is pretty simple; with it you can no longer redirect the confirmation email.
Assignee: user-accounts → glob
Status: NEW → ASSIGNED
Attachment #545336 - Flags: review?(LpSolit)
Also should remove 'old_login' from the account prefs form.
(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.
What refactoring? Just remove the hidden 'old_email' field from the page.
er, 'old_login'.
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?
Wow, WTF.
Flags: blocking4.2?
Flags: blocking4.2+
Flags: blocking4.0.2?
Flags: blocking4.0.2+
Flags: blocking3.6.6?
Flags: blocking3.6.6+
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?
(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 :)
(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?
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 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
Attachment #545336 - Flags: review?(LpSolit) → review+
Flags: approval?
Flags: approval4.0?
Flags: approval3.6?
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.
Flags: blocking3.4.12+
Target Milestone: Bugzilla 3.6 → Bugzilla 3.4
Attachment #545336 - Attachment description: patch v1 → patch for 3.6 and higher, v1
Blocks: 660528
Attachment #547624 - Flags: review?(LpSolit)
Comment on attachment 547624 [details] [diff] [review]
patch for 3.4, v1

r=LpSolit
Attachment #547624 - Flags: review?(LpSolit) → review+
Flags: approval3.4?
Use CVE-2011-2978 for this bug
Alias: CVE-2011-2978
From what I can see, this problem exists since users can change their email address, which happened in 2.16rc1, see bug 23067.
Version: 4.0.1 → 2.16
Flags: approval?
Flags: approval4.0?
Flags: approval4.0+
Flags: approval3.6?
Flags: approval3.6+
Flags: approval3.4?
Flags: approval3.4+
Flags: approval+
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.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Security advisory sent, unlocking this bug.
Group: bugzilla-security
You need to log in before you can comment on or make changes to this bug.