77473 - mysql's encrypt is not DB independent

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement, P2)

Description

[Kevin Brannen]

By having the code use the builtin mysql encrypt function, one can't port the
code to other DBs (my goal).  This might also help other OS's.  [patch to be
attached]

Comment 1

[Kevin Brannen]

Attachment: remove mysql encryption function patch

Comment 2

[Dave Miller [:justdave]]

Retargetting for Bugzilla 3.

CCing barnboy to add a reference to this patch to the docs for people installing 
on NT and the like.

This won't make 2.x for one simple reason: mySQL on many systems uses a different 
crypt method than Perl does.  If you're on one of these systems, the passwords 
crypted with Perl won't necessarily match the passwords crypted with mySQL, and 
thus by installing this patch, you're immediately forcing everyone on your system 
to have their password reset.  Bugzilla doesn't do it for you, either.  It just 
mails you your existing password from the plaintext version that's right next to 
it in the database.  Except now it doesn't match so they still can't get in.  The 
admin would have to go reset them for everyone.

In principle, I think it's great, and it should have been done this way to begin 
with, but changing it now will cause too much damage to existing systems.

Comment 3

[Kevin Brannen]

Interesting about mysql crypt being different from perl's.  I'd expect them to
both use the same underlying function from libc or libcrypt.  Oh well ...

OTOH, how hard could it be to have checksetup.pl to run thru the DB and recrypt
all the passwords since the plaintext one is stored?  Then they'd be updated and
life could go on.

Also, are you saying I should not bother trying to make 2.x DB independent? 
That whatever work I'd do there would not be officially incorporated?  (I
suppose that's one way to get help for Ian, but is that best?)

Comment 4

[Dave Miller [:justdave]]

Heh, looking at this, I see I'm the one that retargetted this...

I'm changing my mind.  After discussing with a couple people in IRC, this could 
be done so that it won't break existing installs, because the database currently 
keeps plaintext passwords in the database.  As such, it would be easy to rehash 
the crypted passwords based on the plaintext ones during the checksetup.pl 
conversion routine.

Since the prospect of breaking existing installations was my primary reason for 
rejecting this before, I am renegging on my previous comment.  We should 
definitely consider this for 2.x since we can do it without breaking existing 
installs.

Comment 5

[Paul Sharpe]

On a related note logging in with the correct password failed for me after the
initial install (bugzilla-2_12/FreeBSD 3.3-RELEASE).  From the tests I did I
think the issue lies with the underlying crypt library functions.  My fix was to
patch CGI.pl to use the complete encrypted password as salt when crypting the
entered password i.e.

       my $sql = "SELECT
encrypt(".SqlQuote($enteredpwd).",".SqlQuote($realcryptpwd).")";
       ...

The Perl docs recommend this

% perldoc -f crypt

       crypt PLAINTEXT,SALT
          ...
               When verifying an existing encrypted string you
               should use the encrypted text as the salt (like
               `crypt($plain, $crypted) eq $crypted').  This
               allows your code to work with the standard `crypt'
               and with more exotic implementations.
          ...

I assume that FreeBSD is a 'more exotic implementation'.

hth

paul

Comment 6

[Myk Melez [:myk] [@mykmelez]]

this should get retargeted to 2.14 since it blocks 2.14 bug 74032.

Comment 7

[Jacob Steenhagen]

Looking at this patch I have the following comments:
 * sub encrypt appears in both CGI.pl and globals.pl - it only needs to be in 1
 * whitespace - my infamous comment as late :) - bugzilla uses the same block
   style for all blocks (that is, '{' appears on the same line that declares
   the block and '}' appears in the same column as the first letter of the
   block it's closing.  EG:
sub encrypt {
    # Code is indented 4 spaces
}
 * This also would need the code added to checksetup.pl to regenerate the
   cryptpassword function.

Please note that I didn't try this patch.  I also don't know enough about
crypt() to say without trying it that its functionally correct, but it does seem
to be complete (other than what's mentioned above ;). 

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Attachment: updated patch which also fixes bug 74032

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Attachment: actual updated patch (ignore prior one)

Comment 10

[Myk Melez [:myk] [@mykmelez]]

taking

Comment 11

[Myk Melez [:myk] [@mykmelez]]

The patch does a few things:

1. Uses "Crypt" instead of "encrypt" as the name of the function that crypts
user passwords because "encrypt" has a specific meaning different from "crypt"
on Unix systems.

2. Conforms to the Bugzilla white space and general formatting "standard"
although the more code I touch the harder it is for me to believe there is
anything approaching a formatting standard in Bugzilla.

3. Uses random salts to crypt passwords instead of the passwords themselves
since the first two characters of a crypted password are the plaintext salt, and
using the plaintext password as the salt thus reveals its first two characters
to anyone who gets hold of the crypted version.

4. When the user forgets their password and requests it by email, randomly
generates a new password for them because it is impossible to recover a
forgotten password without the plaintext password in the database.

Issues:

1. Any reason we limit password characters to letters, numbers, hyphens (-), and
underlines (_) and exclude other special characters like !,@,#,$,%,^,&, and *?

2. Crypted passwords are always 13 characters in length on my machine, so I
resized the cryptpassword field in the database to 13 characters from 64.  Is
this the same on other Unix systems?  Is there a [POSIX] standard or a maximum
length?

Comment 12

[Dave Miller [:justdave]]

Myk, remember the idea we were discussing in IRC last night about changing email 
addresses?  A similar method would be really cool for password resets.  Since the 
password will actually have to be reset if they forget it, it would be common 
courtesy, instead of resetting the password and emailing it to them, to email 
them a one-time-use token, and they have to come back and put that token in to 
reset the password.  This would prevent people from randomly resetting people's 
passwords on them.

Comment 13

[Dave Miller [:justdave]]

Another thought would be to have the old password continue to work until the new 
password is actually used.  Once the new password is used, the old one ceases to 
work.  If someone else used their email and hit reset on accident, and the user 
doesn't really want to change their password, they can just ignore the password 
change email.

Comment 14

[Dave Miller [:justdave]]

> 2. Crypted passwords are always 13 characters in length on my machine, so
> I resized the cryptpassword field in the database to 13 characters from 64.
> Is this the same on other Unix systems?  Is there a [POSIX] standard or a
> maximum length?

This will break systems that use MD5 for crypt().  MD5 encoded passwords 
(encrypted) vary in length.  Some on my system are 35 characters.  See bug 84572.  
(MD5 passwords were already broken because of the 2-character salt thing, which 
it looks like you fixed, since you're using the whole password as the salt now)

Comment 15

[Dave Miller [:justdave]]

er, make that bug 85472...

Comment 16

[Myk Melez [:myk] [@mykmelez]]

I found a web page that describes MD5 crypted passwords as strings of the form
"$foo$bar$baz", where "foo" is a version number, "bar" is a 12-48 bit salt, and
"baz" is a 128 bit hash of the password.

My RedHat 7.1. Linux system uses these passwords with 48-bit salts, and it
encodes the salt and hash using a variant of BASE64 that replaces the plus sign
"+" with a forward slash "/" for compatibility with older versions of crypt.

The result on my system is an 8 character salt and a 22 character hash.  The
version number, "1", is one character long, and there are three dollar signs
"$", for a total of 8+22+1+3 = 34 characters in the string.

MD5 hashes are uniformly 128-bits wide.

Unless other Unix systems use a different encoding mechanism (f.e. the
"Digest::MD5" Perl module has the ability to encode MD5 hashes into a
32-character hexadecimal string), I suggest making the "cryptpassword" field 34
characters long.  I further suggest using 48 bit (eight character) salts for
better security (I will test this for backwards compatibility with older
versions of crypt).

MD5: http://www.ietf.org/rfc/rfc1321.txt
BASE64: http://www.ietf.org/rfc/rfc1521.txt
MD5 Crypt: http://www.usenix.org/events/usenix99/provos/provos_html/node10.html

Comment 17

[Myk Melez [:myk] [@mykmelez]]

Attachment: updates to support MD5 crypt

Comment 18

[Myk Melez [:myk] [@mykmelez]]

Dave, are you sure those passwords on your system were 35 and not 34 characters?

Comment 19

[Matthew Tuck [:CodeMachine]]

> Another thought would be to have the old password continue to work until
> the new password is actually used.

I think the best way to do this would be to send the user an e-mail with a link
to click on to validate their new password, and providing instructions how to
contact the maintainer if they are receiving it in error.

Comment 20

[Tara Hernandez]

Patch is on landfill.  Initial inspection of mysql tables and general cgi looks
good. I would like to give this patch some more love from more people hopefully
smarter than I am to make sure we are good.

Also, it looks like we still have some issues to address regarding password
resets/notifications and supported characters within the password string itself.
 I currently don't have a firm opinion on the former, and the latter I'm sure
was an ease of implementation thing at the time, so can probably be expanded out
provided it doesn't take scary amounts of code to implement.

Comment 21

[Dave Miller [:justdave]]

You are correct, Myk, they're all 34.  The ones that were shorter (way shorter) 
are on disabled accounts, now that I'm looking.  I'm not sure where I got 35 
unless I just counted wrong.

Comment 22

[Dave Miller [:justdave]]

CREATE TABLE pending_tokens (
    userid mediumint not null,         <- user ID
    date_issued datetime not null,     <- time it was issued
    event_type varchar(8) not null,    <- 'password' or 'email'
    event_data tinytext not null,      <- new pass or new email address
    user_ack varchar(1),               <- Y or N for posit. or neg. ack
    user_date datetime,                <- time they acknowledged it
    newuser_ack varchar(1),            <- for new email in case of an email
    newuser_date datetime,             <-   change (ignored by passwd change)

    index (userid)
);

The newuser fields could be ignored by password changes.  Since the password one 
is just going to be a reset, event_data isn't really necessary for a password 
either.  Probably don't need something as big as a tinytext for the data, but I 
was trying to leave it open in case we came up with other types of events that 
needed it later.  We don't need to implement the email one right now, but I'm 
suggesting the table structure that allows it in order to make fixing the email 
one easier later. :-)

Comment 23

[Myk Melez [:myk] [@mykmelez]]

In one sense tokens are like one-time passwords giving users permission to do
things they would not otherwise be able to do.  In another sense they are
one-time passwords allowing users to confirm or cancel the things they (or
malicious hackers acting as them) have already done.

I think a much simpler design could meet our current token needs (username and
password changes) while providing room for future expansion.  On the other hand,
maybe I am missing some crucial piece of the puzzle, so take a look and let me
know what you think:

CREATE TABLE tokens (
    userid mediumint not null ,        <- user ID
    date_issued datetime not null ,    <- time it was issued
    token varchar(34) not null ,       <- random (crypted?) character sequence
    tokentype varchar(8) not null ,    <- 'password', 'oldemail', or 'newemail'

    index(userid)
);

Use Cases:

1. A user forgets his password.  He goes to the log in page, enters his
username, and clicks the "let me change my password" button.  A record is
created in the "tokens" table, and an email containing the token and links for
confirming or cancelling the request is mailed to the user.  The user receives
the email, clicks the link for confirming the request, receives a web form,
enters the token and his new password into the form, and submits it.  His record
in the "profiles" table is updated with his new password, and the token record
is deleted from the "tokens" table.

2. A malicious hacker goes to the log in page, enters a user's username, and
clicks the "let me change my password" button.  A record is created in the
"tokens" table, and an email containing the token and links for confirming or
cancelling the request is mailed to the user.  The user receives the email and
clicks the link for cancelling the request.  The token record is deleted from
the "tokens" table, and the system administrator is notified.

3. A user wants to change email addresses and still has access to her old
address.  She goes to her user preferences, enters her new address, and submits
the form.  Her record in the "profiles" table is updated with her new address,
two token records are created in the "tokens" table, and emails are sent to both
her old and new addresses, each email containing one of the tokens along with
links for confirming and cancelling the request.  She receives the emails and
clicks the links to confirm the request.  The token records are deleted from the
"tokens" table.

3. A user wants to change email addresses and no longer has access to her old
address.  She goes to her user preferences, enters her new address, and submits
the form.  Her record in the "profiles" table is updated with her new address,
two token records are created in the "tokens" table, and emails are sent to both
her old and new addresses, each email containing one of the tokens along with
links for confirming and cancelling the request.  She receives the email at her
new address and clicks the link to confirm the request.  The corresponding token
record is deleted from the "tokens" table.  After a specified period of time, an
automatic process (or possibly a trigger on subsequent log in) removes the token
record from the "tokens" table.

5. A malicious hacker gains access to a user's account and changes her email
address to his own address.  The user's record in the "profiles" table is
updated with the new address, two token records are created in the "tokens"
table, and emails are sent to both the user's and the hacker's address, each
email containing one of the tokens along with links for confirming and
cancelling the request.  The hacker receives an email and clicks the link to
confirm the request.  The corresponding token record is deleted from the
"tokens" table.  The user receives the email and clicks the link to cancel the
request.  Her email address reverts to her old address (embedded into the cancel
link), the corresponding token record is deleted from the "tokens" table, and
the system administrator is notified.

Comment 24

[Dave Miller [:justdave]]

Yep, that's pretty much what I had in mind.  I forgot when I was throwing that 
table spec together that the old address and new address would need different 
token numbers.

I do think that in the case of an email address change that the address shouldn't 
be changed in the profiles table until the tokens are both confirmed or the one 
to the old address expires.  Otherwise it would be easy for the "malicious 
hacker" to prevent someone from getting mail they should be getting, which would 
be a Bad Thing.  This would require an additional data field in the table beyond 
what you have defined.  However, other than adding that field so it's there when 
we do the email stuff later, the rest of how the email stuff works is pretty 
irrelevant to this bug.

Comment 25

[Myk Melez [:myk] [@mykmelez]]

Now that I think about it, a data field makes sense.  I'll add it to the
database design even if I don't need to use it for this bug.

Comment 26

[Myk Melez [:myk] [@mykmelez]]

Attachment: updated patch including token system

Comment 27

[Myk Melez [:myk] [@mykmelez]]

Attachment: updated patch including token system

Comment 28

[Dave Miller [:justdave]]

Myk, this patch is still missing the routines to re-crypt the passwords from the 
cleartext ones before dropping the cleartext password column from the database... 
I just tried it on landfill and was surprised to not see it do that.

Comment 29

[Dave Miller [:justdave]]

heh, you know what?  ignore that.  I copied the database from landfill's primary 
install and you're already running this patch there. :-)  I'll play some more 
before I make any more stupid comments :)

Comment 30

[Dave Miller [:justdave]]

If I hit the "reset my password" button and then click the link for "no, this 
wasn't me, cancel the request" in the email, I get the following error:

Software error:

SELECT issuedate , tokentype , eventdata , login_name , realname FROM tokens 
INNER JOIN profiles ON tokens.userid = profiles.userid WHERE token = 'UTLTNehw': 
You
have an error in your SQL syntax near 'ON tokens.userid = profiles.userid WHERE 
token = 'UTLTNehw'' at line 2 at globals.pl line 195. 

For help, please send mail to the webmaster ([e-mail]), 
giving this error message and the time and date of the error. 

Comment 31

[Dave Miller [:justdave]]

FWIW, the current version of this patch is now installed at
http://landfill.tequilarista.org/bz77473/

I had directory permission problems trying to update the advertised patch on the 
main install there.

Comment 32

[Dave Miller [:justdave]]

After having requested a password reset and ignoring it, I get the following 
error just trying to log in:

Software error:

SELECT issuedate , tokentype , eventdata , login_name , realname FROM tokens 
INNER JOIN profiles ON tokens.userid = profiles.userid WHERE token = 'UTLTNehw': 
You have an error in your SQL syntax near 'ON tokens.userid = profiles.userid 
WHERE token = 'UTLTNehw'' at line 2 at globals.pl line 195. 

For help, please send mail to the webmaster ([e-mail]), 
giving this error message and the time and date of the error. 

Comment 33

[Dave Miller [:justdave]]

Myk, I remember hearing you mention in IRC about updating the code on landfill to 
get this working...  I don't see a new patch here though. Is there one ready yet?

Comment 34

[Myk Melez [:myk] [@mykmelez]]

Attachment: updates to fix problems discovered on landfill

Comment 35

[Myk Melez [:myk] [@mykmelez]]

This latest patch fixes the SQL errors on landfill (ANSI standard INNER JOINs
are not supported on the older version of mysql installed on that machine) and a
few other errors as well (error message when logging out, and inability to
change your password using userprefs.cgi).  This patch is, of course, already
applied on landfill.

Comment 36

[Dave Miller [:justdave]]

Attachment: Patch v7

Comment 37

[Dave Miller [:justdave]]

The only change in the patch I just uploaded was (other than catching it up with 
the tip again) adding the line:

    print "$i... Done.\n";

right after the password rehashing routine.  If you run checksetup.pl on a system 
with less than 500 users, it's a bit disconcerting to see "Updating user #1... 
Deleting password column from profiles table".  Uhh, okay, what happened to all 
the users other than #1?  So this prints the last userID changed also.

Comment 38

[Dave Miller [:justdave]]

Only other comment after playing with this a little bit...

Since we're sending an email to the maintainer every time someone cancels a 
token...

1) We should record the IP address the original request was made from (and maybe 
the domain name if we know it)
2) We should record the IP address (and domain?) it was cancelled from.
3) Maybe we should send the email to the user instead of the admin, with 
instructions to forward the message to the admin if they think the request was 
malicious.

Comment 39

[Myk Melez [:myk] [@mykmelez]]

Attachment: v8: stores IP address as event data and sends email to user, not admin

Comment 40

[Dave Miller [:justdave]]

AAAarrrrggghhhhh....   so close!!

This version looks really good...  except that it breaks changing your password 
from userprefs.cgi if you have cookies off.  This was broken before and was fixed 
a few weeks ago in bug 45918.  This patch backs out the change made on that bug.

If that one item is fixed, this one has my approval.

Comment 41

[Dave Miller [:justdave]]

Attachment: Patch v9: restores fix from bug 45918

Comment 42

[Dave Miller [:justdave]]

OK, fixed that, I think...

Following protocol, since I fixed the patch, I need someone else to r= it now.  
This is live at http://landfill.tequilarista.org/bz77473/
Please play :-)

Comment 43

[Dave Miller [:justdave]]

r= Jake in irc, except that he pointed out I missed Token.pm and token.cgi in my
patch.

This has been checked in with those included.

Comment 44

[Dave Miller [:justdave]]

Uh oh...  Matty just got this on bugzilla-tip, and I've duplicated it...  it's
obviously a result of this checkin...

Software Error

SELECT token FROM tokens WHERE userid = LIMIT 1: You have an error in your SQL
syntax near 'LIMIT 1' at line 1 at globals.pl line 205.

This is after attempting to log in with an invalid username.

Comment 45

[Myk Melez [:myk] [@mykmelez]]

Attachment: tiny patch to fix reopened problem

Comment 46

[Dave Miller [:justdave]]

Installed patch on bz77473 on landfill and tested.  Works as advertised and 
doesn't seem to produce any other problems. :-)

r= justdave

checked in.

Comment 47

[Dave Miller [:justdave]]

Moving to Bugzilla product