211006 - (bz-sha256) Bugzilla should not use crypt to store hashed passwords in the database

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, defect, P1)

Description

[Brant Gurganus]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4) Gecko/20030624
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4) Gecko/20030624

I wrote bogus text in the current password box and changed my user name. 
However, Bugzilla accepted the change.

Reproducible: Always

Steps to Reproduce:
1. Use bogus text as your current password.
2. Change your real name.
3. Submit
Actual Results:  
Bugzilla makes the changes.

Expected Results:  
Bugzilla should not make the change.

I think the bogus text I was using began with my actual password so if it was
foo, I enterered foobar.

This is a security issue so setting security flag.  This is also a critical issue.

Comment 1

[Dave Miller [:justdave]]

If your password is more than 8 characters long and the junk you put in there
did begin with your regular password, then this is normal, since unix crypt only
looks at the first 8 characters of the password.  (I think it's 8 characters).

Note that it's actually doing a login with that password on that screen, so you
should be able to reproduce this on the main login screen as well.  Can you?

Comment 2

[Brant Gurganus]

I was able to reproduce it on the main login screen of both
landfill.bugzilla.org and bugzilla.mozilla.org installations.  The password I am
using is eight characters.  I just discovered that despot has the same
vulnerability and was reported in bug 211011.

Comment 3

[Dave Miller [:justdave]]

Most unix systems used this method for passwords until very recently, and many
of them still do.  This is among the reasons some of them (RedHat that I know
of) are using MD5 instead of Crypt on system passwords now.

Find a linux/unix distribution that still uses crypt for passwords and try this
same trick there.  I bet you can get in with it.  Very few people have passwords
that are 8 characters or longer.  And for those that do, you still have to
figure out what those first 8 characters are.  If you've managed to figure out
that big of a chunk of the password, you probably won't have a hard time
figuring out the rest anyway.  Is this even worth trying to fix?

There's two ways I know of to fix this...   
a) use MD5 instead of Crypt
b) limit password entry to 8 characters, since that's all that counts anyway.
c) don't do anything...  as mentioned, is this even worth it?

Comment 4

[Brant Gurganus]

I don't think think your option (b) should be considered as it would reduce
security.  Consider, for example, that I log-in at a library.  Somebody sees
that I typed 10 characters.  If they are going to try to break in, that is what
they think they need.  Then, their password generation mechanism potentially
takes 128^2 times as long to "break" the password.  If (b) were implemented, it
would remove those illusive 128^2 times.

In reading some Web pages regarding this issue at the crypt level, most talked
about how crypt needs to allow longer passwords.  Most did find MD5 as the
solution, so if MD5 is a feasible option, then it should be considered.

Comment 5

[Dave Miller [:justdave]]

This wouldn't require any schema changes, because the cryptpasswd field was
already made big enough to allow for MD5 data on the assumption that some
implementations of Perl might actually use it for the crypt() command.

However, conversion would be pretty interesting to accomplish, since short of
brute force, we can't really decrypt the existing passwords in order to recrypt
them.  We would probably have to resort to allowing crypt() passwords to
continue to work, but recrypt them with MD5 the next time the user logs in. 
Which potentially leaves users in the system with crypt() passwords forever if
they don't log in again.

Comment 6

[Dave Miller [:justdave]]

I'm going to remove the security flag on this bug, because although this is a
security issue, it's not something that's going to immediately allow someone's
Bugzilla to get compomised if people know about it.  People who would exploit
this type of thing would have already figured it out by looking at the freely
available source code, and it still involves brute force or good guessing to
crack.  Good guessing we won't be able to do anything about, since passwords are
only as strong as the user makes them.  But this is a very worthwhile
enhancement request to make the brute force methods that much more difficult. 
With this bug open, we're more likely to get code contributions, too. :)

Something we might be able to do is have checksetup set the cryptpassword field
on all accounts to some known non-encrypted value on all accounts.  When the
login code finds that value, it informs the user why their password has been
invalidated, and requests them to choose the "reset my password" option to have
a token mailed to them.

Alternatively, we could just fall back on crypt() if the MD5 validation fails,
and if the crypt() passes, transparently rehash the password with MD5 and store
it.  This will leave crypted passwords around on inactive accounts if nobody
ever logs in with them, though.

Perhaps we could offer both options and let the sysadmin choose which policy
they want to follow at upgrade time.

Comment 7

[Dave Miller [:justdave]]

Enhancements which don't currently have patches on them which are targetted at
2.18 are being retargetted to 2.20 because we're about to freeze for 2.18. 
Consideration will be taken for moving items back to 2.18 on a case-by-case
basis (but is unlikely for enhancements)

Comment 8

[Tobias Sager]

Please keep in mind that MD5 has not to be the first choice hash function.
There are a few others that should be considered.
(Especially as there are rumors of collisions in MD5 and RIPEMD-128...)

Maybe SHA-1 (yet SHA-0 has been broken) or RIPEMD-160?

Comment 9

[Dave Miller [:justdave]]

Bugzilla 2.20 feature set is now frozen as of 15 Sept 2004.  Anything flagged
enhancement that hasn't already landed is being pushed out.  If this bug is
otherwise ready to land, we'll handle it on a case-by-case basis, please set the
blocking2.20 flag to '?' if you think it qualifies.

Comment 10

[Dave Miller [:justdave]]

*** Bug 285906 has been marked as a duplicate of this bug. ***

Comment 11

[Dave Miller [:justdave]]

*** Bug 286832 has been marked as a duplicate of this bug. ***

Comment 12

[Dave Miller [:justdave]]

OK, we were just chatting this up on IRC a bit because the guy who filed the
last bug I just duped to this one was there at the time :)

Here's how we should probably go about doing this:

When a user logs in with an account that still has a crypt() password, and we
successfully validate it with crypt(), then:
 - if the password they entered was 8 characters or less, automatically md5sum
it and store it in the DB without them knowing the difference.
 - if the password they entered was 9 characters or longer, pretend we just
issued them a password reset token and they clicked the link, and toss them into
the "set a new password" page with a message at the top something to the effect
of "Bugzilla previously used a password encryption method that is no longer
considered secure.  For improved security we need to re-encrypt your password
with a more secure mechanism, and in order to do that we need you to enter a new
password.  It can be the same as your old one, but doesn't need to be." or
something to that effect.  The reason for this is to avoid the case where
someone has a 10-character password and typos something in the last 2 characters
of it that one time.  crypt() wouldn't have noticed and would log them in
anyway, and then what they thought was there password would no longer work.

We also need to provide a mechanism where the admin can say "invalidate the
passwords for anyone who still has a password encrypted with crypt()" for those
who are paranoid.  Presumably on most systems, admins will let it go for a month
or two to let everyone transition before invoking this.

Comment 13

[Dave Miller [:justdave]]

Or maybe we could scare users less by stating something like "Your password has
expired, please enter a new one"  :)

Comment 14

[Max Kanat-Alexander]

Or we could just say, "Please reconfirm your password one more time. This will
be the only time you will be asked to do this, and it will improve the security
of your account."

Comment 15

[Dave Miller [:justdave]]

That starts to sound like a phisher managed to get into our site :)  (my
original version was worse for that)

Comment 16

[Brant Gurganus]

"one more time" is also misleading.  Already there are known problems with MD5
and I believe also with SHA-1 so there likely will be a desire for moving to a
stronger hash in the future.  To avoid sounding like some sort of scam or phish
to the paranoid, it could probably be preanounced in the broadcast header as is
done prior to an upgrade of bugzilla, at least for the Mozilla implementation.

To make the change sound less like a phish, I cannot come up with a "good" idea,
but some ideas that would help are to make the change after an upgrade which
changes like this would generally occur, cite an announcement from the Bugzilla
project of the Mozilla Foundation for user-oriented details and the bug for
administrator-oriented details.

Comment 17

[Gervase Markham [:gerv]]

Here's a way of fixing it without alarming the user with a scary message, and
yet avoiding the problem of a one-time typo locking the person out.

If someone logs in with a crypted password, do the MD5sum and store it. Keep
using crypt() for that user.

Next time, if they log in with a password that hashes to the same value,
increment a per-user counter. When that counter gets to 3, start using the
MD5summed version permanently. If the password hashes to a different value,
store the new value and reset the counter. In other words, they need exactly the
same password 3 times in succession before a switch.

This scheme would actually be fairly simple to implement; the only disadvantage
is that it makes an admin auto-logout everyone multiple times over a month or so
in order to get their counts up to 3. Perhaps 2 would be enough?

Gerv

Comment 18

[Olav Vitters]

*** Bug 290551 has been marked as a duplicate of this bug. ***

Comment 19

[Frédéric Buclin]

It would be fine to have it for 2.24. We now have several dupes complaining that only the first 8 characters of the password are considered.

Comment 20

[Max Kanat-Alexander]

*** Bug 341244 has been marked as a duplicate of this bug. ***

Comment 21

[Frédéric Buclin]

This bug is retargetted to Bugzilla 3.2 for one of the following reasons:

- it has no assignee (except the default one)
- we don't expect someone to fix it in the next two weeks (i.e. before we freeze the trunk to prepare Bugzilla 3.0 RC1)
- it's not a blocker

If you are working on this bug and you think you will be able to submit a patch in the next two weeks, retarget this bug to 3.0.

If this bug is something you would like to see implemented in 3.0 but you are not a developer or you don't think you will be able to fix this bug yourself in the next two weeks, please *do not* retarget this bug.

If you think this bug should absolutely be fixed before we release 3.0, either ask on IRC or use the "blocking3.0 flag".

Comment 23

[İsmail Dönmez]

bz_crypt function in Bugzilla/Util.pm :

[...]
    my $salt = ''; <--- HERE
    for ( my $i=0 ; $i < 8 ; ++$i ) {
        $salt .= $saltchars[rand(64)];
    }

    # Crypt the password.
    my $cryptedpassword = crypt($password, $salt);
[...]
}

According to crypt(3), salt should start with $1$ to use MD5 so checking the above code to do 

 my $salt = '$1$'; 

should fix the problem.

Comment 24

[Max Kanat-Alexander]

(In reply to comment #23)
> According to crypt(3), salt should start with $1$ to use MD5 so checking the
> above code to do 
> 
>  my $salt = '$1$'; 
> 
> should fix the problem.

  You should really read the whole bug.

  Also, we support non-unix platforms. (But that's OK, Digest::MD5 ships with Perl.)

Comment 26

[Max Kanat-Alexander]

Bugzilla 3.2 is now frozen. Only enhancements blocking 3.2 or specifically approved for 3.2 may be checked in to the 3.2 branch. If you would like to nominate your enhancement for Bugzilla 3.2, set the "blocking3.2" flag to "?", and either the target milestone will be changed back, or the blocking3.2 flag will be granted, if we will accept this enhancement for Bugzilla 3.2.

Comment 29

[Max Kanat-Alexander]

Okay, I have a client who is going to fund Everything Solved to implement this for Bugzilla 3.4.

Comment 30

[Max Kanat-Alexander]

Honestly, I think some of the suggestions are going pretty far for the case where somebody typos their password once--we do have the password-reset ability for that, which is fairly easy to use. But I'll see how easy everything is to do, and whether or not it adds complexity.

Comment 31

[Max Kanat-Alexander]

Attachment: v1

Okay, this patch uses SHA-256 to store passwords in the database. It converts passwords currently using crypt() to SHA-256 after a successful login. I think the case where somebody typos their password after the eighth character is rare enough that we can just depend on the standard password reset feature for that.

I picked SHA-256 after some research indicating that it's probably the best way to go. It requires the Digest::SHA module, which is included with Perl 5.10, but for Perl 5.8, you have to get it from CPAN.

We still use an eight-character salt, so that our salts are the same between our old crypt implementation and our new SHA implementation.

We also store the algorithm we're using along with the crypted password, so that we can change it more easily in the future if we want to (or so that local installations can use different algorithms if they want to).

Now, I need to write a contrib/ script that allows the admin to send password reset tokens to all users who are still using crypt.

Comment 32

[Frédéric Buclin]

We use crypt() by design. I don't see how this can be a major bug (per the definition of "major").

Comment 33

[Frédéric Buclin]

(In reply to comment #31)
> to go. It requires the Digest::SHA module, which is included with Perl 5.10,
> but for Perl 5.8, you have to get it from CPAN.

Digest::SHA is not installed by default with Perl 5.10, at least not in Mandriva Linux. So this means again another Perl module to install to get Bugzilla working. That's irritating.

Comment 34

[Max Kanat-Alexander]

(In reply to comment #33)
> Digest::SHA is not installed by default with Perl 5.10, at least not in
> Mandriva Linux. So this means again another Perl module to install to get
> Bugzilla working. That's irritating.

  If that's the case, then Mandriva has removed it. That's like saying that CGI isn't installed by default in Mandriva, so we shouldn't require it. :-)

Comment 35

[Reed Loden [:reed]]

(In reply to comment #32)
> We use crypt() by design. I don't see how this can be a major bug (per the
> definition of "major").

I'm not sure how you can sanely call fixing a major security problem with the way Bugzilla stores passwords an "enhancement". Using crypt() is a _bug_ and should be treated as such.

(In reply to comment #33)
> Digest::SHA is not installed by default with Perl 5.10, at least not in
> Mandriva Linux. So this means again another Perl module to install to get
> Bugzilla working. That's irritating.

Considering the protection it adds to Bugzilla's users, I don't see this as a problem at all. Also, it's sad that such a module isn't included by default in Perl, but if it's not, then that still shouldn't stop us from requiring it for the sake of the security of our users.

Comment 36

[Frédéric Buclin]

(In reply to comment #35)
> I'm not sure how you can sanely call fixing a major security problem with the
> way Bugzilla stores passwords an "enhancement". Using crypt() is a _bug_ and
> should be treated as such.

We clearly don't have the same definition of bug.

Comment 37

[Max Kanat-Alexander]

Attachment: Tool for resetting passwords, v1

Okay, here is a tool to automatically send password reset tokens to all users who are still using crypt(). It also allows you to (optionally) set the users' passwords to "*" so that they can't log in until they do the password reset.

Eventually we could also expose this in the admin UI, so that admins could reset users' passwords with an appropriate email (which is a feature that has been requested).

Comment 38

[A. Shimono [:himorin]]

(In reply to comment #33)
> > to go. It requires the Digest::SHA module, which is included with Perl 5.10,
> > but for Perl 5.8, you have to get it from CPAN.
> 
> Digest::SHA is not installed by default with Perl 5.10, at least not in
> Mandriva Linux. So this means again another Perl module to install to get
> Bugzilla working. That's irritating.

I think Digest::SHA is one of the core modules. If you said about 5.8, yes.
http://search.cpan.org/~rgarcia/perl-5.10.0/pod/perl5100delta.pod#New_modules
> Digest::SHA is a module used to calculate many types of SHA digests,
> has been included for SHA support in the CPAN module.

(In reply to comment #35)
> I'm not sure how you can sanely call fixing a major security problem with the
> way Bugzilla stores passwords an "enhancement". Using crypt() is a _bug_ and
> should be treated as such.

I don't think so, neither.
We do NOT expose the hashed password to outer side.

Comment 39

[Max Kanat-Alexander]

(In reply to comment #38)
> I don't think so, neither.
> We do NOT expose the hashed password to outer side.

  Accepting passwords longer than eight characters but not actually acting on anything but the first eight characters is a bug.

Comment 40

[Frédéric Buclin]

Comment on attachment 354320 [details] [diff] [review]
v1

>Index: Bugzilla/Constants.pm

>+# of your users will be able to log in until thye reset their passwords.

Nit: s/thye/they/



>Index: Bugzilla/Util.pm

> sub bz_crypt {

Please update the POD for this function. It still mentions crypt().


>-    # Wide characters cause crypt to die

Digest::* is not affected by wide characters?



>Index: Bugzilla/Auth/Verify/DB.pm

>+    # If their old password was using crypt(), convert it to using the
>+    # more modern hashing system we use now. We know if was using crypt
>+    # if it doesn't end with something like "{SHA-256}".
>+    if ($real_password_crypted !~ /}$/) {

We should do this if the algorithm stored in the crypted password doesn't match PASSWORD_DIGEST_ALGORITHM. This way, this will also work in the future.


r=LpSolit, but please fix the comments above.

Comment 41

[Frédéric Buclin]

a=LpSolit with the review comments fixed.

Comment 42

[Max Kanat-Alexander]

Okay, I did all the checkin fixes that you mentioned. And yes, Digest is fine with wide characters, I tested during the patch-writing process. :-)

I'm going to attach the other patch here to a separate bug, so that we don't have an open review request on a fixed bug.

Checking in Bugzilla/Constants.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Constants.pm,v  <--  Constants.pm
new revision: 1.100; previous revision: 1.99
done
Checking in Bugzilla/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v  <--  Util.pm
new revision: 1.79; previous revision: 1.78
done
Checking in Bugzilla/Auth/Verify/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Verify/DB.pm,v  <--  DB.pm
new revision: 1.10; previous revision: 1.9
done
Checking in Bugzilla/Install/Requirements.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/Requirements.pm,v  <--  Requirements.pm
new revision: 1.55; previous revision: 1.54
done

Comment 43

[Max Kanat-Alexander]

Attachment: Checked-In Version

Here's what was actually checked in. Carrying forward LpSolit's r+.

Comment 44

[Max Kanat-Alexander]

Comment on attachment 354355 [details] [diff] [review]
Tool for resetting passwords, v1

This patch has been moved to bug 471844.

Comment 45

[Max Kanat-Alexander]

Added to the release notes for Bugzilla 3.4 in bug 494037.