Re: [aur-dev] FS#17109: AUR passwords are not salted
Loui Chang wrote:
Thank you for your suggestions. These are things that are best discussed on the aur-dev mailing list.
Seems like you've put some thought into this. Why don't you submit a patch?
Thanks for your support. Here goes an attempt. I have mixed my suggestions with Denis idea of changing the hash algorithm at the same time plus a few bits I found on the way. Actions performed by this patch: salt to be NULL, in which case it is treated as md5 hashed password. *All entries can be automatically updated by a -to be written- script. *Removes add salt on login code, per above. *Salted passwords now use sha512 instead of md5. *Adds requirement on hash extension (usually bundled as static). *try_login() only performs one query to verify user login instead of 5. *generate_salt now uses mt_rand() *Reject passwords given by GET.
On Mon 19 Apr 2010 15:39 +0200, Linas wrote:
Loui Chang wrote:
Thank you for your suggestions. These are things that are best discussed on the aur-dev mailing list.
Seems like you've put some thought into this. Why don't you submit a patch?
Thanks for your support. Here goes an attempt. I have mixed my suggestions with Denis idea of changing the hash algorithm at the same time plus a few bits I found on the way.
Actions performed by this patch: salt to be NULL, in which case it is treated as md5 hashed password. *All entries can be automatically updated by a -to be written- script. *Removes add salt on login code, per above. *Salted passwords now use sha512 instead of md5. *Adds requirement on hash extension (usually bundled as static). *try_login() only performs one query to verify user login instead of 5. *generate_salt now uses mt_rand() *Reject passwords given by GET.
Sorry for the late response. Ideally each of these actions should be a separate patch. That would enable changes that are straightforward to be applied quickly, while the other changes might still need review. The -to be written- script also needs to be included for the change to be complete. Cheers.
Loui Chang wrote:
On Mon 19 Apr 2010 15:39 +0200, Linas wrote:
Loui Chang wrote:
Thanks for your support. Here goes an attempt. I have mixed my suggestions with Denis idea of changing the hash algorithm at the same time plus a few bits I found on the way.
Actions performed by this patch: salt to be NULL, in which case it is treated as md5 hashed password. *All entries can be automatically updated by a -to be written- script. *Removes add salt on login code, per above. *Salted passwords now use sha512 instead of md5. *Adds requirement on hash extension (usually bundled as static). *try_login() only performs one query to verify user login instead of 5. *generate_salt now uses mt_rand() *Reject passwords given by GET.
Sorry for the late response. Ideally each of these actions should be a separate patch.
That would enable changes that are straightforward to be applied quickly, while the other changes might still need review. The -to be written- script also needs to be included for the change to be complete.
Cheers.
Ok. I have splitted it on three patches. The first patch with the changes to acctfuncs.inc, which speeds up try_login, affect the lazy salting, and changes the $_REQUEST to $_POST. A second patch adding the mt_rand (the one-line change to generate_salt). A third patch actually using sha512 (salted_hash function, aur-schema.sql and README) with a format that allows the script update to work. Should the update script try to perform the alter table itself? If so, assuming that it has the Salt field (ie. Kobozev change was applied to the db) or not?
On Wed 26 May 2010 22:22 +0200, Linas wrote:
Loui Chang wrote:
On Mon 19 Apr 2010 15:39 +0200, Linas wrote:
Loui Chang wrote:
Thanks for your support. Here goes an attempt. I have mixed my suggestions with Denis idea of changing the hash algorithm at the same time plus a few bits I found on the way.
Actions performed by this patch: salt to be NULL, in which case it is treated as md5 hashed password. *All entries can be automatically updated by a -to be written- script. *Removes add salt on login code, per above. *Salted passwords now use sha512 instead of md5. *Adds requirement on hash extension (usually bundled as static). *try_login() only performs one query to verify user login instead of 5. *generate_salt now uses mt_rand() *Reject passwords given by GET.
Sorry for the late response. Ideally each of these actions should be a separate patch.
That would enable changes that are straightforward to be applied quickly, while the other changes might still need review. The -to be written- script also needs to be included for the change to be complete.
Ok. I have splitted it on three patches. The first patch with the changes to acctfuncs.inc, which speeds up try_login, affect the lazy salting, and changes the $_REQUEST to $_POST. A second patch adding the mt_rand (the one-line change to generate_salt). A third patch actually using sha512 (salted_hash function, aur-schema.sql and README) with a format that allows the script update to work.
Should the update script try to perform the alter table itself? If so, assuming that it has the Salt field (ie. Kobozev change was applied to the db) or not?
No structural changes have been applied to the db since at least the last code release. I'll apply the second patch. I'll still have to review the rest of it. * Messages generally should use i18n __("my message") * We should keep failed auth ambiguous: "wrong username or passwd" instead of "wrong passwd" * Please use `git format-patch` to generate patches. Thanks
Here's a patch with a script to salt passwords in the database. It assumes that there already a Salt field in the Users table. Hopefully it will integrated with Linas's patches. Linas, I think salted_hash() should not call md5() internally, otherwise it's not very useful to the script. You can take a look at the patch if I'm being ambiguous. Best, Denis.
Same thing, with trailing space removed. Best, Denis.
Denis Kobozev wrote:
Here's a patch with a script to salt passwords in the database. It assumes that there already a Salt field in the Users table. Hopefully it will integrated with Linas's patches.
Linas, I think salted_hash() should not call md5() internally, otherwise it's not very useful to the script. You can take a look at the patch if I'm being ambiguous.
Best, Denis.
My idea was to simply replicate the salted_hash() code in the script when writing it. Note that your patch is not incremental to mine, although it's another way to perform a scripty change. The functions changed are the previous ones, and I also took advantage of the opportunity of adding password salting for updating the hash to sha512. The query in addsalt() function should have a WHERE Salt IS NULL. That's nicer than checking it in php. __________________________________________________ Do You Yahoo!? Tired of spam? Yahoo! Mail has the best spam protection around http://mail.yahoo.com
On Fri, Jun 25, 2010 at 12:11 PM, Linas
Note that your patch is not incremental to mine...
No, it's not. The patch is against the current HEAD in the git repo, but it should play nicely with yours. The only incompatible change I introduced is that now you should do salted_hash(md5($password), $salt)) instead of just salted_hash($password, $salt).
The query in addsalt() function should have a WHERE Salt IS NULL. That's nicer than checking it in php.
Is it faster/more memory efficient/more readable? Best, Denis.
Denis Kobozev wrote:
The query in addsalt() function should have a WHERE Salt IS NULL. That's nicer than checking it in php.
Is it faster/more memory efficient/more readable?
Best, Denis.
Supposing that the script is only used once, it's irrelevant, since it will need to traverse the whole user table. If some rows were done and others don't (eg. you want to do it in batches), then it makes a different since filtering client side means retrieving each and every user, sending it over the net and then discard it whereas filtering on the db only the relevant rows are sent. __________________________________________________ Do You Yahoo!? Tired of spam? Yahoo! Mail has the best spam protection around http://mail.yahoo.com
On Fri, Jun 25, 2010 at 2:25 PM, Linas
Denis Kobozev wrote:
The query in addsalt() function should have a WHERE Salt IS NULL. That's nicer than checking it in php.
Is it faster/more memory efficient/more readable?
If some rows were done and others don't (eg. you want to do it in batches), then it makes a different since filtering client side means retrieving each and every user, sending it over the net and then discard it whereas filtering on the db only the relevant rows are sent.
That's assuming the host running the script and the host running MySQL are different machines? I realize it's not really important for this script, but it's definitely useful information. Best, Denis.
On Fri, Jun 25, 2010 at 12:11 PM, Linas
The query in addsalt() function should have a WHERE Salt IS NULL. That's nicer than checking it in php.
Done. Denis.
participants (3)
-
Denis Kobozev
-
Linas
-
Loui Chang