Page 1 of 2

General PHP question.

Posted: Sun Jan 03, 2010 10:28 pm
by Payton
Hi all.

Last time I was here, I said something about a project I was working on, called PHP Quick Journal (at least I think I said something about it). Well, I recently purchased a book on PHP to improve, and so far, I have been doing well with the project.

Still, though, I have questions. :mrgreen:

I basically need to know: Will the following script work? It's supposed to serve as a log in page, and according to this website, my syntax is correct, however I have not tested it yet, so I just wanted to make sure. I will be testing it soon.

Code: Select all

<?php
 
/*
Ramen
*/
 
include('../files/functions.php'); //Include our functions.
 
//Check to see if the form has been submitted.
if (isset ($_POST['submit'])) {
    if (($_POST['username'] == '') or ($_POST['password'] == '')) {
        echo "Something was left blank. Please go back and try again.";
    }
    
    else {
        connect(); //Connect to the database.
        if (!connect()) {
            die(mysql_error());
        }
    
        $check = "SELECT * FROM ".$prefix."users WHERE username == '{$_POST['username']}' && password == '{$_POST['password']}'";
        mysql_query($check);
        
            if (!$check) {
                die ('There was a problem logging in. Please try again.');
            }
            
            else {
                session_start(); 
                $_SESSION['loggedin'] = 'yes';
                echo "You have logged in successfully. <a href='index.php'>Click here to continue to the index.</a>";
            }
}
}
 
else {
   //Form stuff goes here.
}
 
?>

Re: General PHP question.

Posted: Sun Jan 03, 2010 11:45 pm
by requinix
Line 21: "==" is not a valid operator. Just use "="
Line 21: SQL injection
Line 21: poor design to store passwords in plaintext
Line 22: this only executes the query - it won't tell you if there are any username/password matches
Line 24: will never work properly. Check the return value of mysql_query

Re: General PHP question.

Posted: Mon Jan 04, 2010 12:00 am
by Payton
tasairis wrote:Line 21: "==" is not a valid operator. Just use "="
Line 21: SQL injection
Line 21: poor design to store passwords in plaintext
Line 22: this only executes the query - it won't tell you if there are any username/password matches
Line 24: will never work properly. Check the return value of mysql_query
See, now I'm glad I asked. :P

1: Thanks for the tip.
2: Oh lord, don't want that happening. Can you be more elaborate as to how it would be done?
3: It isn't meant to store passwords, if that's what you meant.
4: Thanks -again- for the tip.
5: Okay.

Re: General PHP question.

Posted: Mon Jan 04, 2010 12:24 am
by flying_circus
Payton wrote: 2: Oh lord, don't want that happening. Can you be more elaborate as to how it would be done?
Always escape data before pushing it into a database. Mysql has a built in escape function just for this purpose and I really like the sprintf function to implement it.


Consider the following pseudo code (it may need to be tweaked, but reading the manual should make it obvious)

Code: Select all

 
$check = sprintf("SELECT * FROM {$prefix}users WHERE username='%s' && password='%s';",
                  mysql_real_escape_string($_POST['username']),
                  mysql_real_escape_string($_POST['password']));
 
Edit: also check if magic quotes is enabled. If it is, be sure to strip slashes before escaping, or you can run into a double escaping (negating) scenario.

Re: General PHP question.

Posted: Mon Jan 04, 2010 1:08 pm
by Payton
flying_circus wrote:
Payton wrote: 2: Oh lord, don't want that happening. Can you be more elaborate as to how it would be done?
Always escape data before pushing it into a database. Mysql has a built in escape function just for this purpose and I really like the sprintf function to implement it.


Consider the following pseudo code (it may need to be tweaked, but reading the manual should make it obvious)

Code: Select all

 
$check = sprintf("SELECT * FROM {$prefix}users WHERE username='%s' && password='%s';",
                  mysql_real_escape_string($_POST['username']),
                  mysql_real_escape_string($_POST['password']));
 
Edit: also check if magic quotes is enabled. If it is, be sure to strip slashes before escaping, or you can run into a double escaping (negating) scenario.
Thanks for that. I'll read up more on those (sprintf and mysql_real_escape_string).

Re: General PHP question.

Posted: Mon Jan 04, 2010 5:31 pm
by Payton
Here is a newer version of the script.

Code: Select all

<?php
 
/*
Ramen
*/
 
include('../files/functions.php'); //Include our functions.
 
//Check to see if the form has been submitted.
if (isset ($_POST['submit'])) {
    if (($_POST['username'] == '') or ($_POST['password'] == '')) {
        echo "Something was left blank. Please go back and try again.";
    }
   
    else {
        connect(); //Connect to the database.
        if (!connect()) {
            die(mysql_error());
        }
   
        $check = sprintf("SELECT * FROM {$prefix}users WHERE username = '%s' && password = '%s'",
            mysql_real_escape_string($_POST['username']); 
            mysql_real_escape_string($_POST['password']));
        $query = mysql_query($check);
       
            if (!$query) {
                die ('There was a problem logging in. Please try again.');
            }
           
            else {
                mysql_close(connect()); //Close the MySQL connection
                session_start();
                $_SESSION['loggedin'] = 'yes';
                echo "You have logged in successfully. <a href='index.php'>Click here to continue to the index.</a>";
            }
}
}
 
else {
   //Form stuff goes here.
}
 
?>

Re: General PHP question.

Posted: Mon Jan 04, 2010 8:41 pm
by Griven
Overall
1. The MySQL extension is meant for older versions of MySQL. Although it will work for newer versions, you may want to consider using the MySQLi extension instead.
2. You really should hash the passwords in your database with MD5 (at least) or SHA1. Storing them in plain text is very bad practice and significantly lessens the security of your application.

Line 18 - I don't recommend dumping database errors directly to the page in a production environment. Rather, place a string there saying that an error occurred. Better yet, add an email function that sends you a message when the error occurs.

Line 21 - Is $prefix being passed from somewhere else that we're not seeing? If not, then the server will consider this an undefined variable, and thrown an error.

Line 22, 23 - Escape the POST data separately prior to forming the query. Your current format throws a syntax error due to the semicolons being inside the sprintf() function.

Line 26 - Currently, this line just checks whether or not the query ran successfully. Right now, even if I were to enter the wrong username and password, and zero rows were returned, it would still log me in. You need to check how many rows were returned, and go from there.

Re: General PHP question.

Posted: Mon Jan 04, 2010 8:49 pm
by Payton
Griven wrote:Overall
1. The MySQL extension is meant for older versions of MySQL. Although it will work for newer versions, you may want to consider using the MySQLi extension instead.
2. You really should hash the passwords in your database with MD5 (at least) or SHA1. Storing them in plain text is very bad practice and significantly lessens the security of your application.

Line 18 - I don't recommend dumping database errors directly to the page in a production environment. Rather, place a string there saying that an error occurred. Better yet, add an email function that sends you a message when the error occurs.

Line 21 - Is $prefix being passed from somewhere else that we're not seeing? If not, then the server will consider this an undefined variable, and thrown an error.

Line 22, 23 - Escape the POST data separately prior to forming the query. Your current format throws a syntax error due to the semicolons being inside the sprintf() function.

Line 26 - Currently, this line just checks whether or not the query ran successfully. Right now, even if I were to enter the wrong username and password, and zero rows were returned, it would still log me in. You need to check how many rows were returned, and go from there.
2: At the registration, I plan on doing that. Would I need to change something in my login script to work with the MD5 hash that is stored in the database?

Line 18: Will do.

Line 21: $prefix comes from the functions.php file that I included at the top of the script. I probably also need to include config.php since that's where it's ACTUALLY coming from; functions.php includes config.php and uses $prefix, so I assume that just including functions.php would work.

Line 22: Okay.
Line 23: Thanks for the heads up. I'll change that.

Line 26: Don't want to be logged in if we aren't putting in the right credentials, that's a big security hole on the user's end. :P Thanks for the heads up, I'll figure out a way to do what you said. Would mysql_num_rows work?

I'd just like to note, I do appreciate everyone's help more than I probably express it here. :oops: I love programming, and I can't much call myself a programmer if I don't learn how to program successfully. So thank you, again. :wink:

Re: General PHP question.

Posted: Mon Jan 04, 2010 9:29 pm
by Griven
I forgot to mention... Take a look at WAMP (Windows) or MAMP (Mac). Those will give you the testing environment you need on your local machine by giving you an Apache web server with PHP, along with a MySQL server. They're indispensable tools, in my opinion.
Payton wrote:I'd just like to note, I do appreciate everyone's help more than I probably express it here.
It's refreshing to see someone who's putting the effort into their programming, rather than expecting us to do it for them. Helping us help you is the best way to gain assistance here (kudos).
Payton wrote:Line 26: Don't want to be logged in if we aren't putting in the right credentials, that's a big security hole on the user's end. Thanks for the heads up, I'll figure out a way to do what you said. Would mysql_num_rows work?
Try replacing lines 21 - 28 with the following code:

Code: Select all

$username = mysql_real_escape_string($_POST['username']);
$password = mysql_real_escape_string($_POST['password']));
 
$check = sprintf("SELECT * FROM {$prefix}users WHERE username = '%s' && password = '%s'", 
    $username, 
    $password);
    
$result = mysql_query($check);
 
if (mysql_num_rows($result) != 1){
    echo 'Your username or password are incorrect.  
        Please go back and try again.';
}
Password Encryption
Payton wrote:2: At the registration, I plan on doing that. Would I need to change something in my login script to work with the MD5 hash that is stored in the database?
You'll need to add a few things to the script, at the very least. It's likely that your database can stay the same (just make sure that it can store at least 40 characters in the password field).

When users sign up, hash their passwords using MD5 or SHA1 (preferably), along with a salt, and then store that in the database.

When they go to log in, hash the password they provide with whatever hashing method you chose before (MD5 or SHA1) and that same salt. If those passwords and their username match, then they're good to go.

If you have other questions regarding this, feel free to PM me.

Re: General PHP question.

Posted: Mon Jan 04, 2010 11:26 pm
by Payton
About the password encryption; would hashing the password ($hash = sha1($password);) and then switching $password in sprintf to $hash work? Here is the code:

Code: Select all

$username = mysql_real_escape_string($_POST['username']);
        $password = mysql_real_escape_string($_POST['password']));
       
        $hash = sha1($password); //Hash our password.
        $check = sprintf("SELECT * FROM {$prefix}users WHERE username = '%s' && password = '%s'",
            $username,
            $hash);
   
        $result = mysql_query($check);
 
        if (mysql_num_rows($result) != 1){
            echo 'Your username or password are incorrect.  
            Please go back and try again.';
        }

Re: General PHP question.

Posted: Mon Jan 04, 2010 11:56 pm
by Griven
That should work fine.

Re: General PHP question.

Posted: Tue Jan 05, 2010 12:34 am
by Payton
Okay, with everyone's help, this is the final product.

Code: Select all

<?php
 
/*
Ramen
Login script
Version 1.0 
Thanks to http://forums.devnetwork.net for their help :P
*/
 
include('../files/functions.php'); //Include our functions.
 
//Check to see if the form has been submitted.
if (isset ($_POST['submit'])) { 
    if (($_POST['username'] == '') or ($_POST['password'] == '')) { //If something was left blank...
        echo "Something was left blank. Please go back and try again."; // ... we get this.
    }
   
    else { //But if nothing was left blank...
        connect(); //... connect to the database.
        if (!connect()) { //If we couldn't connect...
            die(mysql_error()); //... we display a MySQL error.
        }
   
        $username = mysql_real_escape_string($_POST['username']); //Our username from the form.
        $password = mysql_real_escape_string($_POST['password'])); //Our password from the form.
        
       /*
       We put the username and password after mysql_real_escape_string so an SQL injection isn't possible. 
       It strips the form submission of special characters when it's queried. 
       */
       
        $hash = sha1($password); //Hash our password in sha1. 
        $check = sprintf("SELECT * FROM {$prefix}users WHERE username = '%s' && password = '%s'", 
            $username,
            $hash);
   
        /*
        Above, we select from our users table where our username equals $username and password equals $hash. 
        We don't use $password because $hash is what is stored inside the database. 
        */
        
        $result = mysql_query($check); //Execute a MySQL query. 
 
        if (mysql_num_rows($result) != 1){ //If we could not find a match for what the user submitted...
            echo 'Your username or password are incorrect.  
            Please go back and try again.'; //... we display this. 
        }
           
        else { //If all went well:
                mysql_close(connect()); //Close our connection.
                session_start(); //Since we've logged in, start a session.
                $_SESSION['loggedin'] = 'yes'; //The session is named loggedin, and is set to 1. 
                echo "You have logged in successfully. <a href='index.php'>Click here to continue to the index.</a>"; //Display this message.
            }
}
}
 
else { //If the form has not been submitted yet...
   echo "<form action='login.php' method='post' name='submit'>
   Username: <br />
   <input type='text' name='username' /> <br />
   Password: <br />
   <input type='password' name='password' /> <br />
   <input type='submit' />"; //.., we will show this form.
}
 
?>
I'll test it tonight! Thanks everyone. I'll use it as a reference for my registration script. :)

Re: General PHP question.

Posted: Tue Jan 05, 2010 2:25 am
by flying_circus
I re-wrote your code how I personally would write it. Just want to present a different perspective... OK, so really, I can't sleep and I like your enthusiasm 8)

I added a little functionality to track user login attempts and left a little bit of homework in there, if you are up to the challenge. Otherwise you may just delete what I've done and go on your merry way.

A few tips:

magic quotes can throw a wrench in your plan, in a hurry. If magic quotes are enabled on the server and then you escape the data before putting it through the database, you run the risk of double-escaping, or negating escaping, which can cause you headaches in the best case scenario.

escaping data before it gets put into a query/database is the LAST thing to do before it goes into the query or database. The last
thing you want to do is escape the data, then change it, then run it through the database. This leaves the door wide open for un-intentional errors or sql injection.

As far as starting a session. Feel free to start it at the beginning of each page, whether it is accessed or not on that particular page. It uses an insignificant ammount of resources (provided you aren't abusing it, and storing a rediculous ammount of data within it).

Also, rather then echoing a hyperlink for the user to follow to the next page, you can use the header() function and automatically direct them where you want them to go. It's the preferred method to keep any users from monkey'ing with your system.

try to use include_once(), instead of include(). They do the same basic function, except if you try to include the same file by accident later, it wont be included.

I dont fully understand how your connect() function works. Typically you would assign a database connection to a variable, as in: $database = connection(). However, your method might just work fine, provided it's implemented correctly. I just cant see that part of the code. Be sure to test it on your own to verify it's doing what you intend.

lastly, pay special attention to how I constructed your SQL query, using back-ticks (`) around column and table names and singe quotes(') around data. Dont forget to terminate the query with a semi colon either. It's proper syntax, good code, and prevents SQL injection.

ok, a shot of whiskey and then it's bed time for me. :drunk:

Code: Select all

<?php
/**
 * Ramen
 * Login script
 * Version 1.0
 * Thanks to http://forums.devnetwork.net for their help :P
 */
 
  # Begin Session
    session_start();
    
  # Disable Magic Quotes
    if(get_magic_quotes_gpc()) {
      $input = array(&$_GET, &$_POST, &$_COOKIE, &$_ENV, &$_SERVER);
      
      while(list($k, $v) = each($input)) {
        foreach($v as $key => $val) {
          if(!is_array($val)) {
            $input[$k][$key] = stripslashes($val);
            continue;
          }
          $input[] =& $input[$k][$key];
        }
      }
      
      unset($input);
    }
  
  # Vars
    $max_login_attempts = 5;
    $username = (isset($_POST['username'])) ? $_POST['username'] : "";
    $password = (isset($_POST['password'])) ? $_POST['password'] : "";
    
  # Includes
    include_once(dirname(__FILE__) . '../files/functions.php');  // Always use a full path when possible
    
  # Check to see if the form has been submitted.
    if(isset($_SERVER['REQUEST_METHOD']) && strtolower($_SERVER['REQUEST_METHOD']) == "post") {
    # Increment LoginAttempts Session Var
      $_SESSION['loginAttempts'] = (isset($_SESSION['loginAttempts'])) ? $_SESSION['loginAttempts'] + 1 : 1;
      
    # Check if max login attempts has been exceeded
      if($_SESSION['loginAttempts'] > $max_login_attempts) {
        echo "Too many login attempts.  Please try again in xx minutes";
        exit();
        // To Do: Build code that checks if it has been xx minutes since last login
          // We can do this by assigning a timestamp to the users session (i.e. $_SESSION['timestamp'] = strtotime("now"))
      }
      
    # Sanity Check
      if(empty($username) || empty($password)) {
        echo "Something was left blank. Please go back and try again.";
        exit();
      } else {
      # Connect to the Database
        if(!connect()) die(mysql_error());
        
      # Query Database for user credentials
        $result = mysql_query(sprintf("SELECT * FROM `{$prefix}users` WHERE username='%s' && password='%s';", 
                                       mysql_real_escape_string($username),
                                       mysql_real_escape_string(sha1($password))));
                                       
        if(mysql_num_rows($result) == 1) {
        # Login Successful
          mysql_close(connect()); // Close our connection.
          $_SESSION['loggedin'] = 1; // The session var named loggedin is set to 1. 
          unset($_SESSION['loginAttempts']); // Set the loginAttempts Session Var back to 0.
          header("location:https://www.mydomain.com/my_secure_index.php"); // Redirect user to user's home
          exit();
        } else {
        # We could not find a match for what the user submitted (or a duplicate)...
          echo 'Your username or password are incorrect.  Please go back and try again.';
          exit();
        }
      }
    }
?>
      <form action="login.php" method="post" name="submit">
        Username: <br />
        <input type="text" name="username" value="<?php print $username; ?>" /> <br />
        Password: <br />
        <input type="password" name="password" /> <br />
        <input type="submit" />
      </form>

Re: General PHP question.

Posted: Tue Jan 05, 2010 10:16 am
by Payton
Wow, that looks quite different. After reading everything you typed up there, I am kind of scared to even use my script. 8O I'll take a look over your modification and see what you did differently, thanks.

Re: General PHP question.

Posted: Tue Jan 05, 2010 8:43 pm
by flying_circus
Payton wrote:Wow, that looks quite different. After reading everything you typed up there, I am kind of scared to even use my script. 8O I'll take a look over your modification and see what you did differently, thanks.
Hah, jump on in, the waters fine! Nobody told me all of those things when I started, so I figured I'd try to educate rather than just provide an answer :)

If you have any questions about the code, either how it works or why I did it that way, post up your question. It's only usefull if it can teach someone something.

Don't be shy about the code you are able to produce, the only real way to learn is by doing. I look back on code I wrote 2 years ago and am somewhat disgusted that I charged for it. Such is life. Keep at it, you're doing fine.