2

within index.php, I have placed this at the top

<?php
session_start();

function generate_secure_token($length = 16)
{
    return bin2hex(openssl_random_pseudo_bytes($length));
}

$_SESSION['csrf_token'] = generate_secure_token();
$token = $_SESSION['csrf_token'];

?>

Within my form I then have a hidden field

<input type="hidden" name="csrf_token" id="csrf_token" value="<?php echo $token; ?>">

Within my Javascript I make an Ajax request

submitHandler: function(form) {
    $.ajax({
        type: "POST",
        url: "php/process.php",
        dataType: "json",
        data: {
            'csrf_token': $("#csrf_token").val()
        }
    }).done(function(response) {
        if (response === 'success') {
            window.location.replace("thanks.php");
        }
    }).fail(function(jqXHR, textStatus) {
        return false;
    });
}

And then finally within process.php I check the CSRF

<?php
session_start();

$errors = array();
$userData = array();

if (!isset($_POST['csrf_token']) ||
    empty($_POST['csrf_token']) ||
    $_POST['csrf_token'] != $_SESSION['csrf_token']) {
    $errors['csrf_token'] = 'Something went wrong';
}

if (!empty($errors)) {
    echo json_encode('failure');
    sendErrorEmail($errors, "Validation", $userData, __LINE__);
} else {
    //Do something
}

I have noticed that I am getting a lot of error emails relating to the CSRF token not being set. Within sendErrorEmail I am sending myself the browser information for those that fail, and I have noticed that 90% of them are IPhone or Android.

Is there anything specific to this code that may not work within smart phones?

Thanks

Funk Forty Niner
  • 74,450
  • 15
  • 68
  • 141
katie hudson
  • 2,765
  • 13
  • 50
  • 93
  • I guess first step to check is whether or not the form input is getting a value. – miken32 Nov 06 '18 at 22:41
  • 98% of submissions come through fine, with the CSRF. It is just that when I added the browser details, I noticed most of them were mobile devices. I was just wondering if there may be any reason for this? – katie hudson Nov 06 '18 at 22:44
  • It looks to me like it should work. – miken32 Nov 06 '18 at 22:46
  • On another note, using `!isset()` and `empty()` together is redundant. Just use `empty()`. – Mike Nov 06 '18 at 22:51
  • *"Is there anything specific to this code that may not work within smart phones?"* - Not with serverside code, I doubt that; I'd take a wild guess where the fault could lie in JS. – Funk Forty Niner Nov 06 '18 at 23:05

1 Answers1

4

You are regenerating the CSRF token on every request to index.php, so if a user opens something up in a new window/tab after visiting the form page, their token won't validate when they try to submit the form, which would explain why the results you are seeing are also inconsistent. I suggest instead making the CSRF token last for the user's entire session:

<?php
session_start();

function generate_secure_token($length = 16)
{
    if (!isset($_SESSION['csrf_token'])) {
        $_SESSION['csrf_token'] = bin2hex(openssl_random_pseudo_bytes($length));
    }
}

generate_secure_token();

The point of using CSRF tokens is to prevent a remote website from causing a currently logged in user from executing unwanted actions on your website. In order to execute the action, a token is required, and this token is only supplied by your web application. If the token is pseudo-randomly generated (as in your case), guessing it is already essentially impossible, so regenerating the token on each request does not add much to the overall security, unless your app has some sort of other vulnerability (e.g. XSS) that could then cause the token to be leaked back to the malicious website.

See also: New CSRF token per request or NOT?

Mike
  • 23,542
  • 14
  • 76
  • 87
  • Can I make the csrf_token just the session token or it's hash ? – Accountant م Nov 06 '18 at 22:56
  • 2
    I think an explanation on why this affects mostly phone users might be because they use the back-button more extensively than desktop users, and thus getting an invalid/old CSRF-token. – Johan Nov 06 '18 at 22:59
  • 2
    @Accountantم, If you mean just taking the CSRF token straight from the session instead of from `$_POST` or `$_GET`, that definitely wouldn't work because the browser will automatically send this information to the server even if the form is being submitted from a remote server. However if the session ID is also submitted in the form (e.g. ``), I can't think of anything inherently wrong in doing that off the top of my head, but generating it randomly is basically 2-3 simple extra lines of code, so you might as well do that instead. – Mike Nov 06 '18 at 23:02
  • @Johan It could be that most people that visit the website are on mobile too. – Mike Nov 06 '18 at 23:03
  • @Mike Thanks, yes I was meaning to generate the csrf_token from the session (e.g ` – Accountant م Nov 06 '18 at 23:11
  • The whole point of CSRF is to provide better security, and stop hackers from submitting forms they shouldn't be able to. If you use the same CSRF token for every form in a session, then a hacker only needs to generate one form and then they can use this token to submit 1000s for forms in an attempt to break into your application. You should be using a different token for each form generated, and if the token includes some identification tied to the form, then this would also improve the security of your application. – muz the axe Jul 02 '20 at 09:21