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 = ...;
...
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 = ...;
...