Closed
Bug 77473
Opened 23 years ago
Closed 23 years ago
mysql's encrypt is not DB independent
Categories
(Bugzilla :: Bugzilla-General, enhancement, P2)
Tracking
()
RESOLVED
FIXED
Bugzilla 2.14
People
(Reporter: kevin.brannen, Assigned: myk)
References
Details
Attachments
(11 files)
6.94 KB,
patch
|
Details | Diff | Splinter Review | |
14.46 KB,
patch
|
Details | Diff | Splinter Review | |
23.31 KB,
patch
|
Details | Diff | Splinter Review | |
24.96 KB,
patch
|
Details | Diff | Splinter Review | |
43.69 KB,
patch
|
Details | Diff | Splinter Review | |
43.69 KB,
patch
|
Details | Diff | Splinter Review | |
43.92 KB,
patch
|
Details | Diff | Splinter Review | |
29.24 KB,
patch
|
Details | Diff | Splinter Review | |
44.04 KB,
patch
|
Details | Diff | Splinter Review | |
28.63 KB,
patch
|
Details | Diff | Splinter Review | |
895 bytes,
patch
|
Details | Diff | Splinter Review |
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]
Reporter | ||
Comment 1•23 years ago
|
||
Comment 2•23 years ago
|
||
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.
Assignee: tara → ian
Component: Bugzilla → Bugzilla 3
Target Milestone: --- → Bugzilla 3.0
Reporter | ||
Comment 3•23 years ago
|
||
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•23 years ago
|
||
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.
Assignee: ian → tara
Component: Bugzilla 3 → Bugzilla
Priority: -- → P2
Target Milestone: Bugzilla 3.0 → Bugzilla 2.16
Comment 5•23 years ago
|
||
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
Assignee | ||
Comment 6•23 years ago
|
||
this should get retargeted to 2.14 since it blocks 2.14 bug 74032.
Updated•23 years ago
|
Target Milestone: Bugzilla 2.16 → Bugzilla 2.14
Comment 7•23 years ago
|
||
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 ;).
Updated•23 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•23 years ago
|
||
Assignee | ||
Comment 9•23 years ago
|
||
Assignee | ||
Comment 10•23 years ago
|
||
taking
Assignee | ||
Updated•23 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 11•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
> 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)
Blocks: 84572
Comment 15•23 years ago
|
||
er, make that bug 85472...
Assignee | ||
Comment 16•23 years ago
|
||
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
Assignee | ||
Comment 17•23 years ago
|
||
Assignee | ||
Comment 18•23 years ago
|
||
Dave, are you sure those passwords on your system were 35 and not 34 characters?
Comment 19•23 years ago
|
||
> 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•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
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. :-)
Assignee | ||
Comment 23•23 years ago
|
||
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•23 years ago
|
||
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.
Assignee | ||
Comment 25•23 years ago
|
||
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.
Assignee | ||
Comment 26•23 years ago
|
||
Assignee | ||
Comment 27•23 years ago
|
||
Comment 28•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
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 (webmaster@proton.bluemartini.com), giving this error message and the time and date of the error.
Comment 31•23 years ago
|
||
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•23 years ago
|
||
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 (webmaster@proton.bluemartini.com), giving this error message and the time and date of the error.
Comment 33•23 years ago
|
||
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?
Assignee | ||
Comment 34•23 years ago
|
||
Assignee | ||
Comment 35•23 years ago
|
||
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•23 years ago
|
||
Comment 37•23 years ago
|
||
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•23 years ago
|
||
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.
Assignee | ||
Comment 39•23 years ago
|
||
Comment 40•23 years ago
|
||
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•23 years ago
|
||
Comment 42•23 years ago
|
||
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•23 years ago
|
||
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.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 44•23 years ago
|
||
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 45•23 years ago
|
||
Comment 46•23 years ago
|
||
Installed patch on bz77473 on landfill and tested. Works as advertised and doesn't seem to produce any other problems. :-) r= justdave checked in.
Status: REOPENED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
Comment 47•23 years ago
|
||
Moving to Bugzilla product
Component: Bugzilla → Bugzilla-General
Product: Webtools → Bugzilla
Version: Bugzilla 2.11 → unspecified
Updated•12 years ago
|
QA Contact: matty_is_a_geek → default-qa
You need to log in
before you can comment on or make changes to this bug.
Description
•