Solved

Is this going to work properly against CSRF - my first attempt with an actual form

Posted on 2016-10-23
17
65 Views
Last Modified: 2016-10-24
I had this question after viewing CSRF session variables.

So, before I try to take the example from the above link and rewrite the ternary code in a way that I understand, I just wanted to try get an actual form going that had some sort of CSRF protection. Before I was just using a form that did nothing except post with the token which isn't what would happen in the real world. So, could an expert please tell me if I have implemented it properly in this rough example.

 $message = "";

if (isset($_POST['update_email'])) {
	if (empty($_POST['user_email']) || false === filter_var($_POST['user_email'], FILTER_VALIDATE_EMAIL)) {
		$message = "<div class='alert alert-danger'>Email invalid.</div>";
	}

	if (!$message) {
		if (isset($_POST['_csrfname']) && isset($_POST['_csrfvalue']) && isset($_SESSION[$_POST['_csrfname']]) && $_SESSION[$_POST['_csrfname']] === $_POST['_csrfvalue'] && $_POST['_csrfvalue'] !== '') {
			$user_email = $_POST['user_email'];
			$safe_email = filter_var($user_email, FILTER_SANITIZE_EMAIL);
			$safe_id = $_SESSION['sessionID'];
			$stmt = $link->prepare("SELECT `user_email` FROM `db_users` WHERE `user_email` = ?");
			$stmt->bind_param("s", $safe_email);
			$stmt->execute();
			$result = $stmt->get_result();
			$numRows = $result->num_rows;
			if ($numRows > 0) {
				$message = "<div class='alert alert-danger'>Sorry, you can't use that email address.</div>";
			}
			else {
				$stmt = $link->prepare("UPDATE `db_users` SET `user_email` = ? WHERE `safe_user_id` = ?");
				$stmt->bind_param("ss", $safe_email, $safe_id);
				$stmt->execute();
				$stmt->close();
				$message = "<div class='alert alert-success'>Email address updated successfully.</div>";
			}
		}
		else {
			header("location:logout.php");
			exit;
		}
	}
}

$name = 'token-' . mt_rand();
$token = bin2hex(random_bytes(32));
$_SESSION[$name] = $token;

Open in new window

0
Comment
Question by:Black Sulfur
  • 8
  • 5
  • 2
  • +1
17 Comments
 
LVL 108

Accepted Solution

by:
Ray Paseur earned 425 total points
ID: 41856090
Let me try to answer this question by showing you Chris Shiflett's work on HTML forms from about a decade ago, and this script.  His book on PHP Security is worth owning.  The script below illustrates a few things, to wit:

1. How to create a form token and save it in the PHP session (lines 29-31)
2. How to inject the form token into a hidden HTML input control (line 45)
3. How to use HEREDOC notation to create a web page from a template string (lines 34-52)
4. A function that checks the form token, returning True or False (lines 9-17)
5. A way of making the token available for a single use only (line 14)
6. A way to test for a POST-method request (line 21)
7. A way to verify and visualize the form token checking (line 23-25)

<?php // lame_form_token_client.php
/**
 * A client side script that saves a form token and injects it into the request variables
 */
error_reporting(E_ALL);
session_start();


// FUNCTION TO EVALUATE THE IDENTITY IN THE FORM
function check_form_token()
{
    $sess_token = !empty($_SESSION['form_token']) ? $_SESSION['form_token'] : 'X';
    $post_token = !empty($_POST['form_token'])    ? $_POST['form_token']    : 'Y';
    $_SESSION['form_token'] = NULL;
    if ($sess_token == $post_token) return TRUE;
    return FALSE;
}


// IF THERE IS A POST-REQUEST
if (!empty($_POST))
{
    $status = check_form_token();
    if (!$status) echo "Attack!  Run like hell!";
    if ( $status) echo "Success! Trust this client.";
}


// CREATE RANDOM FORM TOKEN, SAVED IN THE SESSION, INJECTED INTO THE HTML
$token = md5( uniqid() . rand() );
$_SESSION['form_token'] = $token;


$html = <<<EOF
<!DOCTYPE html>
<html dir="ltr" lang="en-US">
<head>
<meta charset="utf-8" />
<title>A Lame Form Token Example</title>
</head>
<body>

<form name="my_form" method="post">
<input type="submit" value="Verify Token" />
<input type="hidden" name="form_token" value="$token" />
</form>

</body>
</html>
EOF;

echo $html;

Open in new window

0
 
LVL 82

Expert Comment

by:Dave Baldwin
ID: 41856149
In your code, this section
if (empty($_POST['user_email']) || false

Open in new window

will always return 'false' because 'false' is not a result of a comparison but a condition that will always be 'false'.
0
 
LVL 7

Assisted Solution

by:Gauthier
Gauthier earned 75 total points
ID: 41856160
There is a lack of proper Same Origin check, which is the first critical line of defense against csrf attacks:
See:
https://www.owasp.org/index.php/Cross-Site_Request_Forgery_(CSRF)_Prevention_Cheat_Sheet
So you need to:
- Add a check that the connection is https
- Add code to verify the origin and referer domains.


Your code took care of randomizing the name and value.
Consequently a request downstream will never invalidate the current form token.
Using Ray's code, if a user open multiple instance of the same page or the form rely partly on xmlhttpRequest his code will fail with an invalid token.

On the down side, in your code, you also fail to unset the token once it is used, in effect lowering the security to the same level as a never changing token.

That said, both of your code are terribly outdated, those check are better implemented by using php http middleware, google for PSR-7 middleware for more information.
0
 
LVL 108

Expert Comment

by:Ray Paseur
ID: 41856163
Dave: I think there is more to that statement and it actually makes sense to me:
if (empty($_POST['user_email']) || false === filter_var($_POST['user_email'], FILTER_VALIDATE_EMAIL)) {

Open in new window

This is saying if the form field is either empty() or if you get FALSE back from filter_var(), load $message with something.  Later on, the script tests $message for a null string, etc.
0
 
LVL 82

Expert Comment

by:Dave Baldwin
ID: 41856173
Ok.  That's why I always use more parentheses to isolate the blocks so I can read it right.  And I always put the 'test' value on the right because I'm used to doing that.
if ((empty($_POST['user_email'])) || (filter_var($_POST['user_email'], FILTER_VALIDATE_EMAIL) === false)) {

Open in new window

0
 
LVL 108

Expert Comment

by:Ray Paseur
ID: 41856177
@Gauthier: Good points.  I'm trying to lead our Author along one-step-at-a-time.  The OWASP link was part of an earlier Q-n-A dialog.  The check_form_token() function is intended to unset the token, thereby making the token a single-use token.  I'm still writing the other parts!

This describes the middleware:
http://weierophinney.github.io/2015-10-20-PSR-7-and-Middleware/#/
https://mwop.net/blog/2015-01-08-on-http-middleware-and-psr-7.html
0
 
LVL 7

Expert Comment

by:Gauthier
ID: 41856182
@Ray Passeur
$token = md5( uniqid() . rand() )

Open in new window

csrf token are required to be cryptographically secure:
However look at:
http://php.net/manual/en/function.uniqid.php
and
http://php.net/manual/en/function.rand.php
=> fail.


The original code by black Sulfur
$token = bin2hex(random_bytes(32))

Open in new window

is good.
0
 
LVL 108

Expert Comment

by:Ray Paseur
ID: 41856190
I agree random_bytes() is good, as is PHP7, but it's not realistic to expect everyone to be on PHP7 yet.  My guess is that most users will get Fatal error: Call to undefined function random_bytes() ... when they try to use that function.  And I know there is a userland back-port for this thing, but it might be a lot to expect the E-E audience to know what to do with that.  In other questions we discussed coding to the interface, so that when it's possible to get new concrete classes, the changes will not be difficult.
0
How to run any project with ease

Manage projects of all sizes how you want. Great for personal to-do lists, project milestones, team priorities and launch plans.
- Combine task lists, docs, spreadsheets, and chat in one
- View and edit from mobile/offline
- Cut down on emails

 

Author Comment

by:Black Sulfur
ID: 41856210
Man, now I don't know what to say. I am trying to learn to code PHP and I am told my code is outdated and I should be using middleware. I appreciate the comment but it leaves me wondering what I am actually meant to be learning?
0
 
LVL 108

Expert Comment

by:Ray Paseur
ID: 41856221
what I am actually meant to be learning?
It's always a moving target.  If you think you're at risk because you're not using crypto-secure tokens, you can try something like random_bytes() but for right now, getting the design patterns correct would seem a good first step.  You could just use a token that had "xyz" in it for proof of concept.  You wouldn't deploy that, but it would let you get all the forms and request/response parts working.

The kind of design we usually use for something like this is a function call that returns the token.  When we're satisfied that the underlying design of our app is correct, then we can go look critically at the function calls, upgrading them as appropriate.

This is a fragment of an article I'm writing about CSRF tokens.  Hope it's helpful...
<?php // form_token_class.php
/**
 * A helper class for form token processing
 *
 * Method get() returns a form token object
 * Method tidy() removes expired tokens
 * Method check() verifies that a token is valid
 */
error_reporting(E_ALL);


// A CLASS TO DEFINE OUR FORM TOKEN
Class FormToken
{
    const FORM_TOKEN_PREFIX = 'form_token_';
    const FORM_TOKEN_EXPIRY = 300;

    public static function get()
    {
        $obj = new StdClass;
        $obj->time  = time();
        if (function_exists('random_bytes')) // CRYPTO-SECURE
        {
            $obj->name  = static::FORM_TOKEN_PREFIX . bin2hex( random_bytes(32) );
            $obj->token = static::FORM_TOKEN_PREFIX . bin2hex( random_bytes(32) );
        }
        else // FALL-BACK FOR PHP < 7
        {
            $obj->name  = static::FORM_TOKEN_PREFIX . md5( uniqid() . rand() );
            $obj->token = static::FORM_TOKEN_PREFIX . md5( uniqid() . rand() );
        }
        return $obj;
    }

    public static function tidy()
    {
        $timex = time() - static::FORM_TOKEN_EXPIRY;
        $prefix_length = strlen(static::FORM_TOKEN_PREFIX);
        foreach ($_SESSION as $key => $value)
        {
            if (substr($key,0,$prefix_length) == static::FORM_TOKEN_PREFIX)
            {
                if ($token = json_decode($value))
                {
                    if (!empty($token->time) && ($token->time < $timex)) unset($_SESSION[$key]);
                }
            }
        }
    }

    public static function check()
    {
        static::tidy(); // REMOVES EXPIRED TOKENS

        $prefix_length = strlen(static::FORM_TOKEN_PREFIX);
        foreach ($_SESSION as $key => $value)
        {
            if (substr($key,0,$prefix_length) == static::FORM_TOKEN_PREFIX)
            {
                if ($session_token_obj = json_decode($value))
                {
                    if (!empty($_POST[$session_token_obj->name]) && ($_POST[$session_token_obj->name] == $session_token_obj->token))
                    {
                        unset($_SESSION[$key]); // MAKES EACH TOKEN INTO A SINGLE-USE TOKEN
                        return TRUE;
                    }
                }
            }
        }
        return FALSE;
    }
}

Open in new window

0
 

Author Comment

by:Black Sulfur
ID: 41856227
Thanks Ray. Sometimes I just feel like I am taking one step forward and two steps back. I thought there was a "right" way to do things but it seems like there are so many opinions of what is right and what is wrong and what is outdated and what is the new better way to do things etc.

It's just kinda disheartening.

Anyway, getting back to my original post. I thought that it was working because when using the Chrome browser, I right click and choose "inspect". If I go to the actual token hidden form field I see:

b4c152ff6d5871672897e60df5e863700bb8ff3ace885e6726c4a457e2897cd0

If I alter this in anyway and try click the submit button, it does what I wanted it to do and redirects me to the logout page which ends any sessions and destroys all cookies. So, I thought that the CSRF functionality was working. Or is that not the proper way to test it?
0
 

Author Comment

by:Black Sulfur
ID: 41856230
And a point about unsetting the session, why do other published examples not show this if it should be done?

I just looked at Security and Encryption - PHP cookbook, 3rd edition(2014)  and it shows nothing like that. Here it is:

<?php

session_start();

$_SESSION['token'] = md5(uniqid(mt_rand(), true));

?>

<form action="buy.php" method="POST">
<input type="hidden" name="token" value="<?php echo $_SESSION['token']; ?>" />
<p>Stock Symbol: <input type="text" name="symbol" /></p>
<p>Quantity: <input type="text" name="quantity" /></p>
<p><input type="submit" value="Buy Stocks" /></p>
</form>

Open in new window


session_start();

if ((! isset($_SESSION['token'])) ||
    ($_POST['token'] != $_SESSION['token'])) {

    /* Prompt user for password. */
} else {
    /* Continue. */
}

Open in new window


And that's it!
0
 
LVL 108

Expert Comment

by:Ray Paseur
ID: 41856265
thought there was a "right" way to do things
There is - give up on PHP and use Ruby on Rails!  Ruby designs favor convention over configuration, resulting in systems that have only one way to do things.  I'm exaggerating a little bit, but you will find Ruby-like concepts in Laravel, and that's one of the reasons it's so popular.

The main problem with PHP is that it's a 20+ year old language.  When it was "invented" there had never been any hacking.  We never even knew that there were security holes, much less how to close them.

The main problem with trying to learn PHP "all at once" is that you must conflate about 20 years of language evolution into what should be simple design patterns, and you just can't do that.  You have to learn one-step-at-a-time, combining the building blocks of knowledge as you build the application components you need.

No set of examples can cover everything, and many of the "discussions" lose track of the original design strategy.  For an example, look at the comment thread in this article.  The article presented a simple client-authentication design pattern.  The comment thread is all over the place, wandering about in minutiae that, each taken separately, has importance in its own right, but does nothing to focus the intent of the article.  It's like the story of the blind men and the elephant.

I guess that's why it's hard to learn a programming language by asking questions in an online forum -- you will not only get a variety of opinions about what's "best," you can't really evaluate the relevance of the opinions until you have tried all the ideas and seen what worked and what did not!
0
 

Author Comment

by:Black Sulfur
ID: 41856278
There is - give up on PHP and use Ruby on Rails!

Haha! As much as PHP frustrates me because there doesn't seem one right way to do something, I really like it and don't want to give up on it just yet.

Okay, back to business. Could someone please just tell me if my code would work? I have mentioned the Google Chrome test I did but not sure if that is a good enough way of testing it. Also, I would like to please know where I need to put in the line to unset the session (Ray, I looked at your segment of your upcoming article and it's way beyond me, going into public functions etc. I am not even close to getting to that yet!)

I can close the question out if someone could let me know.

Thanks!
0
 
LVL 108

Expert Comment

by:Ray Paseur
ID: 41856327
I think you're OK, if I understand things correctly.  This is what I understand:  Your script generated a form token and it worked correctly, allowing you to make new POST requests as long as it was intact.  But when you altered or deleted the form token, you could no longer make new POST requests because the script needed the token to be intact in order to allow the requests to proceed.

There are still a lot of ifs and buts, however if this is what you've demonstrated, you're on the right path.
0
 

Author Comment

by:Black Sulfur
ID: 41856570
@ Ray,

you worded that perfectly. And as far as I can tell, that is exactly what is happening.
0
 
LVL 108

Expert Comment

by:Ray Paseur
ID: 41857691
I tried to pull together some of the theory, practice and links in this article:
https://www.experts-exchange.com/articles/28802/Improved-Form-Tokens-to-Guard-Against-CSRF-and-Screen-Scrapers.html
1

Featured Post

IT, Stop Being Called Into Every Meeting

Highfive is so simple that setting up every meeting room takes just minutes and every employee will be able to start or join a call from any room with ease. Never be called into a meeting just to get it started again. This is how video conferencing should work!

Join & Write a Comment

Suggested Solutions

Popularity Can Be Measured Sometimes we deal with questions of popularity, and we need a way to collect opinions from our clients.  This article shows a simple teaching example of how we might elect a favorite color by letting our clients vote for …
Author Note: Since this E-E article was originally written, years ago, formal testing has come into common use in the world of PHP.  PHPUnit (http://en.wikipedia.org/wiki/PHPUnit) and similar technologies have enjoyed wide adoption, making it possib…
Explain concepts important to validation of email addresses with regular expressions. Applies to most languages/tools that uses regular expressions. Consider email address RFCs: Look at HTML5 form input element (with type=email) regex pattern: T…
The viewer will learn how to count occurrences of each item in an array.

705 members asked questions and received personalized solutions in the past 7 days.

Join the community of 500,000 technology professionals and ask your questions.

Join & Ask a Question

Need Help in Real-Time?

Connect with top rated Experts

18 Experts available now in Live!

Get 1:1 Help Now