Nemcio/Grav
1
0
Fork 0
mirror of https://github.com/getgrav/grav.git synced 2026-03-16 01:21:07 +01:00
Code Issues Packages Projects Releases Activity

Fix broken nounce handling (#2121)

Browse Source 
* Remove deprecated "getNonceOldStyle" function

This commit removes the following functions:

- getNonceOldStyle
- generateNonceStringOldStyle

The functions have been replaced in newer versions of
grav. It seems to me that they only existed in order to make a
upgrade to a newer version of grav painless (i.e. accept both types of
nonce tokens). Nowadays, existing old style nonces are expired long
time ago so it should be save to delete the deprecated funtions.

* Fix caching of nonces in static class variable

Currently, the behavior of `getNonce` is broken because it saves the
generated nonce in an array and only use the $action as the
key. However, the generated nonce does not only depend on the $action,
but also on $plusOneTick.

* Fix broken "plusOneTick" for nonces

It looks to me that there is a bug in the current implemention of
verifyNonce. Here is an example:

- 2018-08-01 10:00: We respond to a request and generate a nonce. The
  current tick is at 35489

- 2018-08-01 10:05: We use the previously generated nonce to make
  another request. We compare the given nounce with a new generated
  one (based on the same tick). The result is exactly the same and the
  request succeeds.

- 2018-08-01 14:00: We're now one tick ahead. Remember: A day (24
  hours) is separated into two ticks (each 12 hours). A request comes
  in, we compare the given nounce with a newly generated one based on
  the current tick (now at 35490). They don't match (which is totally
  okay).

  If the comparison fails, we then compare the given nounce with a
  another, newly generated one. This time, we pass "plusOneTick", to
  the function, which increases the current tick by one. Our tick is
  now at 35491. We generate a nonce based on that tick and of course,
  it still does not match the given nonce.

  Instead of increasing the tick, we should rather decreasing it by
  one (i.e. use the previous tick). If the first comparison fails, we
  use the current tick (35490), decrease it by one (35489) and then
  compare it again. 35489 is the same tick as in the very first
  request.

This bug leads to a maximum life time of 12 hours for a nonce and in
worst case only a few seconds (!)

I would like to prove the bug with an unit test but I'm too unexperienced
in PHP. Furthermore it seems that we need some kind of library which
is able to mock builtin functions (like "time"). Maybe
<https://github.com/Codeception/AspectMock> would be a good canditate?
This commit is contained in:
Jascha Geerds
2018-08-10 00:05:24 +02:00
committed by Andy Miller
parent c84983ad5b
commit 63161e62a2
1 changed files with 13 additions and 59 deletions
Download Patch File Download Diff File Expand all files Collapse all files

72
system/src/Grav/Common/Utils.php
View File

@@ -705,11 +705,11 @@ abstract class Utils
* with reverse proxy setups.
*
* @param string $action
* @param bool $plusOneTick if true, generates the token for the next tick (the next 12 hours)
* @param bool $previousTick if true, generates the token for the previous tick (the previous 12 hours)
*
* @return string the nonce string
*/
private static function generateNonceString($action, $plusOneTick = false)
private static function generateNonceString($action, $previousTick = false)
{
$username = '';
if (isset(Grav::instance()['user'])) {
@@ -720,29 +720,8 @@ abstract class Utils
$token = session_id();
$i = self::nonceTick();
if ($plusOneTick) {
$i++;
}
return ($i . '|' . $action . '|' . $username . '|' . $token . '|' . Grav::instance()['config']->get('security.salt'));
}
//Added in version 1.0.8 to ensure that existing nonces are not broken.
private static function generateNonceStringOldStyle($action, $plusOneTick = false)
{
if (isset(Grav::instance()['user'])) {
$user = Grav::instance()['user'];
$username = $user->username;
if (isset($_SERVER['REMOTE_ADDR'])) {
$username .= $_SERVER['REMOTE_ADDR'];
}
} else {
$username = isset($_SERVER['REMOTE_ADDR']) ? $_SERVER['REMOTE_ADDR'] : '';
}
$token = session_id();
$i = self::nonceTick();
if ($plusOneTick) {
$i++;
if ($previousTick) {
$i--;
}
return ($i . '|' . $action . '|' . $username . '|' . $token . '|' . Grav::instance()['config']->get('security.salt'));
@@ -768,33 +747,20 @@ abstract class Utils
* action is the same for 12 hours.
*
* @param string $action the action the nonce is tied to (e.g. save-user-admin or move-page-homepage)
* @param bool $plusOneTick if true, generates the token for the next tick (the next 12 hours)
* @param bool $previousTick if true, generates the token for the previous tick (the previous 12 hours)
*
* @return string the nonce
*/
public static function getNonce($action, $plusOneTick = false)
public static function getNonce($action, $previousTick = false)
{
// Don't regenerate this again if not needed
if (isset(static::$nonces[$action])) {
return static::$nonces[$action];
if (isset(static::$nonces[$action][$previousTick])) {
return static::$nonces[$action][$previousTick];
}
$nonce = md5(self::generateNonceString($action, $plusOneTick));
static::$nonces[$action] = $nonce;
$nonce = md5(self::generateNonceString($action, $previousTick));
static::$nonces[$action][$previousTick] = $nonce;
return static::$nonces[$action];
}
//Added in version 1.0.8 to ensure that existing nonces are not broken.
public static function getNonceOldStyle($action, $plusOneTick = false)
{
// Don't regenerate this again if not needed
if (isset(static::$nonces[$action])) {
return static::$nonces[$action];
}
$nonce = md5(self::generateNonceStringOldStyle($action, $plusOneTick));
static::$nonces[$action] = $nonce;
return static::$nonces[$action];
return static::$nonces[$action][$previousTick];
}
/**
@@ -818,20 +784,8 @@ abstract class Utils
}
//Nonce generated 12-24 hours ago
$plusOneTick = true;
if ($nonce === self::getNonce($action, $plusOneTick)) {
return true;
}
//Added in version 1.0.8 to ensure that existing nonces are not broken.
//Nonce generated 0-12 hours ago
if ($nonce === self::getNonceOldStyle($action)) {
return true;
}
//Nonce generated 12-24 hours ago
$plusOneTick = true;
if ($nonce === self::getNonceOldStyle($action, $plusOneTick)) {
$previousTick = true;
if ($nonce === self::getNonce($action, $previousTick)) {
return true;
}