Skip to main content
Code Review

Return to Answer

Misread the question
Source Link
200_success
  • 145.6k
  • 22
  • 190
  • 479

Security practices and correctness

I don't understand the workflow of this mini-application. It appears that anybody can reset any user's password just by knowing the victim's username. The passres nonce does nothing to help you confirm the requestor's identity — typically the secret link is e-mailed to the requestor to at least ascertain that the requestor has control of the user's registered e-mail address. As it stands, you're just redirecting the requestor to the reset.php page with the nonce, which merely provides some protection against cross-site request forgery for the reset.php page. If you were truly worried about CSRF attacks, you would be better off implementing a general mechanism to protect the POST handlers for all forms on the site.

It's probably a good idea to keep an audit log of the times and client IP addresses for all attempted and completed password changes.

<?php echo $us ?> and <?php echo (isset($_GET['er'])) ? $_GET['er'] : ''; ?> are vulnerable to cross-site scripting (or HTML injection) attacks. You should take care to call htmlspecialchars(...) whenever emitting HTML.

Instead of <input style="display: none;" ...>, you should use <input type="hidden" ...>, which is more straightforward. In fact, some browsers have been known to not submit those display: none form fields at all, due to an implementation bug or misinterpretation of the CSS specification.

Salted MD5 and SHA hashes are no longer considered good password-storage mechanisms. If you have PHP ≥ 5.5.0, you should use password_hash(), which uses the bcrypt algorithm.

Implementation

There is also a lot of related code between resetpass.php and reset.php, such as the passres generator and verifier, that should be extracted into functions and placed in a file that is included by both pages.

The indentation in reset.php is very deep. Keeping in mind that this usually one way to succeed but many ways to fail, it would help to invert your conditions such that the error handler comes first.

if (query->num_rows < 1) {
 header('HTTP/1.0 404 Not Found');
 exit();
}
$row = $query->fetch_assoc();
if (!isset($row['passres'], $row['passexp']) ||
 hash('sha256', $key) != $row['passres']) {
 header('HTTP/1.0 404 Not Found');
 exit();
} elsif ($row['passexp'] <= time()) {
 echo 'This link has expired!';
 ...
 exit();
}
$pass = ...;
...

Security practices and correctness

I don't understand the workflow of this mini-application. It appears that anybody can reset any user's password just by knowing the victim's username. The passres nonce does nothing to help you confirm the requestor's identity — typically the secret link is e-mailed to the requestor to at least ascertain that the requestor has control of the user's registered e-mail address. As it stands, you're just redirecting the requestor to the reset.php page with the nonce, which merely provides some protection against cross-site request forgery for the reset.php page. If you were truly worried about CSRF attacks, you would be better off implementing a general mechanism to protect the POST handlers for all forms on the site.

It's probably a good idea to keep an audit log of the times and client IP addresses for all attempted and completed password changes.

<?php echo $us ?> and <?php echo (isset($_GET['er'])) ? $_GET['er'] : ''; ?> are vulnerable to cross-site scripting (or HTML injection) attacks. You should take care to call htmlspecialchars(...) whenever emitting HTML.

Instead of <input style="display: none;" ...>, you should use <input type="hidden" ...>, which is more straightforward. In fact, some browsers have been known to not submit those display: none form fields at all, due to an implementation bug or misinterpretation of the CSS specification.

Salted MD5 and SHA hashes are no longer considered good password-storage mechanisms. If you have PHP ≥ 5.5.0, you should use password_hash(), which uses the bcrypt algorithm.

Implementation

There is also a lot of related code between resetpass.php and reset.php, such as the passres generator and verifier, that should be extracted into functions and placed in a file that is included by both pages.

The indentation in reset.php is very deep. Keeping in mind that this usually one way to succeed but many ways to fail, it would help to invert your conditions such that the error handler comes first.

if (query->num_rows < 1) {
 header('HTTP/1.0 404 Not Found');
 exit();
}
$row = $query->fetch_assoc();
if (!isset($row['passres'], $row['passexp']) ||
 hash('sha256', $key) != $row['passres']) {
 header('HTTP/1.0 404 Not Found');
 exit();
} elsif ($row['passexp'] <= time()) {
 echo 'This link has expired!';
 ...
 exit();
}
$pass = ...;
...

Security practices and correctness

It's probably a good idea to keep an audit log of the times and client IP addresses for all attempted and completed password changes.

<?php echo $us ?> and <?php echo (isset($_GET['er'])) ? $_GET['er'] : ''; ?> are vulnerable to cross-site scripting (or HTML injection) attacks. You should take care to call htmlspecialchars(...) whenever emitting HTML.

Instead of <input style="display: none;" ...>, you should use <input type="hidden" ...>, which is more straightforward. In fact, some browsers have been known to not submit those display: none form fields at all, due to an implementation bug or misinterpretation of the CSS specification.

Salted MD5 and SHA hashes are no longer considered good password-storage mechanisms. If you have PHP ≥ 5.5.0, you should use password_hash(), which uses the bcrypt algorithm.

Implementation

There is also a lot of related code between resetpass.php and reset.php, such as the passres generator and verifier, that should be extracted into functions and placed in a file that is included by both pages.

The indentation in reset.php is very deep. Keeping in mind that this usually one way to succeed but many ways to fail, it would help to invert your conditions such that the error handler comes first.

if (query->num_rows < 1) {
 header('HTTP/1.0 404 Not Found');
 exit();
}
$row = $query->fetch_assoc();
if (!isset($row['passres'], $row['passexp']) ||
 hash('sha256', $key) != $row['passres']) {
 header('HTTP/1.0 404 Not Found');
 exit();
} elsif ($row['passexp'] <= time()) {
 echo 'This link has expired!';
 ...
 exit();
}
$pass = ...;
...
Source Link
200_success
  • 145.6k
  • 22
  • 190
  • 479

Security practices and correctness

I don't understand the workflow of this mini-application. It appears that anybody can reset any user's password just by knowing the victim's username. The passres nonce does nothing to help you confirm the requestor's identity — typically the secret link is e-mailed to the requestor to at least ascertain that the requestor has control of the user's registered e-mail address. As it stands, you're just redirecting the requestor to the reset.php page with the nonce, which merely provides some protection against cross-site request forgery for the reset.php page. If you were truly worried about CSRF attacks, you would be better off implementing a general mechanism to protect the POST handlers for all forms on the site.

It's probably a good idea to keep an audit log of the times and client IP addresses for all attempted and completed password changes.

<?php echo $us ?> and <?php echo (isset($_GET['er'])) ? $_GET['er'] : ''; ?> are vulnerable to cross-site scripting (or HTML injection) attacks. You should take care to call htmlspecialchars(...) whenever emitting HTML.

Instead of <input style="display: none;" ...>, you should use <input type="hidden" ...>, which is more straightforward. In fact, some browsers have been known to not submit those display: none form fields at all, due to an implementation bug or misinterpretation of the CSS specification.

Salted MD5 and SHA hashes are no longer considered good password-storage mechanisms. If you have PHP ≥ 5.5.0, you should use password_hash(), which uses the bcrypt algorithm.

Implementation

There is also a lot of related code between resetpass.php and reset.php, such as the passres generator and verifier, that should be extracted into functions and placed in a file that is included by both pages.

The indentation in reset.php is very deep. Keeping in mind that this usually one way to succeed but many ways to fail, it would help to invert your conditions such that the error handler comes first.

if (query->num_rows < 1) {
 header('HTTP/1.0 404 Not Found');
 exit();
}
$row = $query->fetch_assoc();
if (!isset($row['passres'], $row['passexp']) ||
 hash('sha256', $key) != $row['passres']) {
 header('HTTP/1.0 404 Not Found');
 exit();
} elsif ($row['passexp'] <= time()) {
 echo 'This link has expired!';
 ...
 exit();
}
$pass = ...;
...
default

AltStyle によって変換されたページ (->オリジナル) /