Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I googled around for secure random number generation and random string generation, combining them with some user data.

Is this good or am I totally off-base? I don't know much about cryptography but I do not see many alternatives, other than some bad code that gets copied around a lot with mt_rand and uniqid.

//csrf tokens
    public function csrf_token($regen = false)
    {
        if($regen === true) {
            //we need to give the user a token
            if(isset($_SESSION["__csrf_token"])) {
                unset($_SESSION["__csrf_token"]);
            }
            $max = mt_rand(0, mt_getrandmax());
            $rand_num = floor($max*(hexdec(bin2hex(openssl_random_pseudo_bytes(4)))/0xffffffff));
            $rand_string = "";
            for($i=0; $i < 11; $i++) {
                $x = mt_rand(0, 2);
                switch($x) {
                    case 0: $rand_string.= chr(mt_rand(97,122));break;
                    case 1: $rand_string.= chr(mt_rand(65,90));break;
                    case 2: $rand_string.= chr(mt_rand(48,57));break;
                }
            }

            $_SESSION["__csrf_token"] = hash('whirlpool', $rand_num . $this->username . $rand_string . $this->hash_pw);
            $this->csrf_token = $_SESSION["__csrf_token"];
            return $this->csrf_token;
        }else{
            //the user already has a token
            return $this->csrf_token;
        }
    }

In the long run, i went with the good advice i received here, removing the obfuscation and simplifying the code.

public function csrf_token($regen = false)
{
    if($regen === true) {
        //we need to give the user a token
        if(isset($_SESSION["__csrf_token"])) {
            unset($_SESSION["__csrf_token"]);
        }
        $rand_num = openssl_random_pseudo_bytes(16);
        $build_string = $rand_num . $this->hash_pw . time();
        $_SESSION["__csrf_token"] = hash('whirlpool', str_shuffle($build_string));
        $this->csrf_token = $_SESSION["__csrf_token"];
        return $this->csrf_token;
    }else{
        //the user already has a token
        return $this->csrf_token;
    }
}
share|improve this question
1  
To me, it looks like it's doing too much. I'd just generate 16 random bytes, convert them to hex, and be done with it. –  icktoofay Apr 13 at 1:06
    
thanks icktoofay. you're probably right. i will consider this when refactoring the code. thank you for your time. –  r3wt Apr 13 at 1:34

1 Answer 1

If CSRF stands for Cross Site Request Forgery, then it's hard to imagine why I should help.

In any case, simply doing a cryptographic Whirlpool hash of a user-supplied string with a random seed value should be sufficiently random for most every purpose. The rest is just obfuscation and doesn't add to security.

share|improve this answer
    
I appreciate your reply. i am not the sharpest knife in the drawer and it is hard for me to decide how much is good enough. so what i infer from your post, i should just hash the hash_pw(user supplied string) with a random number(Random seed), or should i hash it with the random string as well. –  r3wt Apr 13 at 1:33
    
    
@syb0rg how is that related? –  r3wt Apr 13 at 6:09
    
@r3wt It was more related to Edward's comment about why he found it hard to help. –  syb0rg Apr 13 at 16:18
1  
i'm lost.. what about my code is malicious? –  r3wt Apr 13 at 20:00

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.