2

I want to know if this code is strong enough to prevent CSRF attack on PHP Form?

<?php
session_start();
session_regenerate_id(true);

if (isset($_POST['submit'])) {
if (isset($_SESSION['token']) && ($_POST['token'] == $_SESSION['token'])) {
}
}
$token = hash('sha256', uniqid(mt_rand(), true));
$_SESSION['token'] = $token;
?>

//FORM

<form method="POST" action="page.php">
<input type="hidden" name="token" value="<?php echo $token; ?>">
<input type="submit" name="submit">
</form>

Thanks.

virs90
  • 31
  • 4

3 Answers3

2

If the victim has not viewed any forms on your site, he will not yet have a token stored in his session.

If the attacker presents the victim with a form with no token field at all, the POST request made by the victim will pass the CSRF check because $_POST['token'] and $_SESSION['token'] will both be null. (Or both empty strings depending on how PHP initialises unknown variables.)

You must also check that the token exists in the session before checking for equality and abort if either of those tests fail.

Depending on your site, a user not having seen a form may be very likely or it may be an extreme edge case. With checking for the existence of the token first, it doesn't matter how many forms you have on your website, there is no possibility of a CSRF attack.

Apart from that small problem, I can't see any CSRF vulnerability in it. That code looks like it will do the job.

Ladadadada
  • 508
  • 3
  • 15
  • I guess the logics are that only a request with token that is equivalent to the token in the session gets processed. So your remarks are rather obsolete. – Gumbo May 12 '13 at 09:48
  • @Gumbo Why would you *guess* when the code is right there in the question? Or are you guessing that there might be some more code that wasn't included in the question that does what I suggest already? – Ladadadada May 12 '13 at 10:07
  • The code does not say anything about on what condition the form is actually processed. But my guess is that it’s inside the innermost `if` branch. Anything else would be senseless. – Gumbo May 12 '13 at 10:38
  • @Gumbo That's where I assumed it was too. The vulnerability is that if an attacker can find a victim who doesn't have a token in their session (i.e, has not yet viewed a form on the site) then they can cause that victim to send a POST request with no token and this will execute the innermost `if` branch because null == null. – Ladadadada May 12 '13 at 12:25
  • The two `isset` should prevent that. – Gumbo May 12 '13 at 12:26
  • @Gumbo You're right. I don't know how I misread that line when I was looking at the code but I did. The improvement that I suggested already exists. – Ladadadada May 15 '13 at 16:27
1

On the products I support, I'd say "no." Your random number generator is based on rand() which is predictable. Also, it looks like the random number is very short - it needs to be long enough that it cannot be brute forced during the session's validity - nor cany any of the many active sessions' CSRF tokens be cracked.

Check out the OWASP page on CSRF They'll give you good guidance.

Freedom_Ben
  • 11,247
  • 10
  • 69
  • 89
atk
  • 9,244
  • 3
  • 32
  • 32
  • 2
    I don't see any possibility for brute-forcing the token. A new token is generated on every POST request. Getting microsecond accuracy while either using javascript running on the victim's computer or relying on the victim's clicks to trigger the request is already beyond feasibility before we've even looked at predicting the output of `mt_rand()`. – Ladadadada May 12 '13 at 00:33
  • 1
    @Ladadadada In practical terms you're right, but remember that only one attacker has to get it right one time. If this is just making a friend request on Facebook then maybe we don't care. If it's approving a $1M bank transfer though, it's not worth the risk IMO. Secure Random functions are plentiful these days. Might as well use them. – Freedom_Ben May 12 '13 at 03:50
  • @Freedom_Ben Indeed, there's rarely any *harm* in using a better random number generator. The only complaints I've heard are about slowness. Apparently `openssl_pseudo_random_bytes()` on Windows can take upwards of 30 seconds. – Ladadadada May 12 '13 at 08:46
  • @Gumbo: When I looked up the doc of mt_rand(), it said, that by default, it's based on rand(). Reading the post to which you linked, it's still not a cryptographically strong PRNG, but based on time, pid, and some other very weak seeding bits. – atk May 12 '13 at 14:07
  • @Ladadadada: My comment about brute forcing was about the length of the value being hashed. Like I ahd said, I don't know the value range, but if it's small - say 0-255 - then an attacker need only submit 128 times to win, on average. – atk May 12 '13 at 14:10
  • @Ladadadada Good point, 30 seconds is unacceptable for a web app, especially if it's being called for every request. – Freedom_Ben May 12 '13 at 14:38
1

I’d say it suffices for the given purpose.

The values returned by uniqid(mt_rand(), true) should be up to 33 bytes:

  • up to 10 bytes prefix from mt_rand
  • 8 bytes system time in seconds
  • 5 bytes current microseconds
  • 10 bytes from the internal linear congruence generator php_combined_lcg

However, these 33 bytes do not provide 264 bits of entropy but way less:

  • log2(231-1) ≈ 31 bits for the mt_rand prefix
  • system time is known (e.g. Date response header field)
  • microseconds can only have one of 106 values, so log2(106) ≈ 20 bits
  • LCG value is log2(109) ≈ 30

This sums up to almost 81 unknown bits. To brute force this, one would need on average 281/2 ≈ 1.2·1024 guesses that result in a given token when hashed. The data to process would be approximately 8·1013 TB. With a todays computer, you should be able to run this in approximately 5.215·1017 seconds.

This should be sufficient to render an attack impracticable.

Gumbo
  • 643,351
  • 109
  • 780
  • 844