Increase salt-length

[Christopher Kramer]

Originally reported on Google Code with ID 172

I think the length of the used salt is pretty short (6 chars if I remember right).
I don't see any good reason why we use such a short salt and would propose to use a
longer one (e.g. 32 chars).
I mean longer salts don't necessarily mean higher security, but as they are more or
less for free, I don't see a good reason against using a longer one. Okay, cookies
get some bytes larger and therefore http-requests, but come on... We don't even use
lots of images or such, so we don't have a lot of HTTP requests anyway.

Any comments?

(This issue has been mentioned first in issue #170.)

Reported by crazy4chrissi on 2013-02-09 21:25:06

[dreadnaut]

As far as I know, the only good reason to use a longer salt (since we store it on the
client anyway) is to keep the hashed 'salt+pass' string longer enough not to be found
in precalculated hash/rainbow tables.

Given that we don't require a minimum password length, anything above 16 characters
should give us a few years of relative safety :-p  32 is definitely better, but I wouldn't
overdo it and go for 20 or 24.

Reported by dreadnaut on 2013-02-10 14:05:58

[dreadnaut]

Reported by dreadnaut on 2013-02-10 14:25:01 - Labels added: Security

[Christopher Kramer]

Completely agree.

I used 32 as an example as lots of simple salt-generation algorithms use an md5 of
some random input. Of course MD5 hashs are in [0-9A-F]{32}, which means less possibilities
(3,4e38) than alphanumeric [0-9A-Za-z]{22}, which makes 2,7e39. So the md5-approach
wastes 10 characters of salt length.
And of course using other characters like special chars is a very good idea for salts
and would easily make a 20 chars salt as good as a 32 char md5-salt (or even better).

So I'd say 20 is a good length given the fact that we are not securing fort knox here.

Reported by crazy4chrissi on 2013-02-10 15:51:13

[dreadnaut]

Something like this then?

$set = 'A....Za....z0....9';
$setLast = strlen($set) - 1;
$salt = '';
for ($saltSize = 20; $saltSize > 0; $saltSize--)
  $salt .= $set[mt_rand(0, $setLast)];

Reported by dreadnaut on 2013-02-11 13:12:16 - Status changed: Accepted

[Christopher Kramer]

Yeah, this should do fine.

Reported by crazy4chrissi on 2013-02-15 14:37:06

[dreadnaut]

Patch against r338 attached.

Reported by dreadnaut on 2013-02-15 23:16:28

<hr> * Attachment: newsalt-r338.diff

[Christopher Kramer]

Looks good. Especially that you created a new static method that takes saltSize as parameter.
If we continue like that, phpLiteAdmin might become a well oo-designed software one
day ;-)

Please commit it.

Reported by crazy4chrissi on 2013-02-16 11:25:03 - Status changed: Started

[dreadnaut]

This issue was closed by revision r340.

Reported by dreadnaut on 2013-02-16 11:49:20 - Status changed: Fixed

[dreadnaut]

One day :)

Committed as r340!

Reported by dreadnaut on 2013-02-16 11:49:58