Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
function RegisterN() {
if($_SERVER['REQUEST_METHOD'] == 'POST'){
    if (isset($_POST["password"],$_POST["password1"]) && $_POST["password"]!=$_POST["password1"]) { # Check if password and password1 are same
    echo "Your password confirmation is not equal to your password or it missing.";
        } else if(!$_POST["email"]){ # Check if email is present
        echo "Please enter your email."; 
            } else if (!$_POST["password"]){ # Check if password is present again
            echo "Please enter your password";
                } else if (!$_POST["password1"]){ # Check if password1 is present again
                echo "Please enter your second password";
    } else {
     if (strlen ($_POST["password"])>25 || strlen ($_POST["password"])<6) # check password length max 25, min 6
        {
          echo "Password must be between 6 and 25 characters";
        } else {
        $dbh = new Database(); # Open database
        $dbh->setAttribute(PDO::ATTR_EMULATE_PREPARES, false); # Small SQL injection security 

            $stmt = $dbh->prepare('SELECT email FROM members WHERE email=?'); # Check if email exists
            $stmt->bindParam(1, $_POST['email'], PDO::PARAM_INT);
            $stmt->execute();
            $rows = $stmt->fetchAll(PDO::FETCH_ASSOC); 

            if( ! $rows)
            { # No email found - moving on - If all set - let's register :)
                $email = $_POST['email'];
                $password = $_POST['password'];

                $hash_cost_log2 = 8;
                $hash_portable = FALSE;

                require("includes/PasswordHash.php"); # Hash password lib

                $hasher = new PasswordHash($hash_cost_log2, $hash_portable); # Hashing the password
                $hash = $hasher->HashPassword($password);
                if (strlen($hash) < 20)
                    fail('Failed to hash new password');
                unset($hasher);

                    $STH =$dbh->prepare("INSERT INTO members (email,password) VALUES (:email, :password)"); # Registering new user
                    $STH->bindParam(':email', $email);
                    $STH->bindParam(':password', $hash);
                    $STH->execute();

    echo "Account created successfully.";
    header('Refresh: 2; url=index.php'); # Redirect to index.php in 2 seconds

        } else { # email found - closing database - register fail
            $dbh=null;
            echo('Email already exists.'); 
            }
            $dbh=null; # Closing database after register
            }
            }
} else { 
echo"Invalid Token."; # If user access the page without POST
} }

I want to know if I can make it more secure. Before I made the script, I checked other scripts to see how they work, and what security measures they use.

What the script does:

  • Check if request comes from POST

  • Check if all fields ware filled ( It's being checked by JS as well, but that can be "cracked" )

  • Check if email already exists

  • Hash password

  • Finally register

In addition, I saw that other scripts are far smaller than mine. Does that happen because they use frameworks?

Update : OOP implemented ( http://pastebin.com/0DKVxrDK ) for check email and register

share|improve this question
    
What is the PasswordHash class you're using? It seems you're not salting the hash thus making it irresponsibly weak, but I can't know for sure without having more information. –  amon Feb 2 at 11:30
    
I'm using PasswordHash from Openwall , openwall.com/phpass ( I followed openwall.com/articles/PHP-Users-Passwords ) –  rgerculy Feb 2 at 11:33

2 Answers 2

up vote 6 down vote accepted

Here are my recommendation,

  • you could make sure users password have a minimum length. 8 alphanumeric characters is ideal If a users password is 1 char, it can easily be broken no matter how good the salt is
  • move you database connection and setup into a different class: In the database class, create two separate functions.

    • Does Email Exist and Return boolean
    • Create users returning a new user object

Database Connection Class uses the singleton design pattern. We only need 1 connection object to exist when our application is running. The singleton pattern is a design pattern that restricts the instantiation of a class to one object.

class Database
{
    private static $dbh = null;
    //TODO you should move this inside config file
    private $host = 'mysql:host=localhost;dbname=test';
    private $pwd  = '';
    private $user = '';


    public function __construct()
    {
        try
        {
            Database::$dbh = new PDO($this->host, $this->user, $this->pwd);
            Database::$dbh->setAttribute( PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION );
        }
        catch (PDOException $e)
        {
            echo "Connection Error: " . $e->getMessage();
        }
    }

    public static function getInstance()
    {
        if (Database::$dbh === null) {
            new Database();
        }
        return Database::$dbh;
    }
}

//all of our sql code should be in a centralized location for easy updating
class SQLPreparedQuery {
    public static $EMAIL_EXIST = "SELECT email FROM user WHERE email=? LIMIT 1";
    public static $USER_ADD = "INSET INTO user (username, email, first, last) VALUES(?,?,?,?)";
}

require_once './Database.php';
require_once './SQLPreparedQuery.php';
class UserModel {
    public function doesEmailExist($email) {
        $doesEmailExist = false;
        try {
            $dbh = Database::getInstance();

            $stmt = $dbh->prepare(SQLPreparedQuery::$EMAIL_EXIST);
            $stmt->bindParam(1, $email, PDO::PARAM_INT);
            $stmt->execute();
            $result = $stmt->fetchAll(PDO::FETCH_ASSOC);
            $doesEmailExist = (empty($result) ? false : true);
        }
        catch (PDOException $ex) { 
            echo "An Error occured!" . $ex;
        }
        return $doesEmailExist;
    }

//USAGE
//This will return true if it exist and false if it doesn't

$userModel = new UserModel();
$status = $userModel->doesEmailExist('james@example.com');

CREATE TABLE `user` (
  `iduser` int(11) NOT NULL AUTO_INCREMENT,
  `first` varchar(45) DEFAULT NULL,
  `last` varchar(45) DEFAULT NULL,
  `email` varchar(45) NOT NULL,
  PRIMARY KEY (`iduser`),
  UNIQUE KEY `email_UNIQUE` (`email`)
) ENGINE=InnoDB AUTO_INCREMENT=2 DEFAULT CHARSET=latin1$$

INSERT INTO `test`.`user` (`first`, `last`, `email`) VALUES ('james', 'brown', 'james@example.com');

You should always add UNIQUE constraints to usernames and email address to ensure uniqueness in your database.

share|improve this answer
    
Password check was added and tested. Thank you. About the other 3 recommendations I'm a little lost.. but I'll try to apply them asap. –  rgerculy Feb 2 at 13:03
1  
If you make your code modular (each function does one thing only/ DRY - Don't repeat your self), you can reuse that function later on. For example, your current implementation you check if the email exist and insert your new user inside the same RegisterN function. Why not break that code block up into two different function so you can later reuse. It makes it easier to make changes and update your code in the future. –  James Feb 2 at 13:21
    
Oke, since yesterday I was looking for a solution regarding to DRY, however I can't find the right path. I tried this but didn't work ( Don't blame me, still try to learn this new stuff ) pastebin.com/XRf5pV99 –  rgerculy Feb 3 at 15:17
1  
@rgerculy OOP can be difficult. I will post an example once i get off. –  James Feb 3 at 16:10
    
I made some research and found this : stackoverflow.com/questions/3200155/… . Is that what I should use ? If so for me would be : pastebin.com/y892My4A .... ? –  rgerculy Feb 3 at 19:58
  • Hash password

I would recommend reading my "Protip" about secure passwords: https://coderwall.com/p/nnyida - I don't know the class you are using, but mine is just few lines of self-explaining code, so you know what you use.

In addition, I saw that other scripts are far smaller than my. That happens because they use frameworks?

Frameworks simply make your code easier to write, read and use. They will give your code some structure, and share some best-practices, so you are not reinventing wheel.

share|improve this answer
    
Your function you link to uses the function crypt() which uses an internal "DSE based hashing algorithm" which should not be considered secure for password hashing. See this question for a better list of hashing functions which are aprropriate. –  Bobby Feb 4 at 12:44

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.