(no longer active) Reporter
	Description • 13 years ago

IIRC it used to require that you enter your password on that page before...

	:glob ✱
	Comment 1 • 13 years ago

(In reply to comment #0)
> IIRC it used to require that you enter your password on that page before...

this was changed in bug 553693 (one and a quarter years ago).

	Reed Loden [:reed]
	Comment 2 • 13 years ago

Why is 'old_login' even needed on this page? Bugzilla already has that info.

This is potentially a security issue, especially since you don't have to know the current password in order to change settings. Requesting blocking.

	Max Kanat-Alexander
	Comment 3 • 13 years ago

LpSolit: Your opinion on this?

	Reed Loden [:reed]
	Comment 4 • 13 years ago

If you're changing the actual password, the old password is required, but if you're just changing the e-mail address or the real name, no password is required (though you must enter *something* in the current password box if you want to change the e-mail address... doesn't actually have to be a password, though).

	Gervase Markham [:gerv]
	Comment 5 • 13 years ago

(In reply to comment #4)
> If you're changing the actual password, the old password is required, but if
> you're just changing the e-mail address or the real name, no password is
> required (though you must enter *something* in the current password box if
> you want to change the e-mail address... doesn't actually have to be a
> password, though).

That sounds wrong, one way or the other! Either you should have to enter a valid password (which I think is right; otherwise accounts left logged in can be hijacked) or you should be able to leave it blank.

Gerv

	Max Kanat-Alexander
	Comment 6 • 13 years ago

  Okay, I agree that we should require a password for changing the user account, based on the "you left this thing open in a public place" scenario.

  I may move these blocking flags if there's no patch and this bug is preventing a release.

	Frédéric Buclin
	Comment 7 • 13 years ago

I just came back from vacation, so I didn't investigate yet. If I can confirm the bug, I will fix it. Being able to change the real name without entering the password is not a bug. It has always been so, and is known to work that way for years.

	Frédéric Buclin
	Comment 8 • 13 years ago

OK, you need to enter your *valid* password to be able to change your email address. I tested in 3.6, 4.0 and 4.2. So this bug is invalid (WFM).

	Reed Loden [:reed]
	Comment 9 • 13 years ago

That's not entirely true at all.

Let's look at the code, shall we?

91     if ($user->authorizer->can_change_password
92         && ($oldpassword ne "" || $pwd1 ne "" || $pwd2 ne ""))
93     {
94         my $oldcryptedpwd = $user->cryptpassword;
95         $oldcryptedpwd || ThrowCodeError("unable_to_retrieve_password");
96 
97         if (bz_crypt($oldpassword, $oldcryptedpwd) ne $oldcryptedpwd) {
98             ThrowUserError("old_password_incorrect");
99         }

So, the only time the password check code is called is if the user has the ability to change his/her password. If not, this check is ignored.

Moving down in the code to where e-mail address changes happen:

118     if ($user->authorizer->can_change_email
119         && Bugzilla->params->{"allowemailchange"}
120         && $new_login_name)
121     {
122         if ($old_login_name ne $new_login_name) {
123             $oldpassword || ThrowUserError("old_password_required");

Note that the password isn't actually checked here. All you need to have is *something* in the old password field to get by this check.

Going to mark this as security again, though this will only occur if you're using an auth mechanism that denies can_change_password but allows can_change_email. Still, it's a bug, and we should fix it.

	Teemu Mannermaa (:wicked)
	Comment 10 • 13 years ago

Reed, did you test this? According to LpSolit, it does require a *valid* password to change certain things (and he did test it).

It's right that code you quoted doesn't seem to validate the password but look at the code starting at line 491 (http://mxr.mozilla.org/mozilla/source/webtools/bugzilla/userprefs.cgi#491) and especially lines 501 and 402. To me that looks like requiring a valid login to even run the code in SaveAccount() sub.

	Reed Loden [:reed]
	Comment 11 • 13 years ago

(In reply to comment #10)
> Reed, did you test this? According to LpSolit, it does require a *valid*
> password to change certain things (and he did test it).

For the common case, LpSolit is correct. However, from reading the code, I found a second case where that is not true. I'm not sure why somebody would have an auth mechanism that allows e-mail changes but not password changes, but it could happen for whatever reason. I have not tested that particular case, no, but it's fairly obvious from reading the code that there is an issue.

> It's right that code you quoted doesn't seem to validate the password but
> look at the code starting at line 491
> (http://mxr.mozilla.org/mozilla/source/webtools/bugzilla/userprefs.cgi#491)
> and especially lines 501 and 402. To me that looks like requiring a valid
> login to even run the code in SaveAccount() sub.

No, it requires a valid logincookie. The password itself doesn't have to be supplied. There is a difference. :)

	Dave Miller [:justdave]
	Comment 12 • 13 years ago

A situation where an auth mechanism doesn't allow password changes probably means it's either using an external auth mechanism (like a single-signon or http auth) or it using some other means to authenticate which doesn't use a password.  In either of those cases, Bugzilla's not going to know the password to authenticate it, and they've presumably already cleared the external auth in order to reach that page.

	Frédéric Buclin
	Comment 13 • 13 years ago

(In reply to comment #11)
> For the common case, LpSolit is correct. However, from reading the code, I
> found a second case where that is not true. I'm not sure why somebody would
> have an auth mechanism that allows e-mail changes but not password changes,
> but it could happen for whatever reason.

This means that vanilla Bugzilla is not affected. Look at the Bugzilla::Auth and Bugzilla::Auth::* code, and you will notice that there exists no combination which has can_change_password = false and can_change_email = true at the same time. So we should stop alarming everybody when 99% of installations are not affected. Definitely not a blocker, and decreasing the severity accordingly. IMO, it's not even a (security) bug, but could rather be seen as an enhancement. mkanat, what's your opinion?

	Reed Loden [:reed]
	Comment 14 • 13 years ago

In Perl, especially with regards to CGI form values, is the difference between |if ($oldpassword)| vs. |if ($oldpassword ne "")|?

If there is any type of difference, then the original issue is back in play.

	Reed Loden [:reed]
	Comment 15 • 13 years ago

(In reply to comment #14)
> In Perl, especially with regards to CGI form values, is the difference
> between |if ($oldpassword)| vs. |if ($oldpassword ne "")|?

s/is the/is there a/

	Reed Loden [:reed]
	Comment 16 • 13 years ago

From http://perldoc.perl.org/CGI.html#FETCHING-THE-VALUE-OR-VALUES-OF-A-SINGLE-NAMED-PARAMETER:

"""
If a value is not given in the query string, as in the queries "name1=&name2=", it will be returned as an empty string.

If the parameter does not exist at all, then param() will return undef in a scalar context, and the empty list in a list context.
"""

So, what stops me from doing "old_password=&otherparam=" ..., which fails the |if ($old_password ne "")| check, as $old_password is an empty string, yet passes the |if ($old_password)" check, as $old_password is defined?

Please do correct me if I'm not understanding this correctly.

	Frédéric Buclin
	Comment 17 • 13 years ago

(In reply to comment #16)
> Please do correct me if I'm not understanding this correctly.

Line 123: $oldpassword || ThrowUserError("old_password_required").

You *must* pass a password. So you cannot bypass security checks using your trick.

	Dave Miller [:justdave]
	Comment 18 • 13 years ago

(In reply to comment #16)
> So, what stops me from doing "old_password=&otherparam=" ..., which fails
> the |if ($old_password ne "")| check, as $old_password is an empty string,
> yet passes the |if ($old_password)" check, as $old_password is defined?
> 
> Please do correct me if I'm not understanding this correctly.

|if ($old_password)| checks boolean-ness of the value, not whether it's defined or not.  If it's defined, but empty, it'll still be considered "false".  To check if it's defined or not, you actually have to use |if (defined($old_password))|.

	Frédéric Buclin
	Comment 19 • 13 years ago

(In reply to comment #12)
> password.  In either of those cases, Bugzilla's not going to know the
> password to authenticate it

And this means we cannot validate any password! That's why I don't consider this bug as a security bug, nor as something I would see fixed on branches.

	Frédéric Buclin
	Comment 20 • 13 years ago

Bug 553693 is not responsible for this "bug".

	Max Kanat-Alexander
	Comment 21 • 13 years ago

(In reply to comment #13)
> mkanat, what's your opinion?

  If it's not possible to trigger this in the version of Bugzilla that we ship, then we should fix it, but only as an enhancement for trunk versions of Bugzilla.

  If somebody can confirm that this is not possible to trigger in the version of Bugzilla as we ship it, then you can remove the security flag. Security issues that cannot be exploited do not go through our security process, but they should still be fixed for future versions of Bugzilla.

	Dave Miller [:justdave]
	Comment 22 • 13 years ago

See also bug 670868 which seems strongly related. :|

	Frédéric Buclin
	Comment 25 • 9 years ago

Comment on attachment 8588417 [details] [diff] [review]
bug670669-v1.patch

>+            my $oldcryptedpwd = $user->cryptpassword;
>+            $oldcryptedpwd || ThrowCodeError("unable_to_retrieve_password");
>+
>+            if (bz_crypt($oldpassword, $oldcryptedpwd) ne $oldcryptedpwd) {
>+                ThrowUserError("old_password_incorrect");
>+            }

This looks good, but instead of duplicating this code, please write a subroutine, e.g. check_old_password(). Also, you shouldn't call this code twice if it has already been run before.

	Gervase Markham [:gerv]
	Comment 26 • 9 years ago

Comment on attachment 8588417 [details] [diff] [review]
bug670669-v1.patch

What LpSolit said :-)

Gerv

	Simon Green Assignee
	Comment 27 • 9 years ago

With the changes suggested by LpSolit

	Frédéric Buclin
	Comment 28 • 9 years ago

Comment on attachment 8635644 [details] [diff] [review]
bug670669-v2.patch

>+++ b/Bugzilla/User.pm

>+sub verify_password {
>+    my $self = shift;
>+    my $password = shift;
>+
>+    my $cryptpwd = $self->cryptpassword;
>+    return bz_crypt($password, $cryptpwd) eq $cryptpwd ? 1 : 0;
>+}

Where is the test for $oldcryptedpwd || ThrowCodeError("unable_to_retrieve_password")? If this test is no longer needed, then unable_to_retrieve_password should be removed from code-error.none.tmpl. Else it should be restored.
Also, this method is the right place to check $oldpassword || ThrowUserError("old_password_required") instead of leaving that check in userprefs.cgi.
And if passwords do not match, then this method should also call ThrowUserError("old_password_incorrect") itself. This will make the code in userprefs.cgi much cleaner.
And it should be named check_password() or check_old_password(), as we generally use check_ for validators which can throw errors.



>+++ b/userprefs.cgi

>+    my $verified_password = undef;

No need to specify "= undef". You can simply write "my $foo;" as we do everywhere else.


>+        $oldpassword || ThrowUserError("old_password_required");

This should go into verify_password().


>+        unless ($verified_password //= $user->verify_password($oldpassword)) {
>             ThrowUserError("old_password_incorrect");
>         }

At this point, $verified_password is known to be undefined, so //= doesn't make sense here. You can simply write:

  $user->verify_password($oldpassword);
  $verified_password = 1;

Here, I assumed the call to ThrowUserError("old_password_incorrect") has been moved into verify_password() (well, its renamed method s/validate/check/).


>             $oldpassword || ThrowUserError("old_password_required");

Same here, this check should go into verify_password().


>+            unless ($verified_password //= $user->verify_password($oldpassword)) {
>+                ThrowUserError("old_password_incorrect");
>+            }

Simply write: $verified_password || $user->verify_password($oldpassword); because if it was 0, then an error would have been thrown already and so $verified_password cannot be 0 at this point. So it's either undef, in which case we want to call this method, or 1, in which case it's ok.

	Simon Green Assignee
	Comment 29 • 9 years ago

The reason I didn't use check_password was to avoid confusion with _check_password (the validator for the password field). I didn't like check_old_password either, since the function checks the current password. I am happy to rename it to check_current_password though. This makes it clear that it is different from _check_password.

I'll do a new patch tomorrow.

	Frédéric Buclin
	Comment 30 • 9 years ago

(In reply to Simon Green from comment #29)
> I'll do a new patch tomorrow.

Tomorrow of which week? :) I need check_current_password() for bug 1185241 to avoid duplicating code.

	Simon Green Assignee
	Comment 31 • 9 years ago

Sorry.  Work got in the way. I'll do it. Some time in the next 24 hours,  promise.

	David Lawrence [:dkl]
	Comment 33 • 9 years ago

Comment on attachment 8638473 [details] [diff] [review]
bug670669-v3.patch

Review of attachment 8638473 [details] [diff] [review]:
-----------------------------------------------------------------

r=dkl

	Frédéric Buclin
	Comment 34 • 9 years ago

Per comment 19 and comment 21, I wouldn't take it for 5.0.

	Simon Green Assignee
	Comment 35 • 9 years ago

(In reply to Frédéric Buclin from comment #34)
> Per comment 19 and comment 21, I wouldn't take it for 5.0.

Fair enough. I didn't read all the comments this morning.

	Frédéric Buclin
	Comment 36 • 9 years ago

Please commit this patch and remove the security flag. It will land in master only, so no need to wait for a release.

	Simon Green Assignee
	Comment 37 • 9 years ago

To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   d3a74a9..19d20ef  master -> master