Login not working - Information not kept across pages?

sikuneh

New Member
Messages
55
Reaction score
0
Points
0
On my login page when a user tries to login it takes them to the next page but nothing shows up and on the source code it shows I have some errors that shouldn't be there. It seems it loses the information when it goes to validate the information.

login page
HTML:
<form action="includes/users/validateLogin.php" method="POST">
	<table border="0">
    	<tr>
        	<td id="user">Username:</td>
            <td><input type="text" name="username" size="10" /></td>
        </tr>
        <tr>
        	<td id="pass">Password:</td>
            <td><input type="password" name="password" size="10" /></td>
        </tr>
        <tr>
        	<td colspan="2"><input type="submit" value="Login" /></td>
        </tr>
	</table>
</form>

Validation
PHP:
$vars = array(
'name'=>$_POST['username'],
'pass'=>$_POST['password']);

$errors = array();

foreach($vars as $values){
	if(empty($values)) {
		$errors[]="Invalid Value";
	}
}

$checkForm = $dbh->prepare("SELECT username,password FROM users WHERE username = :name && password = :pass");
	$checkForm->bindValue(":name",
		$_POST['username']);
	$checkForm->bindValue(":pass",
		md5($_POST['password']));
$checkForm->execute();

if($checkForm->rowcount()===0) {$errors[]="Invalid Username or Password";}

if($errors) {userError($errors);}

If you need a live link Here.

test login
Username: test
Password: test
 

bunny.invasion76

New Member
Messages
12
Reaction score
0
Points
0
I'm testing...

You don't use session_start() ...
If you want an example you can ask me ... :p

---------- Post added at 12:26 PM ---------- Previous post was at 12:20 PM ----------

Try this maybe...
PHP:
<?php
session_start();


$vars = array( 
'name'=>$_POST['username'], 
'pass'=>$_POST['password']); 

$errors = array(); 

foreach($vars as $values){ 
    if(empty($values)) { 
        $errors[]="Invalid Value"; 
    } 
} 

$checkForm = $dbh->prepare("SELECT username,password FROM users WHERE username = " . $vars['name'] ." AND password =  " . $vars['pass'] .""); 
    $checkForm->bindValue("" . $vars['name'] ."", 
        $_POST['username']); 
    $checkForm->bindValue("" . $vars['pass'] ."", 
        md5($_POST['password'])); 
$checkForm->execute(); 

if($checkForm->rowcount()===0) {$errors[]="Invalid Username or Password";} 

if($errors) {userError($errors);}  

?>

PS: I just edited one thing but I can give you also a full edited ... I use another way to log in ...
 
Last edited:

misson

Community Paragon
Community Support
Messages
2,572
Reaction score
72
Points
48
On my login page when a user tries to login it takes them to the next page but nothing shows up and on the source code it shows I have some errors that shouldn't be there. It seems it loses the information when it goes to validate the information.
The test authentication page generates no content on successful login, which is why you see none. The error messages show up in the source view because the page is re-retrieved with a GET. Use your browser's DOM inspector to view the live page structure rather than view source.

In short, the code you posted does what it's supposed to: authenticate a user. However, it has other problems. Don't use MD5 to hash, as it's considered broken by security professionals. Use a newer cryptographic hash function, such as Whirlpool or something in the SHA2 family (SHA1 is also considered broken, though attacks are still more complex than for MD5). You need to salt the hashes, else they're vulnerable to rainbow tables.

login page
HTML:
<form action="includes/users/validateLogin.php" method="POST">
	<table border="0">
        ...
Don't use tables for layout, use styling ([2]).

Good problem write-up, by the way.



Try this maybe...
PHP:
<?php
...
$checkForm = $dbh->prepare("SELECT username,password FROM users WHERE username = " . $vars['name'] ." AND password =  " . $vars['pass'] .""); 
    $checkForm->bindValue("" . $vars['name'] ."", 
        $_POST['username']); 
    $checkForm->bindValue("" . $vars['pass'] ."", 
        md5($_POST['password']));
This would be a big step backwards. The whole point of prepared statements is you don't have to interpolate values directly into a statement. As it is, your rewrite is vulnerable to SQL injection and the calls to PDOStatement::bindValue are superfluous. Also, concatenating an empty string with another string will have no affect. You need to read up on PDO. If you need a tutorial, try "Writing MySQL Scripts with PHP and PDO"
 

mandy0

New Member
Messages
32
Reaction score
0
Points
0
Use session global variable. Set up the session when user is authorised. for example
$_SESSION['logged']=1 If successfully logged in.
and then for LOG IN required pages just do
Code:
if($_SESSION['logged']!=1)
{
header("location: login.php"); //  IF USER IS NOT LOGGED REDIRECT HIM TO LOG IN PAGE
}

and don't forget to use
Code:
<?php
session_start();
?>
It should be first statement before anything else i.e. even <html> should start after above tag. :) all the best ;)

and whenever you take input from user firstly if you are going to use it in a query do this first.

Code:
$_POST['textboxetcname']=mysql_real_escape_string($_POST['textboxetcname']);

and another thing if log in is validated then use redirection to take him to index page or whatever using
header("location:filename.php");

Hope it would help you :)
 

sikuneh

New Member
Messages
55
Reaction score
0
Points
0
That was my bad, I thought I copied everything. I included the session_start() and the header(). However, it still doesn't work, I looked in the DOM inspector and there's nothing there either.
 

misson

Community Paragon
Community Support
Messages
2,572
Reaction score
72
Points
48
However, it still doesn't work, I looked in the DOM inspector and there's nothing there either.
That's because
The test authentication page generates no content on successful login, which is why you see none.

Add a login-successful notice to see that it's working. Here's a minor rewrite with support for salt (you'll also need to alter the database), though many of the tasks (authorization, db↔user mapping, password security requirements) should be refactored into separate classes (password security requirements should be advice rather than a class or class method, but PHP doesn't support aspect oriented programming at the language level).
PHP:
<?php
/***** this section should be broken up into other scripts. *****/
// file to log errors
$logfile='...';
define('ERRLOG_SYS', 0);
define('ERRLOG_EMAIL', 1);
define('ERRLOG_FILE', 3);
define('ERRLOG_SAPI', 4);

define('ASCII_UPPERCASE', 'ABCDEFGHIJKLMNOPQRSTUVWXYZ');
define('ASCII_LOWERCASE', 'abcdefghijklmnopqrstuvwxyz');
define('ASCII_DECIMAL', '0123456789');
define('ASCII_HEXADECIMAL', '0123456789ABCDEFabcdef');
define('ASCII_ALPHABETIC', ASCII_UPPERCASE . ASCII_LOWERCASE);
define('ASCII_ALPHANUMERIC', ASCII_DECIMAL . ASCII_UPPERCASE . ASCII_LOWERCASE);
define('ASCII_PUNCTUATION', ' !"#$%&\'()*+,-./:;<=>?@[\\]^_`{|}');
define('ASCII_PRINTABLE', ASCII_ALPHANUMERIC . ASCII_PUNCTUATION);

function nonce($len=32, $chars=ASCII_PRINTABLE) 
{
    $nonce = '';
    $chlen = strlen($chars);
    while (strlen($nonce) < $len) {
        $r = mt_rand();
        while ($r) {
            $nonce .= $chars[$r % $chlen];
            $r = (int)($r / $chlen);
        }
    }
    return $nonce;
}

/* You can use ensure_password_requirements to update passwords
  when security requirements change. If it's not possible to fulfill 
  the requirements automatically (such as if you have complexity 
  requirements that have changed for passwords), you can have 
  this function output a form or redirect the user to a page to enter 
  a new password.
 */
function ensure_password_requirements($dbh, &$authInfo, $pw) {
    static $brokenHashes = array('md5'=>1, 'sha1'=>1);
    if (! $authInfo['salt']) {
        $authInfo['salt'] = nonce();
        $rehash = true;
    }
    if (array_key_exists($authInfo['hasher'], $brokenHashes)) {
        $authInfo['hasher'] = 'sha256';
        $rehash = true;
    }
    if ($rehash) {
        $authInfo['password'] = hash($authInfo['hash'], $authInfo['salt'] . $pw);
        $setAuthInfo = $dbh->prepare(
            "UPDATE users 
             SET password=:password, salt=:salt, hasher=:hasher 
             WHERE id = :id"
        );
        $setAuthInfo->execute($authInfo);  
    }
}

/***** We now return you to your regularly scheduled script, already in progress. *****/
session_start();

$dbh = LocalDB::connect(); // or however you do it.

$fields = array('username' => 'Username', 'password' => 'Password');
$errors = array(); 

foreach($fields as $name => $label){ 
    if(empty($_POST[$name])) { 
        $errors[$name]="$label field is empty."; 
    } 
}

if (! $errors) {
  try {
    $getAuthInfo = $dbh->prepare("SELECT id, password, salt, hasher FROM users WHERE username = ?");  
    $getAuthInfo->execute(array($_POST['username'])); 
    if (($authInfo = $getAuthInfo->fetch(PDO::FETCH_ASSOC))
        && $authInfo['password'] == hash($authInfo['hasher'], $authInfo['salt'] . $_POST['password'])) 
    {
        // prevent session fixation
        session_regenerate_id();
        $_SESSION['uid'] = $authInfo['id'];
        ensure_password_requirements($dbh, $authInfo, $_POST['password']);
        ?>
        <p>Login successful.</p>
        <?php
    } else {
        $errors[]="Invalid Username or Password";
    }
  } catch (PDOException $exc) {
      // log error so admin can look into it later. 
      $entryID=uniqid();
      error_log("Entry $entryID: $exc", ERRLOG_FILE, $logfile);
      $errors[] = "There was an internal error when trying to log you in. Please try again
later. Should you wish to <a href='/contact'>contact us</a> about this, the error was
logged as entry $entryID; please include this ID in any communication.";
  }
}

if($errors) {userError($errors);}

To update the user table:
Code:
ALTER TABLE users ADD salt VARCHAR(32) NOT NULL DEFAULT '';
ALTER TABLE users ADD hasher VARCHAR(16) NOT NULL DEFAULT 'md5';

If the password column is less than 64 characters, you also need to update it:
Code:
ALTER TABLE users MODIFY password CHAR(64) NOT NULL;
 
Last edited:

callumacrae

not alex mac
Community Support
Messages
5,257
Reaction score
97
Points
48
Use session global variable. Set up the session when user is authorised. for example
$_SESSION['logged']=1 If successfully logged in.
and then for LOG IN required pages just do
Code:
if($_SESSION['logged']!=1)
{
header("location: login.php"); //  IF USER IS NOT LOGGED REDIRECT HIM TO LOG IN PAGE
}

Would that be vulnerable to session hacking? They can just change $_SESSION['logged'] to 1 and then they'll be able to access your hidden content. Don't quote me on that though, I don't really know anything about session hacking.

and whenever you take input from user firstly if you are going to use it in a query do this first.

Code:
$_POST['textboxetcname']=mysql_real_escape_string($_POST['textboxetcname']);

nononononononono

Don't use mysql_*. Ever.

Using prepared statements will mean that you don't have to use mysql_real_escape_string, either follow Misson's link about PDO or read my article here.

~Callum
 
Last edited:

atlanis

New Member
Messages
24
Reaction score
0
Points
0
Would that be vulnerable to session hacking? They can just change $_SESSION['logged'] to 1 and then they'll be able to access your hidden content. Don't quote me on that though, I don't really know anything about session hacking.
Yes it would. The way I usually do my session variables is something likes this.

On login:
Code:
$validation_string = hash('sha512', $username . $salt . $password_hash);
$validation_string = $validation_string . 'x32' . tostring(strlen($validation_string . $salt) * intval($user_id));
$_SESSION['validation'] = base64_encode($validation_string);
I, of course, use different formulas on different sites. 'x32' is just a short random string to allow my script to separate the two parts of the validation. The 'x' character does not occur in a hexadecimal hash, so to someone with knowledge that will become apparent rather quickly.

Yes, I know strlen($validation_string . $salt) will always be the same number. However, if you ALWAYS keep $user_id hidden from the user, that is: it is never ever sent from server to client, then the potential hacker has a lot of guessing to do to come up with the right one. And that's AFTER they figure out that after x32 is the length times the user_id. I change the formula for each site and on certain sites change the session formula once every month or two.

To check login:
Code:
$logged_in = false;
$validation_original = base64_decode($_SESSION['validation']);
$v_array = explode('x32', $validation_original);
$user_id = mysql_real_escape_string($v_array[1] / strlen($v_array[0] . $salt)); //Use mysql_real_escape_string just in case someone session hacks in an attempt to SQL inject.
$q = "SELECT username, password FROM users WHERE id = '${user_id}' LIMIT 0,1;";
$result = mysql_query($q);
while($row = mysql_fetch_assoc($result))
{
    if($v_array[0] == hash('sha512', $row['username'] . $salt . $row['password'])
        $logged_in = true;
}

The $logged_in variable represents whatever you use in your script on a single page to determine what information should be shown. I normally use a user access level instead of a boolean, because if the access level doesn't match one of my specified ones, the user gets the boot. I gave 15 minute IP bans a try once, as well. It worked fairly well, but had enough issues that I didn't leave it in any production code.

Just a quick note, this code is not guaranteed to work. It's modified versions of my own code (my own code DOES work), but I haven't actually tested it. It's just here to give you some ideas on how to add extra security to your session-based login checks. Also, you should use prepared strings as per Alex Mac. This is an example and I'm too lazy to write it all here. Alex covered it well enough in his article, theres no reason for me to rehash it.


EDIT: Added note about prepared strings.
 
Last edited:

sikuneh

New Member
Messages
55
Reaction score
0
Points
0
Thanks for the interesting trick to auto-update security, however my problem still stands.

Here are the errors in the source:
Username field is empty
Password field is empty

Here is the [updated] validation script:
PHP:
<?php
/***** this section should be broken up into other scripts. *****/
// file to log errors
$logfile='...';
define('ERRLOG_SYS', 0);
define('ERRLOG_EMAIL', 1);
define('ERRLOG_FILE', 3);
define('ERRLOG_SAPI', 4);

define('ASCII_UPPERCASE', 'ABCDEFGHIJKLMNOPQRSTUVWXYZ');
define('ASCII_LOWERCASE', 'abcdefghijklmnopqrstuvwxyz');
define('ASCII_DECIMAL', '0123456789');
define('ASCII_HEXADECIMAL', '0123456789ABCDEFabcdef');
define('ASCII_ALPHABETIC', ASCII_UPPERCASE . ASCII_LOWERCASE);
define('ASCII_ALPHANUMERIC', ASCII_DECIMAL . ASCII_UPPERCASE . ASCII_LOWERCASE);
define('ASCII_PUNCTUATION', ' !"#$%&\'()*+,-./:;<=>?@[\\]^_`{|}');
define('ASCII_PRINTABLE', ASCII_ALPHANUMERIC . ASCII_PUNCTUATION);

function nonce($len=32, $chars=ASCII_PRINTABLE) 
{
    $nonce = '';
    $chlen = strlen($chars);
    while (strlen($nonce) < $len) {
        $r = mt_rand();
        while ($r) {
            $nonce .= $chars[$r % $chlen];
            $r = (int)($r / $chlen);
        }
    }
    return $nonce;
}

/* You can use ensure_password_requirements to update passwords
  when security requirements change. If it's not possible to fulfill 
  the requirements automatically (such as if you have complexity 
  requirements that have changed for passwords), you can have 
  this function output a form or redirect the user to a page to enter 
  a new password.
 */
function ensure_password_requirements($dbh, &$authInfo, $pw) {
    static $brokenHashes = array('md5'=>1, 'sha1'=>1);
    if (! $authInfo['salt']) {
        $authInfo['salt'] = nonce();
        $rehash = true;
    }
    if (array_key_exists($authInfo['hasher'], $brokenHashes)) {
        $authInfo['hasher'] = 'sha256';
        $rehash = true;
    }
    if ($rehash) {
        $authInfo['password'] = hash($authInfo['hash'], $authInfo['salt'] . $pw);
        $setAuthInfo = $dbh->prepare(
            "UPDATE users 
             SET password=:password, salt=:salt, hasher=:hasher 
             WHERE id = :id"
        );
        $setAuthInfo->execute($authInfo);  
    }
}

/***** We now return you to your regularly scheduled script, already in progress. *****/
session_start();

include("../../scripts/modules.php");

$fields = array('username' => 'username', 'password' => 'password');
$errors = array(); 

foreach($fields as $name => $label){ 
    if(empty($_POST[$name])) { 
        $errors[$name]="$label field is empty."; 
    } 
}

if (! $errors) {
  try {
    $getAuthInfo = $dbh->prepare("SELECT id, password, salt, hasher FROM users WHERE username = ?");  
    $getAuthInfo->execute(array($_POST['username'])); 
    if (($authInfo = $getAuthInfo->fetch(PDO::FETCH_ASSOC))
        && $authInfo['password'] == hash($authInfo['hasher'], $authInfo['salt'] . $_POST['password'])) 
    {
        // prevent session fixation
        session_regenerate_id();
        $_SESSION['loggedin'] = $authInfo['id'];
        ensure_password_requirements($dbh, $authInfo, $_POST['password']);
		$getPerms = $dbh->prepare("SELECT perms FROM users WHERE username = ?");
		$getPerms->execute(array($_POST['username']));
		$perms = $getPerms->fetch();
		$_SESSION['permissions'] = $perms;
        ?>
        <p>Login successful.</p>
        <?php
    } else {
        $errors[]="Invalid Username or Password";
    }
  } catch (PDOException $exc) {$entryID = uniqid(); reportError($exc,"There was an internal error when trying to log you in. Please try again
later. Should you wish to <a href='/contact'>contact us</a> about this, the error was
logged as entry $entryID; please include this ID in any communication.",'../../logs/errors.log','Adduser.php',$entryID);
  }
}

else {userError($errors);}

Here is the modules page (in case you need it):
PHP:
function reportError($error,$message,$logfile,$page,$entryID) {
	$log = "Error: $entryID | Error on page - $page. Error: $error. @ ".date("d/m/y h:i:s")."\n";
	$fp = fopen($logfile,"w");
	fwrite($fp,$log);
	fclose($fp);
	echo $message;
	return false;
}

function userError($errorArray) {
	echo "There were errors:\n";
	echo "<ul>\n<li>";
	echo implode("</li>\n<li>",$errorArray);
	echo "</li>\n</ul>";
}

The link is still valid.
 

misson

Community Paragon
Community Support
Messages
2,572
Reaction score
72
Points
48
There is an error in my original ensure_password_requirements. The line:
PHP:
$authInfo['password'] = hash($authInfo['hash'], $authInfo['salt'] . $pw);
should be:
PHP:
$authInfo['password'] = hash($authInfo['hasher'], $authInfo['salt'] . $pw);
Note the key 'hash' should be 'hasher'.

Other than that, the sample code doesn't produce the error I'm seeing on your sample page:
Parse error: syntax error, unexpected $end in /home/sikuneh/public_html/includes/users/validateLogin.php on line 42

That error could be caused by a missing close curly bracket, or by improper use of short tags (mostly, using them when short tags are disabled, which they aren't on the free servers). Please post the block of code (up to the outermost "{}") that contains line 42 and mark that line with a comment. Better still, use an editor that supports auto-indenting and paren highlighting (e.g. emacs, Notepad++, Eclipse) to figure out where the missing token might be.


Would that be vulnerable to session hacking? They can just change $_SESSION['logged'] to 1 and then they'll be able to access your hidden content.

Yes it would.
As far as I've seen, only the session ID is vulnerable, since it's the one thing that is supplied by (and available to) clients. A hacker can try to steal a user's session ID or pick one at random and send that as their own ID, or try to force a user to use a particular ID. All other session data is only accessible to server processes, which means other local users could potentially read it, but not web users under normal circumstances. Local users shouldn't have read access on a properly secured server, but if they do, session data can be stored in a DB. Web users could only access session data if some script has appropriate vulnerabilities (file read or write, script injection), at which point the only way to prevent attacks is to close those vulnerabilities. Do you know of a way web users can read or write session data, or are you coding defensively against unknown vulnerabilities in the server or web scripts?


I, of course, use different formulas on different sites. 'x32' is just a short random string to allow my script to separate the two parts of the validation. The 'x' character does not occur in a hexadecimal hash, so to someone with knowledge that will become apparent rather quickly.
Since hash functions have fixed length output, you could do away with the separator.
PHP:
//shared
$hashLen = array(
	'sha256' => 64, 'sha224' => 64,
	'sha512' => 128, 'sha384' => 128,
	'whirlpool' => 256,
	'tiger192,4', => 64,
	//...
);

// exception codes
define('EXC_DB_ERROR', 1);

// so that the default hasher can be defined in exactly one place:
define('HASHER_DFLT', 'sha512');


function auth_hash($user, $salt, $pw, $hasher=HASHER_DFLT) {
    return hash($hasher, $user . $salt . $pw);
}

/* creation & parsing of authentication data refactored as functions
 rather than spread across multiple scripts to increase cohesion.
 */
function pack_authorization($username, $user_id, $password_hash, $salt, $hasher=HASHER_DFLT) {
    $auth_hash = auth_hash($username, $salt, $password_hash, $hasher);
    $auth_product = strlen($auth_hash . $salt) * intval($user_id);
    return base64_encode($auth_hash . tostring($auth_product));
}

function unpack_authorization($data, $salt, $hasher=HASHER_DFLT) {
    global $hashLen;
    $validation_original = base64_decode($data);
    /* 
    str_split below will split $validation_original into two pieces as long as:
       digits(user_id)+digits(strlen(auth_hash . salt)) <= hashLen(hasher)
    where digits(x) = floor(log10(x))+1 is the number of digits in x. Even with 
    64 bit integers, log(x, 10) < 21. Moreover, if there's overflow, PHP should
    switch to floating point, in which case the length of the product as a string
    will be limited by the precision, and the str_split should still work fine. Of 
    course, if there's overflow, then the $user_id calculation will be incorrect.
    */
    list($auth_info['auth_hash'], $auth_product)= str_split($validation_original, $hashLen[$hasher]);
    $auth_info['uid'] = $auth_product / strlen($auth_info['auth_hash'] . $salt);
    return $auth_info;
}

function check_authorization($data, $salt, $hasher=HASHER_DFLT) {
    $logged_in = false;
    $auth_info = unpack_authorization($data, $salt, $hasher);
    try {
        $q = $db->prepare("SELECT username, password FROM users WHERE id = ? LIMIT 0,1;");
        $q->execute(array($auth_info['uid']));
        $row = $q->fetch(PDO::FETCH_ASSOC);
        if ($auth_info['auth_hash'] == auth_hash($row['username'], $salt, $row['password'], $hasher)) {
            $logged_in = true;
        }
    } catch (PDOException $exc) {
        throw new RuntimeException("Internal database error.", EXC_DB_ERROR, $exc);
    }
    return $logged_in;
}

PHP:
// on successful login:
$_SESSION['validation'] = pack_authorization($username, $user_id, $password_hash, $salt));

PHP:
// to check login:
try {
    $logged_in = check_authorization($_SESSION['validation'], $salt);
} catch (Exception $exc) {
    switch ($exc->getCode()) {
    case EXC_DB_ERROR:
        // inform user & log error 
        ...
        break;
    default:
        // unknown/generic error
        ...
        break;
    }
}

The salt used in creating the authentication data better not be the same as that used for hashing passwords, as that would imply every password was hashed with the same salt (unless you also use the username as salt for the password hashes).

Still, this relies an awful lot on security through obscurity, which historically hasn't been very secure. If someone has access to session data, they probably have access to your script source.

Moreover, the above doesn't seem terribly useful. If an attacker has read-write access to session data, they can simply copy someone's authorization validation data (it doesn't even need to be the same). If they don't have write access, they won't be able to forge the data, so there's no point in checking that it's valid. Only if an attacker has write but not read access will it stymy them for a bit, in which case obscuring the user ID doesn't accomplish anything. It could be useful in reducing the possibility of an attacker successfully picking a valid random ID. Is that what you use it for?
 
Last edited:

atlanis

New Member
Messages
24
Reaction score
0
Points
0
As far as I've seen, only the session ID is vulnerable, since it's the one thing that is supplied by (and available to) clients. A hacker can try to steal a user's session ID or pick one at random and send that as their own ID, or try to force a user to use a particular ID. All other session data is only accessible to server processes, which means other local users could potentially read it, but not web users under normal circumstances. Local users shouldn't have read access on a properly secured server, but if they do, session data can be stored in a DB. Web users could only access session data if some script has appropriate vulnerabilities (file read or write, script injection), at which point the only way to prevent attacks is to close those vulnerabilities. Do you know of a way web users can read or write session data, or are you coding defensively against unknown vulnerabilities in the server or web scripts?

I'm coding defensively against the exploits in my code that are sure to exist without my knowledge as well as any unknown vulnerabilities. A system like that is simple enough to implement and doesn't slow the login process enough to matter (you'll only notice if you're reading time in microseconds), so I always implement them as a Just In Case precaution. Better safe than sorry, especially with user data at risk. If you just have a boolean value and a vulnerability exists by which that value can be changed, your site could go to hell in a handbasket.

I'm not an expert on security--hardly an initiate--but it wouldn't surprise me in the least if there were a way to indirectly modify session data.
 
Last edited:

callumacrae

not alex mac
Community Support
Messages
5,257
Reaction score
97
Points
48
I asked ##php in freenode (admittedly, not the most knowledgable people in the world), and they all seemed to be in agreement that anything that uses $_SESSION is vulnerable unless you're using https.

~Callum
 

atlanis

New Member
Messages
24
Reaction score
0
Points
0
Since hash functions have fixed length output, you could do away with the separator.
True.
The salt used in creating the authentication data better not be the same as that used for hashing passwords, as that would imply every password was hashed with the same salt (unless you also use the username as salt for the password hashes).
My salts do vary. In my example I see I just used the variable $salt throughout the entire thing. That would imply that I use the same one everywhere, but that isn't the case. (I was lazier than even I realized writing that example code) Generally for the password hash I use username, password, date of registration. For other hashes I have a variety of static salts.
Still, this relies an awful lot on security through obscurity, which historically hasn't been very secure. If someone has access to session data, they probably have access to your script source.
There is indeed a lot of security through obscurity there, but even StO is better than none. That assumption is valid (that if they have access to session data, they probably also have source access), but even then you still have to pick a matching user id and password hash, then follow the formula to generate the validation id and pair it with the password in the session field.
Moreover, the above doesn't seem terribly useful. If an attacker has read-write access to session data, they can simply copy someone's authorization validation data (it doesn't even need to be the same). If they don't have write access, they won't be able to forge the data, so there's no point in checking that it's valid. Only if an attacker has write but not read access will it stymy them for a bit, in which case obscuring the user ID doesn't accomplish anything. It could be useful in reducing the possibility of an attacker successfully picking a valid random ID. Is that what you use it for?

You bring up a point that I hadn't thought of: If an attacker can read/write their session information, they can probably read/write other users' session information. That pokes a big hole in the usefulness of this. This bit of code's primary purpose is to deter people from picking a valid id/password hash combination. Even if it doesn't stop a serious hacker who knows his stuff (that's hard enough to do even when you do know more about computer security than a few hours of googling will tell you), it's enough to prevent most script kiddies from breaking in for fun.

One thing I've been meaning to research is the viability of tying the session_id() to the 'validation' string. I don't know if the session id remains static for each user for the entire login duration (it would seem logical, but logical assumptions pave the way for logical failures). IF it does, then you could hash the session_id() into the string and perform an ID check, making it much more difficult to copy another user's session data. They'd have to find a way to force a session_id($id) command or copy the known values and attempt to rainbow the hash, then rehash using their session id.
 

sikuneh

New Member
Messages
55
Reaction score
0
Points
0
PHP:
<?php
/***** this section should be broken up into other scripts. *****/
// file to log errors
$logfile='...';
define('ERRLOG_SYS', 0);
define('ERRLOG_EMAIL', 1);
define('ERRLOG_FILE', 3);
define('ERRLOG_SAPI', 4);

define('ASCII_UPPERCASE', 'ABCDEFGHIJKLMNOPQRSTUVWXYZ');
define('ASCII_LOWERCASE', 'abcdefghijklmnopqrstuvwxyz');
define('ASCII_DECIMAL', '0123456789');
define('ASCII_HEXADECIMAL', '0123456789ABCDEFabcdef');
define('ASCII_ALPHABETIC', ASCII_UPPERCASE . ASCII_LOWERCASE);
define('ASCII_ALPHANUMERIC', ASCII_DECIMAL . ASCII_UPPERCASE . ASCII_LOWERCASE);
define('ASCII_PUNCTUATION', ' !"#$%&\'()*+,-./:;<=>?@[\\]^_`{|}');
define('ASCII_PRINTABLE', ASCII_ALPHANUMERIC . ASCII_PUNCTUATION);

function nonce($len=32, $chars=ASCII_PRINTABLE) 
{
    $nonce = '';
    $chlen = strlen($chars);
    while (strlen($nonce) < $len) {
        $r = mt_rand();
        while ($r) {
            $nonce .= $chars[$r % $chlen];
            $r = (int)($r / $chlen);
        }
    }
    return $nonce;
}

/* You can use ensure_password_requirements to update passwords
  when security requirements change. If it's not possible to fulfill 
  the requirements automatically (such as if you have complexity 
  requirements that have changed for passwords), you can have 
  this function output a form or redirect the user to a page to enter 
  a new password.
 */
function ensure_password_requirements($dbh, &$authInfo, $pw) {
    static $brokenHashes = array('md5'=>1, 'sha1'=>1);
/* This is line 42 */    if (! $authInfo['salt']) {		## This is line 42 ##
        $authInfo['salt'] = nonce();
        $rehash = true;
    }
    if (array_key_exists($authInfo['hasher'], $brokenHashes)) {
        $authInfo['hasher'] = 'sha256';
        $rehash = true;
    }
    if ($rehash) {
        $authInfo['password'] = hash($authInfo['hasher'], $authInfo['salt'] . $pw); 
        $setAuthInfo = $dbh->prepare(
            "UPDATE users 
             SET password=:password, salt=:salt, hasher=:hasher 
             WHERE id = :id"
        );
        $setAuthInfo->execute($authInfo);  
    }
}

/***** We now return you to your regularly scheduled script, already in progress. *****/
session_start();

include("../../scripts/modules.php");

$fields = array('username' => 'username', 'password' => 'password');
$errors = array(); 

foreach($fields as $name => $label){ 
    if(empty($_POST[$name])) { 
        $errors[$name]="$label field is empty."; 
    } 
}

if (! $errors) {
  try {
    $getAuthInfo = $dbh->prepare("SELECT id, password, salt, hasher FROM users WHERE username = ?");  
    $getAuthInfo->execute(array($_POST['username'])); 
    if ($authInfo = $getAuthInfo->fetch(PDO::FETCH_ASSOC) && $authInfo['password'] == hash($authInfo['hasher'], $authInfo['salt'] . $_POST['password']))
    {
        // prevent session fixation
        session_regenerate_id();
        $_SESSION['loggedin'] = $authInfo['id'];
        ensure_password_requirements($dbh, $authInfo, $_POST['password']);
		$getPerms = $dbh->prepare("SELECT perms FROM users WHERE username = ?");
		$getPerms->execute(array($_POST['username']));
		$perms = $getPerms->fetch();
		$_SESSION['permissions'] = $perms;
        ?>
        <p>Login successful.</p>
        <?php
    } else {
        $errors[]="Invalid Username or Password";
    }
  } catch (PDOException $exc) {$entryID = uniqid(); reportError($exc,"There was an internal error when trying to log you in. Please try again
later. Should you wish to <a href='/contact'>contact us</a> about this, the error was
logged as entry $entryID; please include this ID in any communication.",'../../logs/errors.log','Adduser.php',$entryID);
  }
}

else {userError($errors);}
?>

This is pretty strange to me, considering I usually get "unexpected $end" referring to the last line in the script.
 
Last edited:

callumacrae

not alex mac
Community Support
Messages
5,257
Reaction score
97
Points
48
That usually means that you've missed out a }. I can't actually see your code right now though, I'm on my phone :(

~Callum
 

misson

Community Paragon
Community Support
Messages
2,572
Reaction score
72
Points
48
I asked ##php in freenode (admittedly, not the most knowledgable people in the world), and they all seemed to be in agreement that anything that uses $_SESSION is vulnerable unless you're using https.
The session ID is definitely vulnerable, if that's what they were referring to. Did they also mean session data is vulnerable to network threats? (Note that even with HTTPS, session data is potentially vulnerable to local threats.) If so, I'm very much interested in what the exploit is. That would be quite a serious security threat.

One thing I've been meaning to research is the viability of tying the session_id() to the 'validation' string. I don't know if the session id remains static for each user for the entire login duration (it would seem logical, but logical assumptions pave the way for logical failures).
The session ID is set in session_start, session_id and session_regenerate_id (and a few other spots when handling form based file uploads, but those do the same thing that session_start does, which is get the session ID from a cookie or the query string). If a session ID already exists, only session_id and session_regenerate_id will replace it (the others are create only). A session ID is discarded when the user deletes the session ID cookie or the cookie expires.

On a related note, session data exists for a given minimum period, after which it has a probability of being discarded with each request.

Including validation data (a checksum or otherwise) in the session ID is completely doable and could help quite a bit (though not completely) against blind attacks and fixation (though regenerating the ID upon successful authentication is completely effective against the latter). A very secure scheme would be to create a nonce, sign it and set the session ID as a concatenation of the two. Without the private key (derived from a server-wide key and data specific to the user) used to sign the nonce, one can't generate a valid session ID. This has some similarities to your previous approach (where user IDs are the secret data and multiplication by the combined length of the authentication hash & salt is the signing algorithm), though user IDs aren't the kind of thing to be kept secret, and multiplication doesn't obscure the secret data very well.

Session ID validation is a great task for the session handler, but the session module doesn't currently have the appropriate hooks. A session handler (other than a user session handler) can set its own session ID creation method (see also the PS_CREATE_SID_FUNC macro), so creating validation data is easy enough. With a little more work, the session module can be given hooks to allow a handler to validate session IDs. The trickiest aspect would be that scripters can set their own session IDs, which likely won't follow the handler's scheme. I don't think the session module or handlers could tell the difference in general between an invalid ID and a scripter specified ID, so there would need to be an option to disable or enable ID checking. Feel like developing a patch?

@sikuneh: the posted code didn't generate the "unexpected $end" error for me, though I see the test page now does. Would you archive the source page from the server in (e.g.) a zip file and post it here? There might be an issue with character encoding, or a non-displaying character causing problems. Alternatively, copy the code you posted here, place it in a new file and replace the current script with the new one. That should do away with any character issues.
 
Last edited:

callumacrae

not alex mac
Community Support
Messages
5,257
Reaction score
97
Points
48
I sent them a link to this and asked if it was vulnerable. They said yes (looking for logs now, but they won't be mine - I was on my phone)

~Callum
 

misson

Community Paragon
Community Support
Messages
2,572
Reaction score
72
Points
48
The linked sample is vulnerable to hijacking, whether by theft, guessing the SID or XSS. It's also potentially vulnerable to server local attacks. If the folks in ##php were thinking of any other vulnerabilities, I'm not sure what they'd be.
 
Top