5
\$\begingroup\$

I've been building a system for inputting and monitoring shifts for casual staff, who work across multiple sites with the ability to generate accounting information.

I've had some help from Stack Overflow in building this project, as I had no prior knowledge of PHP or MySQL, and each time I posted some of my code I had comments about the lack of security.

In my system, sensitive information like salaries, work hours and things are protected by having different levels of user accounts. The code below is a snippet of code from my Admin Area allowing me to edit an account's userLevel and password.

if(isset($_POST['submit'])) {

    $editid = htmlentities($_POST['id'], ENT_QUOTES, 'UTF-8');
    $userLevel = htmlentities($_POST['userLevel'], ENT_QUOTES, 'UTF-8');

    if(!empty($_POST['password'])) {

        $password = password_hash("$_POST['password']", PASSWORD_DEFAULT)

        $sql = "UPDATE users SET userLevel = ?, password = ?, salt = ? WHERE id = ?";
        $stmt = $connection->prepare($sql);
        $stmt->bind_param('ssss', $userLevel, $password, $salt, $editid); 

    } else {

        $sql = "UPDATE users SET userLevel = ? WHERE id = ?";
        $stmt = $connection->prepare($sql);
        $stmt->bind_param('ss', $userLevel, $editid); 

    }

        $stmt->execute();
}

I specify UTF-8 charset at the top of my php files, and all raw output variables, posts, gets and sessions are wrapped in htmlentities tags. My account passwords are hashed and salted.

My system is only used locally and will never be used on the web (unless I post the code somewhere, which I could do to help other new people learn), but as I'm learning I was told I should get in the habit of writing secure code. So finally we get to the question.. Is the above code of a secure enough level for the web? If not, please offer suggestions.

\$\endgroup\$
5
  • 1
    \$\begingroup\$ You could add $stmt->execute(); outside the if statement so it doesn't have to be there twice. Although that's just being extremely nitpicky. \$\endgroup\$
    – Albzi
    Commented Jan 30, 2014 at 15:55
  • \$\begingroup\$ @BeatAlex, that is a very good point, I would upvote that review(answer) you should post with a little more explanation on it, even though it doesn't need one really. \$\endgroup\$
    – Malachi
    Commented Jan 30, 2014 at 16:48
  • \$\begingroup\$ Thanks for pointing it out, I've probably missed a bunch of little things like that as I've done a complete overhaul of code. Average reduction in lines of code per page is about 50% to give you some idea of how poor it was :p (many of it written in my first day(s) of php) \$\endgroup\$ Commented Jan 30, 2014 at 17:01
  • \$\begingroup\$ @MrLore I followed a tutorial and some stackoverflow posts, that said iterating hashes and injecting the password in each iteration was a good method of increasing the strength of the hash. stackoverflow.com/a/348140/3169285 \$\endgroup\$ Commented Jan 31, 2014 at 9:05
  • \$\begingroup\$ Do not invent your own hashing scheme \$\endgroup\$ Commented Feb 20, 2014 at 17:43

1 Answer 1

4
\$\begingroup\$

One thing to improve on (if your PHP version allows it), is to consider using PHP's built in password_hash() function. Coupled with password_verify(), it eliminates the need to salt and hash passwords manually. More information can be found in the documentation.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks, didn't know this function existed. Have a +1 :) \$\endgroup\$ Commented Feb 20, 2014 at 13:27
  • \$\begingroup\$ If your not one a new enough version, you can use this great resource. github.com/ircmaxell/password_compat \$\endgroup\$ Commented Feb 20, 2014 at 17:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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