How can I check PHP Session in a more secure way?

Question

I currently have a PHP Login System which logins by authenticating the Organisation Code the user enters, thus the database queried will be different.

includes.php

<?php

mysql_connect("mysql.example.com", $dbconn, "MySecurePassword");
mysql_select_db($dbconn);

?>

login.php

// $org is the Organisation Code, will be set when user clicks Login

$dbconn = $org;
include "includes.php";

// Omitted the $userid & $pw variables, assume there is no error, and that MySQL Injection is prevented already

    $query = "SELECT * FROM `Login` WHERE `userid`=TRIM('$userid') AND `password`=TRIM('$pw' )";

    $result = mysql_query($query);

    if(mysql_num_rows($result)>0){

        session_start();
        $_SESSION['logged_in'] = $username;
        header("Location: loggedinpage.php");

    }

loggedinpage.php

<?php

session_start(); 

// As there is no fixed database, I've omitted the DB Connection

 define('DS',  TRUE); // used to protect includes
 define('USERNAME', $_SESSION['logged_in']);
 define('SELF',  $_SERVER['PHP_SELF'] );

// Checks if user is logged in
 if (!USERNAME) {
     header("Location: login.php");
}

?>

Security Measure Taken

Passwords are hashed using SHA-512 and not stored in plaintext.
MySQL injection is prevented using mysql_real_escape_string()

I've omitted some code for ease to read, may I know if this way of checking if user is logged in is secure? If not, how can I improve it?

Thanks in advance!

Updated question to reflect the updates in comments

That's pretty much how it's done. You could instead of `SELECT *`, run `SELECT 1`. Then you're not retrieving database info, since you are not using it. Also quit using `mysql_` functions, go for `mysqli_` instead. And don't forget to escape all user input. — Phiter, Feb 04 '16 at 12:37
One major way would be to switch to using parameterised queries — Mark Baker, Feb 04 '16 at 12:38
Is the way of detecting if user is logged in secure? Thanks! — Panda, Feb 04 '16 at 12:41
*"Thanks! I've already prevented MySQL injection."* - @luweiqi what do you mean by that? I see no escaping functions in your posted code. Plus, that method you're using is not safe to use. Do not go live with this. — Funk Forty Niner, Feb 04 '16 at 14:05
I've omitted out a few lines of code, for ease to view the code. I've prevented MySQL injection already, thanks! — Panda, Feb 04 '16 at 14:07
then your question is misleading. How do we know if you're not hashing passwords and not showing that too? — Funk Forty Niner, Feb 04 '16 at 14:07
I'm asking if the 'way of checking if user is logged in is secure'. It's the focus of my question. And I've also stated that I've omitted the variables and to 'assume there is no error'. Which part of my question is unclear? Thanks! — Panda, Feb 04 '16 at 14:11
the fact that you told others in the answers below, that you've taken care of injection but did not mention it in the question, only in comments. Those answers don't qualify to be answers now, do they? what method of password storage are you using that you're also not telling us about? or is that a secret? plain text? md5? sha1? other? I know of many others. whirlpool, crypt, bcrypt, password_hash etc. you using one of those? — Funk Forty Niner, Feb 04 '16 at 14:13
@Fred-ii- so the question is , "How can I make this login script more secure?" here and there http://stackoverflow.com/questions/35131251/making-login-more-secure/35131364#35131364 and, maybe he already knows the answer and he knows how to take care of sql injection. As to the OP, what kind of answer do you expect if you do not share how are you authenticating. — Mark Ng, Feb 04 '16 at 14:24
@MarkNg No idea, sorry. OP didn't post their method for injection prevention and I don't even know if they're storing plain text passwords or not. I for one will not be submitting an answer and have voted to close the question as unclear. — Funk Forty Niner, Feb 04 '16 at 14:28
Actually my focus of the question is not on the whether what authentication to use, as I stated to 'assume there is no error'. My question is on whether this 'way of checking if user of logged in is secure'. Anyway, thanks for pointing out. — Panda, Feb 04 '16 at 14:29
you see, you're still not answering my question on how you're storing passwords — Funk Forty Niner, Feb 04 '16 at 14:31
I've updated the question, hopefully it clears up any misunderstanding — Panda, Feb 04 '16 at 14:33
your edit still doesn't tell me or others for that matter if you're storing a has or plain text password. Either way, wait for other possible answers, or get the others who have given you answers to improve on their answer. I will pass on this question. Good luck. — Funk Forty Niner, Feb 04 '16 at 14:35

deceze · Accepted Answer · 2016-02-04T15:12:13.913

Assuming that your query works as intended and only returns a row when the match is exactly correct (e.g. no weird fuzzy matching through collate rules, but pure bin comparison), the authentication part is pretty much fine.

_{(You have been warned about SQL injection plenty, you're on your own there.)}

Your security then boils down to this:

$_SESSION['logged_in'] = $username;

and the subsequent:

define('USERNAME', $_SESSION['logged_in']);

if (!USERNAME) {
    header("Location: login.php");
}

And I suppose your question is about this part.
Then the answer is: the session part is fine, the blocking is not.

That's how sessions are used, yes, and they're reasonably safe by default; a user won't be able to somehow set the $_SESSION['logged_in'] value themselves, the value can only be set by your server, and presumably you're doing so only on successful authentication. Do read up about session hijacking, this is the only real vulnerability to the whole scheme.

The real problem is:

if (!USERNAME) {
    header("Location: login.php");
}

Setting a header does not terminate the current page. If you're outputting sensitive information after this line, it will be sent to the client! You need to explicitly exit after setting the header.

Having said all this, we cannot tell you whether your system is "secure" because there may be any number of facepalm backdoors you have created which we're not seeing. In general I'd start with the following:

stop using mysql, use PDO or mysqli
bind your parameters, don't mysql_real_escape_string them; there are security pitfalls there
use password_hash password hashing, not SHA; especially if you're only doing a single SHA pass

score 1 · Answer 2 · answered Feb 04 '16 at 13:03

becareful SQL Injection :

If you type in password field :

 ''=''

The password's rule will be true, because Password = TRIM(''='') is true. You have to control the password's string :

Minimum length
No white space (thanks to Trim function)

And you don't have to store a password like this, you must make a password's hash

How can I check PHP Session in a more secure way?

2 Answers2