A Simple, One-Page PHP Admin Login (with prepared SQL statements)

First off, I want mention that this code works well. This is more of a request for suggestions...

We're attempting to program a one-page, recursive, token-based admin shell that is safe from intrusion, and replicable for several DB backend manipulation pages via tokening. We wanted to protect it from SQL injection (thus all SQL statements are prepared and sanitized) and from being able to relog by reposting form data when hitting the back button.

We could use some critiquing (please be gentle) and breaktesting if you're up to it! Everything is procedural to improve readability. We avoided "get_result()" since this is not supported on our host and is inconsistently installed among various hosts.

A couple of things we are considering:

• A regex test on user input to further mitigate SQL injection possibilities.

• Simplifying the code (getting rid of a lot of variable flushes)

• Hashing passwords (I know, this should be done ASAP - working on an input form to be able to enter hashed passwords now)

• ... Anything else should we consider??

Here's a general overview of what this code does:

• Connects to your DB (table needs are described in preamble)

• [1.1] If() a logout flag has been POSTed...

• --- [1.1.1] If() a SESSION token is set...

• ------ Prepare a SQL statement that pulls the token from the admin log

• ------ [1.1.1.1] If() the number of rows pulled is equal to zero...

• --------- Set the token expiration to now

• ------ [1.1.1.2] Else()...

• --------- Report a logout error ("Token does not exist")

• ------ Close the SQL statement

• --- [1.1.2] (No else() for this tier)

• --- Flush SESSION and POST variables

• [1.2] Elseif() a login flag has been POSTed with username, password and temp token...

• --- Prepare a SQL statement that pulls the password of the supplied username

• --- [1.2.1] If() the number of rows from the password pull is zero...

• ------ Report a login error ("User does not exist")

• ------ Flush SESSION variables

• --- [1

Security

• I'm not all that comfortable with echoing unsanitized user input: echo '';. I did not find any way to exploit it though. I asked a question on security.SE, maybe they can find a way. I would probably use SCRIPT_NAME instead.

• you should regenerate the session id via session_regenerate_id(true); if something in the session changes (such as on login) to prevent session fixation.

• it's not all that clear what a_token/temp_token do, but they seem to be some sort of session id inside the session, which isn't necessary for security (and if it were necessary, basing it on the time isn't a good idea).

• You probably should not be storing database credentials in a publicly available PHP file, but in a configuration file outside the webroot instead (some editors leave backup files (eg admin_login.php~) which could be read if you made small changes on the production server, you always have to remember to remove them when sharing code, etc).

• If you follow the previous point and now have a configuration file, I would create a DEBUG field in it and use that in your code instead of relying on developers to comment/uncomment lines for debugging (will they remember it each time in all of the places? Probably not).

• A regex test on user input to further mitigate SQL injection possibilities.: You could do that, or you could just use a web application firewall such as mod_security. If you do write your own code, I would put it all in one file that cleans all POST/GET variables which then is included everywhere instead of performing these on each POST/GET variable individually.

• use a timing safe compare function to compare passwords (although you really should hash your passwords. If you use bcrypt it will handle the comparison and even salts for you).

Readability

• Everything is procedural to improve readability: are you sure about that? Personally, I would prefer OOP and MVC. But either way, procedural style does allow the use of functions, which I would highly recommend, as it helps you to structure your code into logical blocks (login, logout, index, etc) which makes your code easier to read, and it helps to avoid code-duplication. Eg you have your login form three times! This makes it really difficult to read the code (are they really exactly the same? If not, what is the small difference?), and it makes it hard to maintain (If I want to add an extra field or change the layout, I have to do it in three places).

• your comments are way too much (I'm not even talking about the structured policy comments, but about Database username, Kills the [...], etc). I read the first couple of them, realized that they just say what the code already said, and then ignored all future comments. Most readers will behave the same way. But what if there is something important you have to say in the comments? People would ignore that as well.

Functionality

• unsetting POST will not prevent form resubmits, so I would just get rid of it. If you are sure that you need it, I would write a generic unsetAllPost function and use that to reduce code duplication.

• always write output as if this was already on a production server. Unless this is code for a very specific niche, calling people monkey is just not acceptable.

StackExchange Code Review Q#84641, answer score: 11