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\$
  • 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 Jan 30 '14 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 Jan 30 '14 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\$ – danielsmile Jan 30 '14 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\$ – danielsmile Jan 31 '14 at 9:05
  • \$\begingroup\$ Do not invent your own hashing scheme \$\endgroup\$ – Ronni Skansing Feb 20 '14 at 17:43
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\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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