[RESOLVED] Critique of my simple login script!!

[sois]

Hello all, I'm a financial planner who happens to tinker with PHP in my spare time. I've created the following login script. Let me know what the security issues and general stupidness of this code is. I'm a minimalist and I like to keep the code as simple as possible. I have no formal training in this.

The code is in two parts. The first is a pretty standard login script:

login.php

// login form
print "<form method=post action=\"userauthenticate.php\">";
print "<table>";
$aq = mysql_query("select * from players order by NAME asc");

// list all members
print "<center><br><table><tr><td align=right colspan=2>";
print "<b>Login</b><br>";
print "<tr><td><select name=userid style=\"width: 100%\"><option SELECTED>- Select Name -</option>";
while ($a = mysql_fetch_array($aq))
{ print "<option value='$a[NAME]'>$a[NAME]";  }
print "</select></td>";
print "<td>Name</td></tr>";
print "<tr><td><input type=password name=password size=20></td><td>Password</td></tr>";
print "</table><br>";
print "<input type=submit value=\"Log in\"></form>";

I use a pull down menu for the users (since there are only seven users). I'm sure its not totally smart to do it that way, but with non-important data (my racquetball league stats), I don't think it matters too much.

Next is the "bulk" of my code. I use include("userauthorize.php"); on every page I consider "members only". Here is the code for you guys to read.

userauthenticate.php

include("dbconnectionfile.php");

if (isset($userid) && isset($password))
{
  $aq = mysql_query("select * from players where NAME = '$userid' and PASSWORD = '$password' ");
  $a = mysql_fetch_array($aq);
  if (mysql_num_rows($aq) > 0 )				// check info 
  {
    setcookie("user", "$a[NAME]", time()+1209600, "", "www.sois.com") or die("Could not set cookie"); 
    setcookie("sess",  $PHPSESSID, time()+1209600, "", "www.sois.com") or die("Could not set cookie"); 

$sets = mysql_query("update players set SESSID = '$PHPSESSID' where NAME = '$userid' ") or die (mysql_error());

  // tell user that he is logged in here 

  }
  else							// info incorrect
  {
    print "<h2>Wrong Password</h2>";
    include("loginscript.php"); 			// send them back to login
  } // end check info
}

// use the recently set cookie to "verify" player data and then use that stuff later
$aq = mysql_query("select * from players where SESSID = '$_COOKIE[sess]'");
$a  = mysql_fetch_array($aq);

Like I said, if I want a page to be secure I just include this page. Here is an example of implementation:

samplememberpage.php

include("user authenticate.php");

// un comment this if this user has power to enter stats
$membercheck = $a[MEMBERSTATUS]; if ($membercheck < 1) {  print "<center><h2>This page is for site league members only.</h2></center>"; exit(); }
// put member stuff here

So it's pretty simple, but I think it is going to get the job done. Feel free to put me down or suggest reasons why this is totally stupid. Thanks all!!!!!!

[schajee]

First of all its not totally stupid, its a good start, although there are areas of grave concern.

First of all, I noticed you are using register_globals=On, which is not usually recommended unless you are sitting on something nobody wants to hack. Try to use $POST['username'] and $POST['password'].

Also when you include ("userauthenticate.php") in samplememberpage.php which includes userauthenticate.php, there is no $username or $password are more, so no matter how many times one logs in, he'll never be allowed inside the member pages.

You can do all the login-ing, checks, set cookies, blah blah blah in one page, but that's for later. Right now you just need to fix the flow of you application from one script to another.

There should also have been a lot of errors or warnings about undefined variables.

Hope this inspires...

[sois]

Thanks for the awesome and quick reply. I have changed the code. Does this work in regard to $_POST variables?

$userid   = $_POST['username'];
$password = $_POST['password'];

if ($userid || $password)
{
  $aq = mysql_query("select * from players where NAME = '$userid' and PASSWORD = '$password' ");
  $a = mysql_fetch_array($aq);
// etc.... same as before until last section
$aq = mysql_query("select * from players where SESSID = '$_COOKIE[sess]' and NAME = '$_COOKIE[user]' ");
$a  = mysql_fetch_array($aq);

I am testing everything right now and everything seems to be working. The logic of my code goes like this:

If guy tries to login check to see if this stuff is valid.
If so, assign cookies for session ID and player name.
If guy did not try to login, use cookie information.

If you look above on that last section, before userauthenticate.php is done, the DB is queried again with the cookie information. Since I include() this page on all "member only" pages, it will use that query on the next page.

For example, the first thing memberpage.php does is:
1. include(userauthenticate.php);
2. take the results of that last query on userauthenticate and see if the use has permissions for that page.

Thanks for the input, this is really really helpful in learning how to do this stuff 🙂

[schajee]

Quick question... try to trace this... What happens after 14 days?

I'll be back with more

Ps. There is no such thing as the results of the last query, especially if it is conducted a page ago.

[sois]

I think you're referring to the cookie? I figure two weeks is enough time to boot them off if they don't login again.

[sois]

Re the PS:

I am under the assumption that include() just sticks two pages together. For example, if I have on page1.php:

print "howdy... ";

and on page2.php i have:
include("page1.php");
print "pardner!";

page2.php will display:
howdy ... pardner!

[schajee]

I don't want to write any piece of code here since you are learning right now... and any code here will only complicate matters.

Let me guide you through your script...

1. User accesses memberpage.php, say after a month. Cookies are no longer there as they have expired 2 weeks ago
2. First line loads user authenticate.php
3. user authenticate.php makes connection with the db
4. Then it checks for $userid and $password -> These two variables are non-existent, as you have not defined them anywhere, although depending on your PHP settings and the mysterious workings of "isset" the check here might let the script go through
5. Next up is query... where it will probably return no rows as the values of $userid and $password are both NULL.
6. Which means your next "if" statement will not execute because the mysql_num_rows == 0
7. So you skip to the else part where you print "Wrong Password" and then u include the login form
8. Now step 4 may or may not happen. If it does, then you go to step 5, otherwise you end up at step 9
9. This is assuming isset actually did verify the non-existent nature of $userid and $password, the next query will not yield any results because $_COOKIE['sess'] does not exist
10. This means $a will basically hold nothing
11. Now we get back to memberpage.php where the array $a will hold a variable MEMBERSTATUS and assigns it to $membercheck
12. Next is where it gets weird... and if you say this system is working fine, then I must start all over again

Hope this helps

[sois]

schajee wrote:

I don't want to write any piece of code here since you are learning right now... and any code here will only complicate matters.

Let me guide you through your script...

1. User accesses memberpage.php, say after a month. Cookies are no longer there as they have expired 2 weeks ago
2. First line takes loads user authenticate.php
3. user authenticate.php makes connection with the db
4. Then it checks for $userid and $password -> These two variables are non-existent, as you have not defined them anywhere, although depending on your PHP settings and the mysterious workings of "isset" the check here might let the script go through
5. Next up is query... where it will probably return no rows as the values of $userid and $password are both NULL.
6. Which means your next "if" statement will not execute because the mysql_num_rows == 0
7. So you skip to the else part where you print "Wrong Password" and then u include the login form
8. Now step 4 may or may not happen. If it does, then you go to step 5, otherwise you end up at step 9
9. This is assuming isset actually did verify the non-existent nature of $userid and $password, the next query will not yield any results because $_COOKIE['sess'] does not exist
10. This means $a will basically hold nothing
11. Now we get back to memberpage.php where the array $a will hold a variable MEMBERSTATUS and assigns it to $membercheck
12. Next is where it gets weird... and if you say this system is working fine, then I must start all over again

Hope this helps

I edited the isset out of the code. Now im using a simple if ($userid || $password).

When you get to #10 and $a holds nothing, $a[MEMBERSTATUS] will return null. The code on the samplemember.php page will use this code:

// un comment this if this user has power to enter stats
$membercheck = $a[MEMBERSTATUS]; if ($membercheck < 1) {  print "<center><h2>This page is for site league members only.</h2></center>"; exit(); }

That code (I believe) will check to see if the player has membership status from that $a query. If not, it exit()'s and nothing else on the page happens.

I think everything you wrote in summary is correct. Thanks for checking this over! Let me know if there are any more grave concerns 🙂.

[schajee]

One more thing... before you do anything with any variable, always print to make sure you are getting the desired values.

For string just use print $string and use print_r ($array) to print arrays. This is the best way to debug your application. And make sure your error reporting is turned on in the php.ini and set to E_ALL. Right now I don't think it is.

[sois]

hmmm i have no idea how to turn on error reporting in php.ini.

[schajee]

I'm presuming u r using windows, then php.ini should be in the directory where u installed PHP, generally C:\PHP.

PHP.ini is a file that is parsed for settings every time the PHP engine is called upon. Any changes here are immediately reflected in the page upon refresh.

Just open the php.ini file in your favorite text editor, look for error_reporting. The 2nd result will take you to the Error Reporting section, looking like this...

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
; Error handling and logging ;
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

; error_reporting is a bit-field. Or each number up to get desired error
; reporting level
; E_ALL - All errors and warnings
; E_ERROR - fatal run-time errors
; E_WARNING - run-time warnings (non-fatal errors)
; E_PARSE - compile-time parse errors
; E_NOTICE - run-time notices (these are warnings which often result
; from a bug in your code, but it's possible that it was
; intentional (e.g., using an uninitialized variable and
; relying on the fact it's automatically initialized to an
; empty string)
; E_CORE_ERROR - fatal errors that occur during PHP's initial startup
; E_CORE_WARNING - warnings (non-fatal errors) that occur during PHP's
; initial startup
; E_COMPILE_ERROR - fatal compile-time errors
; E_COMPILE_WARNING - compile-time warnings (non-fatal errors)
; E_USER_ERROR - user-generated error message
; E_USER_WARNING - user-generated warning message
; E_USER_NOTICE - user-generated notice message
;
; Examples:
;
; - Show all errors, except for notices
;
;error_reporting = E_ALL & ~E_NOTICE
;
; - Show only errors
;
;error_reporting = E_COMPILE_ERROR|E_ERROR|E_CORE_ERROR
;
; - Show all errors
;
error_reporting = E_ALL

Just make sure this last line has no ";" before it or if any other line doesn't have one, add one there and remove from the last line. ";" denotes a comment in the php.ini.

Save the file and hit refresh in your browser.

[sois]

I have my webpage on a webserver I pay a few bucks a month for. I don't know if I have access to that.

Should my code have errors? What are the things you are noticing that are totally not good?

[Roger_Ramjet]

The biggest security worry is an sql injection attack. With your code, if I just entered

blah' OR '1' = '1

then your query will end up being

$aq = mysql_query("select * from players where NAME = '$userid' and PASSWORD = 'blah' OR '1' = '1' ");

and '1' = '1' is always true, so it will login the hacker.

Even more malicious code can be introduced into your queries in this way.

Read about it Here and then use the 'best practice' code that is provided. Just copy it into an include file as is and use it for ALL user input.

[sois]

Roger Ramjet wrote:

The biggest security worry is an sql injection attack. With your code, if I just entered

blah' OR '1' = '1

then your query will end up being

$aq = mysql_query("select * from players where NAME = '$userid' and PASSWORD = 'blah' OR '1' = '1' ");

and '1' = '1' is always true, so it will login the hacker.

Even more malicious code can be introduced into your queries in this way.

Read about it Here and then use the 'best practice' code that is provided. Just copy it into an include file as is and use it for ALL user input.

wow thanks for the heads up. i will use this stuff tomorrow. if you guys see anything else, please post more, this is great knowledge!

[schajee]

There really should be a lot of errors and warnings. I can't really tell whether its working alright or not, by any decent standard.

And no, you don't have access to the php.ini and since its a production server, most probably they have disabled error warning.

Lemme try to simplify this problem by giving you my (and in most cases other's) algorithm or the way this problem should be solved.

First up, this script calls itself! If you are not familiar with this concept, you might or might not want to use this approach as it clutters the code a bit, but it makes the whole process self contained.

##########

Section I: (Check for existing session)

--> if session exists then
----> take user back to where he came from

Section II: (Check for whether this form was posted or not)

--> if $POST exists then
----> if $POST['username'] and $_POST['password'] are cleared for invalid characters or empty strings
------> if database validation is ok then
--------> assign cookies, set sessionids, take user back to where he came from
------> if database validation is not OK (user does not exist in database) then
--------> set error flag(s)
----> if inputs are not cleared then
------> set error flag(s)

Section III: (Main HTML Form)

--> if you maintain one error flag then print error reason or message
--> print out the form
--> set form action to $SERVER['PHP_SELF'] (form submits to itself)
--> input field(s) => if error flag(s) was/were raised, set value(s) to $POST['name of input field'])

##########

And that's it... its as simple as this. Usually there is a lot more stuff for user friendliness, but this should suffice for now

Hope this helps

[Roger_Ramjet]

You should also change your db validation to check that 1 and only 1 row is returned

  if (mysql_num_rows($aq) == 1 )                // check info 

// NOT

  if (mysql_num_rows($aq) > 0 )                // check info 

Just a further check that you are not being hacked, the sql examle I gave would return all users

[Weedpacket]

sois wrote:

I have my webpage on a webserver I pay a few bucks a month for.

Ideally you should have PHP and a web server running on your own computer as well so that you don't have to do your coding with the whole world watching.

On your computer error reporting should be set to show E_ALL errors as schajee says, and display_errors should be turned on (and log_errors too, for convenience), while on your host display_errors should be turned off (and have log_errors turned on, and have access to those logs).

Basically you want your development server to make a big fuss as soon and as noisily as possible as soon as something goes wrong (so that you can find the problem to fix it), while the live server just quietly informs the user that there has been an [unspecified] error without going into detail that they don't want (and if they do want they shouldn't have).

[sois]

schajee wrote:

There really should be a lot of errors and warnings. I can't really tell whether its working alright or not, by any decent standard.

I emailed the guys at my server and they told me E_ALL is now set. I don't see any errors. Where should I see them? Will they show up on the page?

Roger Ramjet wrote:

You should also change your db validation to check that 1 and only 1 row is returned.

Done! Thanks for the great idea!

[schajee]

Can you attach your current scripts, so i can see whether I see any errors or not. Maybe I haven't debugged properly.

[sois]

Here is the three pages attached as a text file. Thanks for checking this out!