3

I have writen the following script. Everything works in my application, except the validation keeps returning to login. But I have read a lot about my issue, and everything seems right, but of course there should be something wrong otherwise it would work properly.

In my case a user logs in, a token is stored in the database and in a cookie. For the creation of the token I use: 

bin2hex(openssl_random_pseudo_bytes(16));

What I did next is setup a page that first checks if the cookie token and token in the database match. To be sure I first echo them both and both give the same token. I did it like this:

include 'mydatabase.php'; 
$cookie_name = "My_cookiename";
$result = mysql_query("SELECT * FROM users WHERE token='{$_COOKIE[$cookie_name]}'");
while($row = mysql_fetch_array($result)) {

echo $row['token'];
echo $_COOKIE[$cookie_name];

}

Ok so I am sure at this point the cookie token and database token match. Now I want to compare them with an if/else. And here I am going wrong, because I can't get it to work. What I have now is this:

$result = mysql_query("SELECT * FROM users WHERE token='{$_COOKIE[$cookie_name]}'");
while($row = mysql_fetch_array($result)) {
if ($row['token'] != $_COOKIE[$cookie_name]) { 
header('Location:myloginpage.php'); exit(); } else { // MY PAGE CONTENT IF MATCH }

I think there is something wrong with the line:

if ($row['token'] != $_COOKIE[$cookie_name])

Any help would be great, because I am really stuck at this point.

Josh Beam
  • 19,292
  • 3
  • 45
  • 68
Michael
  • 37
  • 6
  • 1
    Are you using setcookie to set the cookie value? – user2182349 Aug 23 '15 at 23:43
  • You shouldn't need that `if` if you get a return the token matched the cookie. Are there any errors being thrown? – chris85 Aug 23 '15 at 23:43
  • 2
    This doesn't make much sense. You're selecting rows from the database where A equals B, then checking the result to see if A still equals B. Surely you want to check if there are just any matches? e.g. mysql_num_rows – rjdown Aug 23 '15 at 23:45
  • Also please read this http://stackoverflow.com/q/60174/1301076 – rjdown Aug 23 '15 at 23:46
  • Yes I use setcookie to set the cookie value. The strange thing is that when I modify it to this `$result = mysql_query("SELECT * FROM users WHERE token='{$_COOKIE[$cookie_name]}'"); while($row = mysql_fetch_array($result)) if ($row['token'] != $_COOKIE[$cookie_name]) { echo "no match"; } else { echo "match"; } ` when the cookie and db match I get the echo correct. If I modify the token in the database then I get no echo at all, just a blank page. With error_reporting all I get no errors at all :( – Michael Aug 23 '15 at 23:55
  • I will read the post. Thanks for now. I have also tried the num_rows, but I had alsmost the same problem. If a row was found it goes OK, if not I got a blank page :( – Michael Aug 24 '15 at 00:00
  • There might be an error in the code you didn't include in that case.. – chris85 Aug 24 '15 at 00:10
  • You should store your tokens hashed using SHA-256 to prevent anyone with read access to the DB from spoofing a session (including an attacker with an SQLi exploit). – SilverlightFox Aug 24 '15 at 13:21

2 Answers2

3

As the comments on your question have said you are checking things needlessly. The mysql query itself does the token checking for you

include 'mydatabase.php'; 
$cookie_name = "My_cookiename";
$result = mysql_query("SELECT * FROM users WHERE token='{$_COOKIE[$cookie_name]}'");
if (mysql_num_rows($results) != 1) {
    header('Location:myloginpage.php');
    exit();
}
// Content for your page goes here, no need for an else because of exit
MarshallOfSound
  • 2,629
  • 1
  • 20
  • 26
  • Thanks, that did the trick. I will read also the posts suggested. Thanks for the help and suggestions! – Michael Aug 24 '15 at 00:07
  • Actually, you should not check `if (mysql_num_rows($results) == 0)` ... you should check `!= 1` because 2 or more results is just as wrong as 0. – Michael - sqlbot Aug 24 '15 at 02:37
  • @Michael you are correct there, I made an assumption about the schema of his database that the `token` column was marked as `UNIQUE`. If this is not the case then checking you did not get multiple results is essential. However it should be noted that `token` should be marked as `UNIQUE` as the preferred solution to that issue – MarshallOfSound Aug 24 '15 at 03:11
  • I agree for certain that it should have a unique constraint in the database, but even with that constraint in place, I would still be sanity checking the result. "should be impossible" != "is impossible" if someone comes along and subsequently, incorrectly, even unintentionally removes the database constraint, or in extremely rare cases of database corruption. I've seen "cannot happen" turn into "did happen" too many times in the hands of developers who naively coded as if the expected result was a foregone conclusion. – Michael - sqlbot Aug 24 '15 at 12:08
  • Fair points, I will adjust code :). In reality though for this type of usage we should be using proper PHP Sessions anyway :) – MarshallOfSound Aug 24 '15 at 13:53
0

I think I just solved it :D I was having the same issue, I took the & sign out of my random token generator. when I surrounded the cookie string with htmlentities() I noticed the & signs were replaced with &amp, because strings usually read & as code. once I removed & from the tokens, it worked. Hope this helps.

SwiftNinjaPro
  • 787
  • 8
  • 17