Bug 670868 (CVE-2011-2978)

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

RESOLVED FIXED in Bugzilla 3.4

Status

()

Bugzilla
User Accounts
--
major
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: reed, Assigned: glob)

Tracking

2.16
Bugzilla 3.4
Bug Flags:
approval +
blocking4.2 +
approval4.0 +
blocking4.0.2 +
approval3.6 +
blocking3.6.6 +
approval3.4 +
blocking3.4.12 +

Details

Attachments

(2 attachments)

(Reporter)

Description

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

Comment 1

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

Comment 2

6 years ago
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.
Assignee: user-accounts → glob
Status: NEW → ASSIGNED
Attachment #545336 - Flags: review?(LpSolit)
(Reporter)

Comment 3

6 years ago
Also should remove 'old_login' from the account prefs form.
(Assignee)

Comment 4

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

Comment 5

6 years ago
What refactoring? Just remove the hidden 'old_email' field from the page.
(Reporter)

Comment 6

6 years ago
er, 'old_login'.
(Reporter)

Comment 7

6 years ago
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

6 years ago
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 9

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

Comment 10

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

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

6 years ago
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+

Updated

6 years ago
Flags: approval?
Flags: approval4.0?
Flags: approval3.6?

Comment 14

6 years ago
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

Updated

6 years ago
Attachment #545336 - Attachment description: patch v1 → patch for 3.6 and higher, v1

Updated

6 years ago
Blocks: 660528
(Assignee)

Comment 15

6 years ago
Created attachment 547624 [details] [diff] [review]
patch for 3.4, v1
Attachment #547624 - Flags: review?(LpSolit)

Comment 16

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

r=LpSolit
Attachment #547624 - Flags: review?(LpSolit) → review+

Updated

6 years ago
Flags: approval3.4?
Use CVE-2011-2978 for this bug
Alias: CVE-2011-2978

Comment 18

6 years ago
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

Updated

6 years ago
Flags: approval?
Flags: approval4.0?
Flags: approval4.0+
Flags: approval3.6?
Flags: approval3.6+
Flags: approval3.4?
Flags: approval3.4+
Flags: approval+

Comment 19

6 years ago
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
Last Resolved: 6 years ago
Resolution: --- → FIXED

Comment 20

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