PHP Session issue with $Password on input

[edombrowski]

I am having a very strange issue on one server.  I have the same code in a development server running fine, but in my prod server it is failing.  Here is the main issue:

I have a user authentication routine that accepts UserID and Password from a form and validates it against a MySQL database. So to start, UserId and Password are entered via POST variables as is standard:

$UserId=@$_POST['UserId'];
$Password=@$_POST['Password'];


The Password is encrypted using a standard crypt method such as:


$encrypt = crypt($Password,'6!68$7435!');


And this is stored in a MySQL database.  This part is working fine, that is, the password is encrypted in value and stored in the MySQL database as 'epasswd'.  On login, I am using session, so a standard session_start() and eventual session_destroy() on logout are used.  The reason I mention this is because I suspect my issue is session related.

So normally this works well.  User logs in and I check credentials as follows in one part of my auth routine:


elseif(UserAuth($UserId,$Password)){
    $UserLogin=$UserId; session_start();
    $_SESSION['UserLogin'] = $UserLogin;
    sql_insertActivity();
    header("Location: home.php");


And the auth routine is as follows:


<?

function UserAuth($UserId,$Password){
global $conn;
$Stmt="select epasswd from Users where UserId='$UserId' and Approved='1' or Approved='-1' or Approved='-2'";
$Result = mysql_query($Stmt, $conn) or die(mysql_error());
$Result=mysql_fetch_row($Result);
$epasswd=$Result[0];
$retval=($epasswd==crypt($Password,$epasswd));
return($retval);
}
?>


So I am checking for a valid UserID and Password on form input, and I have a few other variables set for approved status.  The retval checks the password they enter versus the encrypted value for a match.  This usually works well.  Then login occurs and session started, etc.

Here is the issue.  I added a quick admin routine a little while ago which helps reset a user's password to a temporary value.  Once this value is set, along with a setting of approved=-1 in my database, then the user is re-directed to a Change Password screen to update his or her password. *Note:  I changed the value to 'Charlie' for this discussion purpose.  

 

Here is that quick admin routine I run when I need to change a User to a temp setting:


// ----- Establish database connection -----
 require "../inc_php/inc_mysql_prod.php";

//

$UserId=@$_GET['UserId'];
$Password='Charlie';
$encrypt = crypt($Password,'6!68$7435!');
$sql = "UPDATE Users set epasswd='$encrypt', approved='-1' where UserId='$UserId'";
mysql_query($sql, $conn) or die(mysql_error());


So this does work as I validate the UserID is updated in the MySQL database along with an encrypted value for 'Charlie'.  

However, this is where things breakdown going forward.  When the user logs in with the temp credentials, and enters in the Change password routine, their new password is saved in the table.  However, when logging back in with the new credentials, the new password is not valid.  And what's odd is that 'Charlie', the temp password, works for them on login and nothing else, no matter how many times they change the password in the form.  

So seems a case of session management out of control?  What is the issue?  I am defining session on all Php pages used, and have a logout to destroy session, etc.  The temp password routine is something I run as an admin in the system and it doesn't have a session start statement.  And I am not defining any global vars for Password.  I lloked into session management and tried some UNSET paths and such, but may not be doing this correctly.  Also I did a complete stop apache, remove all php sess_ files, restart and to no avail.  I tried the clear my client side cookies deal in the browser, and still the same problem.  

What is odd is that this same set of code works fine on my other server, but breaks down on the mirrored server.  They are essentially twins in all setup.  Some minor differences between the two servers regarding PHP setup that might(?) make a difference.  

DEV server:

SERVER_SOFTWARE Apache/2.2.3 (Red Hat)


PROD server: (server showing the issues):

SERVER_SOFTWARE Apache/2.2.3 (CentOS)

HTTP_COOKIE PHPSESSID=3gocr0hhelvsjjlt63pp4qlnp3

_REQUEST["PHPSESSID"] 3gocr0hhelvsjjlt63pp4qlnp3 _COOKIE["PHPSESSID"] 3gocr0hhelvsjjlt63pp4qlnp3

_SERVER["HTTP_COOKIE"] PHPSESSID=3gocr0hhelvsjjlt63pp4qlnp3

Thanks appreciate the help!

-Eddie

 

[Jacques1]

With all due respect: What on earth are you doing there?

 

You keep talking about “standard methods”, but this is one of the oddest hashing attempts I've seen in a while:

$encrypt = crypt($Password,'6!68$7435!');

What is this strange string supposed to do? Do you realize that the second function argument is for the hash parameters and must follow a very specific format? And where do you generate the cryptographic salt? I don't see that anywhere in your code.

 

When you just enter some gibberish on your keyboard, the following happens:

• You get the hopelessly obsolete DES-based hash algorithm from the early 80s.
• There's no salt, which means the hashes are completely unprotected against brute-force attacks and precaculated tables.
• All passwords are truncated to 8 characters.
• An attacker who obtains the hashes will be very, very happy, because they can try around 100 million passwords per second on stock hardware.

Since you also allow the whole world to fetch arbitrary data through SQL injections, you might as well publish all user passwords on your website.

 

The rest of the code doesn't look any better. My suggestion is this: Throw away the code, learn PHP and start over. I know that this sounds brutal, and it's probably not what you wanted to hear. But it's the only valid advice. If I told you that you just need to fix some small detail, I'd be lying.

 

The mysql_* functions are obsolete since more than 10 years, and you don't even use them correctly (never heard of escaping?). Nowadays, we access the database with the PDO or MySQLi extension. To prevent SQL injection attacks, we use prepared statements. Passwords are hashed with a strong algorithm like bcrypt. This DES stuff may have been good enough in the 80s when people cracked passwords on their C64 or something, but a lot has changed since. An attacker today can get a massive amount of computing power for just a few dollars.

[mac_gyver]

one apparent reason for your program/query LOGIC to produce incorrect results is because your sql query that matches the row in the users table contains a logic error - 

select epasswd from Users where UserId='$UserId' and Approved='1' or Approved='-1' or Approved='-2'

this query will match rows where the user UserId is correct and Approved='1', but it will also match any row with Approved='-1' and it will also match any row with Approved='-2'.

 

the correct query logic to match the Userid and any one of the three Approved values would be - 

select epasswd from Users where UserId='$UserId' and (Approved='1' or Approved='-1' or Approved='-2')

or, more simply - 

select epasswd from Users where UserId='$UserId' and Approved IN('1','-1','-2')

also, if your columns are numerical data types, don't put single-quotes around the values in the query.

[edombrowski]

one apparent reason for your program/query LOGIC to produce incorrect results is because your sql query that matches the row in the users table contains a logic error - 

select epasswd from Users where UserId='$UserId' and Approved='1' or Approved='-1' or Approved='-2'

this query will match rows where the user UserId is correct and Approved='1', but it will also match any row with Approved='-1' and it will also match any row with Approved='-2'.

 

the correct query logic to match the Userid and any one of the three Approved values would be - 

select epasswd from Users where UserId='$UserId' and (Approved='1' or Approved='-1' or Approved='-2')

or, more simply - 

select epasswd from Users where UserId='$UserId' and Approved IN('1','-1','-2')

also, if your columns are numerical data types, don't put single-quotes around the values in the query.

Much thanks for the query advice - this will be helpful overall.  However in this case I am seeing session management issues due to the handling of the variable $Password because $_Session and $_Server settings are not being properly unset.  If you have any advice on UNSET parameters I should use to dump session that is causing a conflict with the $Password variable per user Login, please let me know.  Also the fact that HTTP Cookie management was set is something I have to deal with as well.  

[edombrowski]

With all due respect: What on earth are you doing there?

 

You keep talking about “standard methods”, but this is one of the oddest hashing attempts I've seen in a while:

$encrypt = crypt($Password,'6!68$7435!');

What is this strange string supposed to do? Do you realize that the second function argument is for the hash parameters and must follow a very specific format? And where do you generate the cryptographic salt? I don't see that anywhere in your code.

 

When you just enter some gibberish on your keyboard, the following happens:

• You get the hopelessly obsolete DES-based hash algorithm from the early 80s.
• There's no salt, which means the hashes are completely unprotected against brute-force attacks and precaculated tables.
• All passwords are truncated to 8 characters.
• An attacker who obtains the hashes will be very, very happy, because they can try around 100 million passwords per second on stock hardware.

Since you also allow the whole world to fetch arbitrary data through SQL injections, you might as well publish all user passwords on your website.

 

The rest of the code doesn't look any better. My suggestion is this: Throw away the code, learn PHP and start over. I know that this sounds brutal, and it's probably not what you wanted to hear. But it's the only valid advice. If I told you that you just need to fix some small detail, I'd be lying.

 

The mysql_* functions are obsolete since more than 10 years, and you don't even use them correctly (never heard of escaping?). Nowadays, we access the database with the PDO or MySQLi extension. To prevent SQL injection attacks, we use prepared statements. Passwords are hashed with a strong algorithm like bcrypt. This DES stuff may have been good enough in the 80s when people cracked passwords on their C64 or something, but a lot has changed since. An attacker today can get a massive amount of computing power for just a few dollars.

So quite aware of the need for PDO or MYSQLi for the queries, but the mysql is not the issue here.  I should have started the thread with "I know the mysql has to change, but not the issue here".  As to the DES encryption versus other crypt methods, very good point and thanks for the input there.  I can say that the crypt works fine and is not defaulting to 8 characters only as you say.  Perhaps I am not showing all statements there in regards to that method, but that was not my focus once again.  Quite honestly you are missing the main issue here.  I should have focused my posted topic as related to the use of $_Session and $_Server parameters without proper UNSET management.  With more investigation it appears I have some issues with the use of variables without proper session termination.  I am looking for some solid UNSET advice and ways to test for hanging values in session.  The issues on this server started when I introduced a quick routine to test a $Password change, the 'Charlie' statement.  This is when the problem started.  Introducing that quick routine was not advisable and I am now trying to get back to a level set in that regard.  I will be happy to address the other coding standards if I can fix this issue.  Thanks for the advice.    

[Jacques1]

It's fascinating to what extend PHP programmers manage to miss the point.

 

I just told you that your whole user management code is so broken that you might as well have no user accounts at all. And what's your concern? Unsetting a bunch of session variables. Yes, that's what's important. When your house is on fire, you first fix the scratch in your fence. And maybe later you take care of that tiny problem with the fire.

 

I have an easy solution: Get rid of this half-assed session stuff and make the site open for everybody. I'm not joking. Pseudo-security is worse than no security, because it deceives the users and makes them think they can safely hand out their data when they can't. This is particularly harmful in the case of passwords, because people commonly reuse them on different sites.

 

So if you're unable or unwilling to provide actual security, get rid of it altogether. Don't make promises you cannot keep. It will save you a lot of time and trouble.

Edited September 22, 2014 by Jacques1

[ginerjm]

Do I understand that you are trying to remove a password from a session variable, as in $_SESSION['pswd']  ?

 

1 - Why in the world would you store a password anywhere???

2 - Both the $_SESSION and $_SERVER arrays are spelled as I just did - all uppercase.  That could be the root of your problem.

Edited September 22, 2014 by ginerjm

[edombrowski]

It's fascinating to what extend PHP programmers manage to miss the point.

 

I just told you that your whole user management code is so broken that you might as well have no user accounts at all. And what's your concern? Unsetting a bunch of session variables. Yes, that's what's important. When your house is on fire, you first fix the scratch in your fence. And maybe later you take care of that tiny problem with the fire.

 

I have an easy solution: Get rid of this half-assed session stuff and make the site open for everybody. I'm not joking. Pseudo-security is worse than no security, because it deceives the users and makes them think they can safely hand out their data when they can't. This is particularly harmful in the case of passwords, because people commonly reuse them on different sites.

 

So if you're unable or unwilling to provide actual security, get rid of it altogether. Don't make promises you cannot keep. It will save you a lot of time and trouble.

So turns out the DES and crypt security is working just fine.  The SALT is the value I have above and I am storing encrypted values in the database just fine.  Mac_qyver got it right.  It was the actual SQL query that was causing an issue with the 3 options I included, also part of other security settings.  The bad SQL was not failing entirely, but enough to compare the wrong Return values in the function.  Fixing the SQL fixes the issue.  So again, look up how that SALT value in my statement is being used.  It is encrypting values and storing them in the database.  Also, I am using other security for the password values, i.e. requesting so many alpha numberic, special characters, upper and lower case, etc.  It was not included above.  This was a subtle SQL bug that caused me to go downt he session path, and that was wrong as well. 

[edombrowski]

one apparent reason for your program/query LOGIC to produce incorrect results is because your sql query that matches the row in the users table contains a logic error - 

select epasswd from Users where UserId='$UserId' and Approved='1' or Approved='-1' or Approved='-2'

this query will match rows where the user UserId is correct and Approved='1', but it will also match any row with Approved='-1' and it will also match any row with Approved='-2'.

 

the correct query logic to match the Userid and any one of the three Approved values would be - 

select epasswd from Users where UserId='$UserId' and (Approved='1' or Approved='-1' or Approved='-2')

or, more simply - 

select epasswd from Users where UserId='$UserId' and Approved IN('1','-1','-2')

also, if your columns are numerical data types, don't put single-quotes around the values in the query.

So my apologies Mac_qyver and much thanks for the intelligence here and finding the true issue.  The query was actually not failing but returning the wrong or last value of saved password and not the current saved value.  Very odd indeed, but changing the SQL to a correct format and doing some echo tests I saw that I was matching clearly inputed password with the correctly saved encrypted password.  What threw me off as well was that my dev/test server had the old query working just fine!  That messed me up as well, so I defaulted to looking into session vars and UNSET because I suspected I may have a session variable being used improperly.  Turns out not the case.  Thanks again, worked like a charm.

[edombrowski]

Do I understand that you are trying to remove a password from a session variable, as in $_SESSION['pswd']  ?

 

1 - Why in the world would you store a password anywhere???

2 - Both the $_SESSION and $_SERVER arrays are spelled as I just did - all uppercase.  That could be the root of your problem.

Actually I did not set that and would never do it.  But good point, and thanks, I also tried UNSETs with $_SESSION and $_SERVER, then started panciking about possible $HTTP COOKIE settings as well.  I confused myself with session when doing a quick temp password routine and thought I might have messed with session vars causing an issue.  The issue as explained above was the SQL format for the security check.  the password was being saved correctly in the database as encrypted and the other settings I had for security checks also in the DB.  My big mistake was a very bad SQL query.  Thanks for the input!

[Jacques1]

So turns out the DES and crypt security is working just fine.

 

And you've determined that how exactly? You saw same random-looking strings in the database, you were able to log-in, and now you're happy?

 

Programming doesn't work like that. Just because you're seeing something on the screen doesn't mean that it's all good now. It's not. In fact, getting the output right is usually the easiest part. Any fool can print a bunch of HTML markup. The hard part is what you cannot see: Using functions correctly, making sure that the application is secure, writing good code. And for that matter, you have all the work ahead of you.

 

Of course you're free to ignore all problems and walk away. Some people need to learn the hard way, some people never learn. But if you do want to learn, I can assure you that your code is not OK and that you should take the chance of getting in touch with people who know how to do it correctly.

[CroNiX]

Some people need to learn the hard way

Like Home Depot, Target, and other huge stores being lax with security? (Or...ignoring them when employees brought it up)