Never seed a RNG with a timestamp!

[colinmollenhour]

Seeding a random number generator with a timestamp allows an attacker with knowledge about when the secret was generated to greatly reduce the amount of work needed to crack the secret. PHP automatically seeds the RNG with a random number, so seeding is not necessary in the first place.

In this case only the microsecond portion of microtime is used and an attacker would probably not know the microtime, so theoretically there are 1 million possible seeds best case. While 1 million is still a big number, it is much smaller than PHP_INT_MAX (2.14 billion).

This should also be fixed in Magento 1 (actually far more important since Magento 1 always uses mt_rand).

[colinmollenhour]

For some perspective, a truly random password consisting of only [A-Za-z0-9] that was only four characters long would be 14x harder to brute force than Magento's getRandomString of any length (assuming length was known, such as the generated encryption keys).

[piotrekkaminski]

@colinmollenhour in 1.14/1.9 the call to mt_srand was removed so it should be better (not perfect, agreed).

[colinmollenhour]

Ahh, I didn't check 1.9. Glad to hear it was removed!

A script to set a new encryption key and re-encrypt all config values and stored credit cards would probably be a good thing to have for all existing stores since the key is only generated once during install..

[lazyguru]

EE 1.x does re-encrypt when you change the key (there is an admin menu to manage the encryption key)

[colinmollenhour]

@lazyguru Hah! Of course!

[piotrekkaminski]

Hopefully no one stores credit cards there. Otherwise - agreed.

From: Colin Mollenhour
Sent: Friday, September 26, 2014 17:17
To: magento/magento2
Reply To: magento/magento2
Cc: Kaminski, Piotr
Subject: Re: [magento2] Never seed a RNG with a timestamp! (#677)

Ahh, I didn't check 1.9. Glad to hear it was removed!

A script to set a new encryption key and re-encrypt all config values and stored credit cards would probably be a good thing to have for all existing stores since the key is only generated once during install..

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/677#issuecomment-57035487.

[verklov]

@colinmollenhour, thank you for your contribution! The team will be working to make sure we have this issue fixed in M2.

[alankent]

I wonder what the danger is of not calling mt_srand() - if someone else called say mt_srand(0) beforehand that would compromise the random number generator code wouldn't it? So a safer solution is not to delete mt_srand(time), but rather replace it with mt_srand(); (no arguments) to force a new random seed. Does that make sense?

[colinmollenhour]

@alankent That makes sense, though I've never seen anyone do that nor can I think of any reason for someone to do that except perhaps intentionally for unit testing. But I agree, that does add a little additional safety in the presence of third-party code.

[alankent]

Maybe I am overly sensitive, but if we cannot guarantee an extension never calls mt_srand(0), then it is potentially more dangerous than using the million different time values.

[colinmollenhour]

In the case of the encryption key it is only generated once during install so this seems extremely unlikely to be affected. Practically, with respect to encryption strength, 1 is not much worse than 1 million...

However, I still do feel very strongly that Magento should release a script to generate new keys and re-encrypt all known encrypted data for installations created before this bug was fixed in M1. I realize this is not the proper place to be voicing this concern, but I don't even know where else to go..

[maksek]

Hi @colinmollenhour, thanks for such catch. Can you please resubmit your PR into develop branch and also review travis builds. Thanks.

[maksek]

Resolution: reject, due inactivity from creator. We are going to review it internally, but we won't accept current PR.