Is my script secure?

Discussions of secure PHP coding. Security in software is important, so don't be afraid to ask. And when answering: be anal. Nitpick. No security vulnerability is too small.

Moderator: General Moderators

Post Reply
Etherwood
Forum Newbie
Posts: 4
Joined: Tue Dec 01, 2009 3:10 pm

Is my script secure?

Post by Etherwood »

I have had a look through the hotscript directory for a script which is suitable for my requirements. Unfortunately I wasn't able to find one so I'm having to create my own from scratch. So far I have got the register.php and login.php scripts done. I would like someone to have a look and tell me if the scripts I have made is considered secure. The information being held on the server needs to be as secure as possible.

register.php

Code: Select all

<?php
session_start();
include("config.php");
include("inc.php");
?>
 
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en">
<head>
<meta name="Description" content="" />
<meta name="Keywords" content="" />
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1" />
<meta name="Robots" content="index,follow" />
<link rel="stylesheet" href="style.css" type="text/css" />
<title>Registration</title>
</head>
 
<?php
include('header.php');
include('leftbar.php');
include('rightbar.php');
?>
 
<div id="main">
<a name="TemplateInfo"></a>
<h1>Register New User</h1>
 
<?php
if (isset($_POST['submit'])) {
  // Form Submitted
  require_once('recaptchalib.php');
  $privatekey = "";
  $resp = recaptcha_check_answer ($privatekey,
  $_SERVER["REMOTE_ADDR"],
  $_POST["recaptcha_challenge_field"],
  $_POST["recaptcha_response_field"]);
 
  if (!$resp->is_valid) {
    die ("The reCAPTCHA wasn't entered correctly. Go back and try it again.");
  } else {
 
    // ReCaptcha Code Entered Correct
    // Validate Username
    if ($_POST['username'] != "") {
      $username = filter_var($_POST['username'], FILTER_SANITIZE_STRING);
      if ($username == "") {
        $errors .= 'Please enter a valid username.<br/><br/>';
      }
    } else {
      $errors .= 'Please enter your a username.<br/>';
    }
 
   // Validate Password
    if ($_POST['password'] != "") {
      $password = md5($_POST['password']);
    } else {
      $errors .= 'Please enter your a password.<br/>';
    }
 
   // Validate Name
    if ($_POST['name'] != "") {   
      $name = filter_var($_POST['name'], FILTER_SANITIZE_STRING);   
      if ($name == "") {   
        $errors .= 'Please enter a valid name.<br/><br/>';   
      }   
    } else {   
      $errors .= 'Please enter your a name.<br/>';   
    }
 
    if ($_POST['email'] != "") {
      $email = filter_var($_POST['email'], FILTER_SANITIZE_EMAIL);
      if (!filter_var($email, FILTER_VALIDATE_EMAIL)) {
        $errors .= "$email is <strong>NOT</strong> a valid email address.<br/><br/>";
      }
    } else {
      $errors .= 'Please enter your email address.<br/>';
    }
 
    // Check For Errors
    if (!$errors) {
      $query=mysql_query("select * from user where username like '$username'") or die(mysql_error());
      if(mysql_num_rows($query)==0){
        @mysql_query("insert into user (username, password, name, email, date) values('$username','$password','$name','$email', NOW())");
        echo "Thank you, Your account has been created.";
      } else {
        echo '<div style="color: red">That username has already been taken, Please go back and try another.</div>';
      }
    } else {
      echo '<div style="color: red">' . $errors . '<br/></div>';
    }
  }
} else {
  // Form Not Submitted
?>
 
<form name="regitser" action="register.php" method="post">
Username: *<br /><input type="text" name="username" size="35" /><br />
Password: *<br /><input type="text" name="password" size="35" /><Br /><br />
Name: <br /><input type="text" name="name" size="35" /><br />
Email: *<br /><input type="text" name="email" size="35" /><br />
<input type="hidden" name="regform" value="1" /><br />
 
<?php 
  require_once('recaptchalib.php');
  $publickey = ""; // you got this from the signup page
  echo recaptcha_get_html($publickey);
?>
<br />
<input type="submit" name="submit" value="Register" />
</form>
 
<?php
}
?>
 
</div>
 
<?php
include('footer.php');
include ('endhtml.php');
?>
 

login.php

Code: Select all

 
<?php
session_start();
include("config.php");
include("inc.php");
?>
 
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en">
<head>
<meta name="Description" content="" />
<meta name="Keywords" content="" />
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1" />
<meta name="Robots" content="index,follow" />
<link rel="stylesheet" href="" type="text/css" />
<title>Login</title>
</head>
 
<?php
include('header.php');
include('leftbar.php');
include('rightbar.php');
?>
 
<div id="main">
<a name="TemplateInfo"></a>
<h1>Login</h1>
 
<?php
if (isset($_POST['Submit'])) {
  require_once('recaptchalib.php');
  $privatekey = "";
  $resp = recaptcha_check_answer ($privatekey,
  $_SERVER["REMOTE_ADDR"],
  $_POST["recaptcha_challenge_field"],
  $_POST["recaptcha_response_field"]);
 
  if (!$resp->is_valid) {
    die ("The reCAPTCHA wasn't entered correctly. Go back and try it again." .
    "(reCAPTCHA said: " . $resp->error . ")");
  } else {
 
    // ReCaptcha Code Entered Correct
    // Validate Username
    if ($_POST['username'] != "") {
      $username = filter_var($_POST['username'], FILTER_SANITIZE_STRING);
      if ($username == "") {
        $errors .= 'Please enter a valid username.<br/><br/>';
      }
    } else {
      $errors .= 'Please enter your a username.<br/>';
    }
 
    // Validate Password
    if ($_POST['password'] != "") {
      $password = md5($_POST['password']);
    } else {
      $errors .= 'Please enter your a password.<br/>';
    }
 
    // Check For Errors
    if (!$errors) {
      $query = mysql_query("select * from user where username='$username'") or die(mysql_error());
      $rows = mysql_fetch_array($query);
      if(($rows["username"] == $username) && ($rows["password"] == $password)) {
        $_SESSION['user'] = $username;
        echo "Login sucessful";
      } else {
        echo "Login failed";
      }
    } else {
      echo '<div style="color: red">' . $errors . '<br/></div>';
    }
  }
} else {
?>
 
<form name="login" action="login.php" method="post">
Username: <br /><input type="text" name="username" size="35" /><br />
Password: <br /><input type="text" name="password" size="35" /><Br /><br />
 
<?php 
require_once('recaptchalib.php');
$publickey = ""; // you got this from the signup page
echo recaptcha_get_html($publickey);
?>
 
<br />
<input type="submit" name="Submit" value="Login" />
</form>
 
<?php
}
?>
 
</div>
 
<?php
include ('footer.php');
include ('endhtml.php');
?>
 
Thank you for your help.
User avatar
kaisellgren
DevNet Resident
Posts: 1675
Joined: Sat Jan 07, 2006 5:52 am
Location: Lahti, Finland.

Re: Is my script secure?

Post by kaisellgren »

Your registration outputs user supplied data without encoding it. It also places user supplied data into an SQL query without escaping. You should fix those. Your login also has this issue with SQL queries.

The registration puts length limits on the form, but never validates on the server-side. That does not make sense.
transpar3nt
Forum Newbie
Posts: 5
Joined: Sat Dec 12, 2009 8:37 pm
Location: Denver, CO

Re: Is my script secure?

Post by transpar3nt »

Hello,

The most critical security error here was stated by kaisellgren. You need to escape user-defined data before it ever touches the database. In your case using MySQL, use mysql_real_escape_string();

A good method for that to make sure that you cover all post data is to loop through them in one swoop...

Code: Select all

 
foreach($_POST as $field => $value) {
  $field = mysql_real_escape_string($value);
}
 
This will set each field's name as a variable name (so $_POST['username'] is now $username)... and each one is safe to insert into the database. I always like to trim them too, just add trim() to that loop so it cuts out extra spaces at the beginning and end of entries (frustrates users to find out their password has a space at the end accidentally).

EDIT: ... I just noticed you use a function called filter_var()... I assume this does the escaping I mentioned above, but the looping will require less code.


Also, for your password, you are hashing it using MD5. Although not a huge concern, MD5 has been found to be vulnerable to collision. It would be more secure to use sha1() instead, and salting the hash would prevent someone who has just access to the database to reverse it.

Code: Select all

 
// salt should be as random as possible
  $salt = '7FIgyB0ZlxsYhrhmrDn0uNDYrE4jbYtRko2oIVdb';
// $password var was set above in the loop, now to salt-hash it
  $password = sha1($password . $salt);
 
Make sure that you modify your database to accept the correct length of sha1 output, I think it's 40-characters.

In the spirit of being as secure as you can, it would be a good idea to keep the salt in a file that's outside the web directory and include it.

As you did not show us your inc.php and config.php files, I can't determine if your sanitize functions are any good.

I hope this helps, sorry for being so long-winded.

- transpar3nt
Post Reply