3

I have this function for CSRF protection, it is pretty insane.

function GenToken($ranLen) {
    $characters = '0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ!@#$()';
    $randomString = '';
    for ($i = 0; $i < $ranLen; $i++) {
        $randomString .= $characters[rand(0, strlen($characters) - 1)];
    }
    return $randomString;
}

It is called up by this:

$token = GenToken(rand(32,128));

It uses PHP's rand() which I know is far from ideal when it comes to creating random numbers.

What I am wondering is just how bad is it? Is this function suitable for 'good' (granted wacky) CSRF protection? It sure as hell generates one heck of a string.

Currently the function is only used for CSRF however it could be used for other short random strings like a code emailed to the user to activate their account ect. Is this acceptable?

TraceRace
  • 35
  • 1
  • 4
  • It'll be fine for your purposes it'll be more than random enough that it can't be "predicted". – Dave Jan 15 '14 at 11:15
  • Regarding the activation via email, it's probably ok as long as you don't allow the evil-minded people to try a zillion of activation requests with different codes (it also means that your activation secret should not be too short). – Ashalynd Jan 15 '14 at 11:15
  • Maybe relevant to your question: http://stackoverflow.com/a/7808258/1961059 – Maximilian Riegler Jan 15 '14 at 11:17
  • Thanks, it seems that perhaps making the simple change from `rand()` to `mt_rand()` would be better than nothing. – TraceRace Jan 15 '14 at 11:29
  • @TraceRace - use `openssl_random_pseudo_bytes` and it will be flawless. – Vilx- Jan 15 '14 at 11:53

2 Answers2

1

It'll probably be good enough, as long as the generated tokens are user-specific and/or are expired relatively soon. However, if you're going to change it at all, you should change it to use a decent PRNG, which is available on most systems in the form of /dev/random and can be accessed using a number of ways:

mcrypt_create_iv($raw_salt_len, MCRYPT_DEV_URANDOM)
openssl_random_pseudo_bytes($raw_salt_len)
fopen('/dev/urandom', 'r')  // then fread enough bytes from it

Simply bin2hex or base64_encode the return values of the above. Your rand (or better mt_rand) solution should only be a fallback in case none of the above are available.

deceze
  • 510,633
  • 85
  • 743
  • 889
  • Thanks. How is this method in terms of speed and scalability? Will I run into problems if I am constantly generating random numbers on an already loaded server? – TraceRace Jan 15 '14 at 12:41
  • Been experimenting with this `return substr(base64_encode(openssl_random_pseudo_bytes($len)), 0, $len);` where `$len` is the length of characters the token will be (ie, 32-128). It seems promising, can you see anything wrong with such a solution? – TraceRace Jan 15 '14 at 15:27
  • I don't think speed should be any problem. You *may* be exhausting the entropy pool of `/dev/random` if you have a ton of requests to serve, but in practice this is no problem and you'll still get random numbers which are not worse than `rand`. If you `bin2hex` instead of `base64_encode` you can simply read `$len/2` bytes to get the desired length without `substr` (bytes encoded to hex simply double in length). – deceze Jan 15 '14 at 16:09
0

For the similar task I'd prefer to use built-in php function called uniqid http://www.php.net/manual/en/function.uniqid.php It should be faster and safer then your implementation

kryoz
  • 87
  • 4
  • The PHP docs say `...without being passed any additional parameters the return value is little different from microtime().` Can you please explain how this is a safer implementation? I imagine you use the `more_entropy` flag? – TraceRace Jan 15 '14 at 13:09
  • Alright may be it will be equally safe but anyway faster. For a really more secure random sequence I'd use `openssl_random_pseudo_bytes` but not for your case, that's overkill – kryoz Jan 15 '14 at 14:39