Will this code make some attacks (XSS, CSRF...) harder to do?

Question

I have following code on my login page (now i use it for preventing POST variable resubmit on page refresh):

<?php

$cleaner = md5(uniqid(rand(), TRUE));
$_SESSION["cleaner"]=(empty($_SESSION["cleaner"])) ? $cleaner : $_SESSION["cleaner"];

if($_POST['login_button'] && !empty($_POST['cleaner']) && $_SESSION['cleaner'] == $_POST['cleaner']){

    $cleaner = md5(uniqid(rand(), TRUE));
    $_SESSION['cleaner'] = $cleaner;

     //all other verification/login code

}
?>

<form method="POST" action="">
  <input type="hidden" name="cleaner" value="<?php echo $_SESSION['cleaner']; ?>" /> 
  <--! all other login html code and fields -->
</form>

Can this code make some attacks harder to do, or if not - what to change?

Answer 1

I haven't had the need to worry about this kind of thing before, so I can't claim that this will answer your question, but maybe it will help. It looks simple enough, but rand() may not be the best tool here. I know that, at least with Java, the random function is easily beaten because it starts with a number from a list and then iterates over that list each time you call it, and the list is always the same. So maybe this is something to look at in PHP too. Also, I was here anyways and I couldn't help but notice a few things.

A couple of minor notes first. One line of code should only do one thing. This helps with legibility, which is a real problem the deeper you nest functions. And what is "cleaner" by the way? This is a really odd name. Would expect "id" or something else, but anyways.

$id = uniqid( rand(), TRUE );
$cleaner = md5( $id );

You should check if the variable is set, not empty. Unless you already know it is set, which you don't because it hasn't been done in this document. Also, the extra parenthesis are unnecessary in this ternary operation.

$_SESSION[ 'cleaner' ] = isset( $_SESSION[ 'cleaner' ] ) ? $cleaner : $_SESSION[ 'cleaner' ];

A more efficient way, instead of creating something you will now never use, is to only create it if you have to. So...

if( isset( $_SESSION[ 'cleaner' ] ) ) {
    $cleaner = $_SESSION[ 'cleaner' ];
} else {
    $id = uniqid( rand(), TRUE );
    $cleaner = md5( $id );
}

$_SESSION[ 'cleaner' ] = $cleaner;

You may notice that I'm now modifying the session variable no matter what. Its not really necessary, but it keeps the session fresh which is sometimes nice.

With this next line I'm assuming that you are checking if the form has been submitted ("login_button" seems kind of obvious). This is a really debatable subject. Many people still do this, I think because they think it ensures that the user is actually using their form instead of their own, but its not that hard to look at the source and create POST parameters with the same keys, so I don't know how much truth there is in that. I personally find it better to just check the request method, or, if I'm feeling particularly lazy, to see if the POST array is empty. As I said, its really debatable, so you can take it or leave it.

if( $_SERVER[ 'REQUEST_METHOD' ] == 'POST' ) {
//Or the hack
if( $_POST ) {

Next you check for the POST version of "cleaner", this should be another isset() check rather than an empty() check by the way, then you compare the session version to the POST version. So then, this would only be possible, theoretically, if we didn't use the cleaner we created earlier because the session version was set. Think I'm with you so far... But why not then use that earlier version we didn't use instead of creating a new one? To illustrate, a full rewrite might look something like this:

$id = uniqid( rand(), TRUE );
$cleaner = md5( $id );

if( isset( $_SESSION[ 'cleaner' ] ) ) {
    $temp = $_SESSION[ 'cleaner' ];
    if( ! $_SERVER[ 'REQUEST_METHOD' ] || $temp != $_POST[ 'cleaner' ] ) {
        $cleaner = $temp;
    }
}

$_SESSION[ 'cleaner' ] = $cleaner;

Hopefully this helps somewhat, sorry I couldn't really answer your question.