Skip to main content
fixed formatting
Source Link
tim
  • 25.1k
  • 3
  • 30
  • 76

Just from a quick look at your code, I found a couple of issues. I didn't look at it in-depth, so there may be more.

Security

if(!isset($block) && ($saltedPass == $us_pass || $password == "")){:

My password can either be correct or empty to login? That does not seem right.

hash('sha256'

Simple sha256 is not good enough, use bcrypt (this also has the advantage that it manages salts for you and that it uses a timing safe string compare).

$this->dbh->prepare("INSERT INTO {$this->dbtable}`{$this->dbtable}` (username`username`, password`password`, password_salt`password_salt`, $columns) VALUES(:username, :password, :passwordSalt, :$colVals)");

This is open to SQL injection for any additional values, either via $columns, but also via $colVals. You have the same problems in other functions such as updateUser.

$html='<form action="'.$_SERVER['PHP_SELF'].'" method="POST">'; "<input type='hidden' name='token' value='{$_GET['resetPassToken']}'/>";

This is both open to reflected XSS (You are using PHP_SELF in other places as well).

header("Location: $url", true, $status);

Always die after a redirect as clients do not have to follow them, meaning that the code below a redirect will still be executed.

mail($email, $subject, $body);

This is probably open to email injection.

ini_set("display_errors", "on");

Displaying errors is not a good idea. It may provide information for an attacker, and it is not really helpful for legitimate users.

Your cookies should be set as httpOnly.

Also, I did not see any CSRF protection. Did I miss it? If not, you should add it, at least to the login function (see here for an explanation).

Misc

  • Your code is too deeply nested. Especially in the function changePassword. Use early returns to reduce this a bit.
  • Some of your names could be better, for example remCook (don't shorten variable names).

Just from a quick look at your code, I found a couple of issues. I didn't look at it in-depth, so there may be more.

Security

if(!isset($block) && ($saltedPass == $us_pass || $password == "")){:

My password can either be correct or empty to login? That does not seem right.

hash('sha256'

Simple sha256 is not good enough, use bcrypt (this also has the advantage that it manages salts for you and that it uses a timing safe string compare).

$this->dbh->prepare("INSERT INTO {$this->dbtable} (username, password, password_salt, $columns) VALUES(:username, :password, :passwordSalt, :$colVals)");

This is open to SQL injection for any additional values, either via $columns, but also via $colVals. You have the same problems in other functions such as updateUser.

$html='<form action="'.$_SERVER['PHP_SELF'].'" method="POST">'; "<input type='hidden' name='token' value='{$_GET['resetPassToken']}'/>";

This is both open to reflected XSS (You are using PHP_SELF in other places as well).

header("Location: $url", true, $status);

Always die after a redirect as clients do not have to follow them, meaning that the code below a redirect will still be executed.

mail($email, $subject, $body);

This is probably open to email injection.

ini_set("display_errors", "on");

Displaying errors is not a good idea. It may provide information for an attacker, and it is not really helpful for legitimate users.

Your cookies should be set as httpOnly.

Also, I did not see any CSRF protection. Did I miss it? If not, you should add it, at least to the login function (see here for an explanation).

Misc

  • Your code is too deeply nested. Especially in the function changePassword. Use early returns to reduce this a bit.
  • Some of your names could be better, for example remCook (don't shorten variable names).

Just from a quick look at your code, I found a couple of issues. I didn't look at it in-depth, so there may be more.

Security

if(!isset($block) && ($saltedPass == $us_pass || $password == "")){:

My password can either be correct or empty to login? That does not seem right.

hash('sha256'

Simple sha256 is not good enough, use bcrypt (this also has the advantage that it manages salts for you and that it uses a timing safe string compare).

$this->dbh->prepare("INSERT INTO `{$this->dbtable}` (`username`, `password`, `password_salt`, $columns) VALUES(:username, :password, :passwordSalt, :$colVals)");

This is open to SQL injection for any additional values, either via $columns, but also via $colVals. You have the same problems in other functions such as updateUser.

$html='<form action="'.$_SERVER['PHP_SELF'].'" method="POST">'; "<input type='hidden' name='token' value='{$_GET['resetPassToken']}'/>";

This is both open to reflected XSS (You are using PHP_SELF in other places as well).

header("Location: $url", true, $status);

Always die after a redirect as clients do not have to follow them, meaning that the code below a redirect will still be executed.

mail($email, $subject, $body);

This is probably open to email injection.

ini_set("display_errors", "on");

Displaying errors is not a good idea. It may provide information for an attacker, and it is not really helpful for legitimate users.

Your cookies should be set as httpOnly.

Also, I did not see any CSRF protection. Did I miss it? If not, you should add it, at least to the login function (see here for an explanation).

Misc

  • Your code is too deeply nested. Especially in the function changePassword. Use early returns to reduce this a bit.
  • Some of your names could be better, for example remCook (don't shorten variable names).
Source Link
tim
  • 25.1k
  • 3
  • 30
  • 76

Just from a quick look at your code, I found a couple of issues. I didn't look at it in-depth, so there may be more.

Security

if(!isset($block) && ($saltedPass == $us_pass || $password == "")){:

My password can either be correct or empty to login? That does not seem right.

hash('sha256'

Simple sha256 is not good enough, use bcrypt (this also has the advantage that it manages salts for you and that it uses a timing safe string compare).

$this->dbh->prepare("INSERT INTO {$this->dbtable} (username, password, password_salt, $columns) VALUES(:username, :password, :passwordSalt, :$colVals)");

This is open to SQL injection for any additional values, either via $columns, but also via $colVals. You have the same problems in other functions such as updateUser.

$html='<form action="'.$_SERVER['PHP_SELF'].'" method="POST">'; "<input type='hidden' name='token' value='{$_GET['resetPassToken']}'/>";

This is both open to reflected XSS (You are using PHP_SELF in other places as well).

header("Location: $url", true, $status);

Always die after a redirect as clients do not have to follow them, meaning that the code below a redirect will still be executed.

mail($email, $subject, $body);

This is probably open to email injection.

ini_set("display_errors", "on");

Displaying errors is not a good idea. It may provide information for an attacker, and it is not really helpful for legitimate users.

Your cookies should be set as httpOnly.

Also, I did not see any CSRF protection. Did I miss it? If not, you should add it, at least to the login function (see here for an explanation).

Misc

  • Your code is too deeply nested. Especially in the function changePassword. Use early returns to reduce this a bit.
  • Some of your names could be better, for example remCook (don't shorten variable names).