From c644d76d28e697c87165e8c5fc12d7bfa5ff3475 Mon Sep 17 00:00:00 2001 From: Hypolite Petovan Date: Sun, 17 Jan 2021 17:30:18 -0500 Subject: [PATCH] Allow setting arbitrary keys in the cookie array --- src/Model/User/Cookie.php | 206 ++++++++++++++++------------ src/Security/Authentication.php | 34 +++-- tests/Util/StaticCookie.php | 14 +- tests/src/Model/User/CookieTest.php | 89 ++++-------- 4 files changed, 171 insertions(+), 172 deletions(-) diff --git a/src/Model/User/Cookie.php b/src/Model/User/Cookie.php index cda814e69..4b39ab7eb 100644 --- a/src/Model/User/Cookie.php +++ b/src/Model/User/Cookie.php @@ -41,126 +41,118 @@ class Cookie const HTTPONLY = true; /** @var string The remote address of this node */ - private $remoteAddr = '0.0.0.0'; + private $remoteAddr; /** @var bool True, if the connection is ssl enabled */ - private $sslEnabled = false; + private $sslEnabled; /** @var string The private key of this Friendica node */ private $sitePrivateKey; /** @var int The default cookie lifetime */ - private $lifetime = self::DEFAULT_EXPIRE * 24 * 60 * 60; - /** @var array The $_COOKIE array */ - private $cookie; + private $lifetime; + /** @var array The Friendica cookie data array */ + private $data; - public function __construct(IConfig $config, App\BaseURL $baseURL, array $server = [], array $cookie = []) + /** + * @param IConfig $config + * @param App\BaseURL $baseURL + * @param array $SERVER The $_SERVER array + * @param array $COOKIE The $_COOKIE array + */ + public function __construct(IConfig $config, App\BaseURL $baseURL, array $SERVER = [], array $COOKIE = []) { - if (!empty($server['REMOTE_ADDR'])) { - $this->remoteAddr = $server['REMOTE_ADDR']; - } - $this->sslEnabled = $baseURL->getSSLPolicy() === App\BaseURL::SSL_POLICY_FULL; $this->sitePrivateKey = $config->get('system', 'site_prvkey'); $authCookieDays = $config->get('system', 'auth_cookie_lifetime', self::DEFAULT_EXPIRE); $this->lifetime = $authCookieDays * 24 * 60 * 60; - $this->cookie = $cookie; + + $this->remoteAddr = ($SERVER['REMOTE_ADDR'] ?? null) ?: '0.0.0.0'; + + $this->data = json_decode($COOKIE[self::NAME] ?? '[]', true) ?: []; } /** - * Checks if the Friendica cookie is set for a user - * - * @param string $hash The cookie hash - * @param string $password The user password - * @param string $privateKey The private Key of the user - * - * @return boolean True, if the cookie is set + * Returns the value for a key of the Friendica cookie * + * @param string $key + * @param mixed $default + * @return mixed|null The value for the provided cookie key */ - public function check(string $hash, string $password, string $privateKey) + public function get(string $key, $default = null) { - return hash_equals( - $this->getHash($password, $privateKey), - $hash - ); + return $this->data[$key] ?? $default; } /** - * Set the Friendica cookie for a user + * Set a single cookie key value. + * Overwrites an existing value with the same key. * - * @param int $uid The user id - * @param string $password The user password - * @param string $privateKey The user private key - * @param int|null $seconds optional the seconds + * @param $key + * @param $value + * @return bool + */ + public function set($key, $value): bool + { + return $this->setMultiple([$key => $value]); + } + + /** + * Sets multiple cookie key values. + * Overwrites existing values with the same key. + * + * @param array $values + * @return bool + */ + public function setMultiple(array $values): bool + { + $this->data = $values + $this->data; + + return $this->send(); + } + + /** + * Remove a cookie key + * + * @param string $key + */ + public function unset(string $key) + { + if (isset($this->data[$key])) { + unset($this->data[$key]); + + $this->send(); + } + } + + /** + * Clears the Friendica cookie + */ + public function clear(): bool + { + $this->data = []; + // make sure cookie is deleted on browser close, as a security measure + return $this->setCookie( '', -3600, $this->sslEnabled); + } + + /** + * Send the cookie, should be called every time $this->data is changed or to refresh the cookie. * * @return bool */ - public function set(int $uid, string $password, string $privateKey, int $seconds = null) + public function send(): bool { - if (!isset($seconds)) { - $seconds = $this->lifetime + time(); - } elseif (isset($seconds) && $seconds != 0) { - $seconds = $seconds + time(); - } - - $value = json_encode([ - 'uid' => $uid, - 'hash' => $this->getHash($password, $privateKey), - 'ip' => $this->remoteAddr, - ]); - - return $this->setCookie(self::NAME, $value, $seconds, $this->sslEnabled); - } - - /** - * Returns the data of the Friendicas user cookie - * - * @return mixed|null The JSON data, null if not set - */ - public function getData() - { - // When the "Friendica" cookie is set, take the value to authenticate and renew the cookie. - if (isset($this->cookie[self::NAME])) { - $data = json_decode($this->cookie[self::NAME]); - if (!empty($data)) { - return $data; - } - } - - return null; - } - - /** - * Clears the Friendica cookie of this user after leaving the page - */ - public function clear() - { - // make sure cookie is deleted on browser close, as a security measure - return $this->setCookie(self::NAME, '', -3600, $this->sslEnabled); - } - - /** - * Calculate the hash that is needed for the Friendica cookie - * - * @param string $password The user password - * @param string $privateKey The private key of the user - * - * @return string Hashed data - */ - private function getHash(string $password, string $privateKey) - { - return hash_hmac( - 'sha256', - hash_hmac('sha256', $password, $privateKey), - $this->sitePrivateKey + return $this->setCookie( + json_encode(['ip' => $this->remoteAddr] + $this->data), + $this->lifetime + time(), + $this->sslEnabled ); } /** - * Send a cookie - protected, internal function for test-mocking possibility + * setcookie() wrapper: protected, internal function for test-mocking possibility * * @link https://php.net/manual/en/function.setcookie.php * - * @param string $name * @param string $value [optional] * @param int $expire [optional] * @param bool $secure [optional] @@ -168,9 +160,43 @@ class Cookie * @return bool If output exists prior to calling this function, * */ - protected function setCookie(string $name, string $value = null, int $expire = null, - bool $secure = null) + protected function setCookie(string $value = null, int $expire = null, + bool $secure = null): bool { - return setcookie($name, $value, $expire, self::PATH, self::DOMAIN, $secure, self::HTTPONLY); + return setcookie(self::NAME, $value, $expire, self::PATH, self::DOMAIN, $secure, self::HTTPONLY); + } + + /** + * Calculate a hash of a user's private data for storage in the cookie. + * Hashed twice, with the user's own private key first, then the node's private key second. + * + * @param string $privateData User private data + * @param string $privateKey User private key + * + * @return string Hashed data + */ + public function hashPrivateData(string $privateData, string $privateKey): string + { + return hash_hmac( + 'sha256', + hash_hmac('sha256', $privateData, $privateKey), + $this->sitePrivateKey + ); + } + + /** + * @param string $hash Hash from a cookie key value + * @param string $privateData User private data + * @param string $privateKey User private key + * + * @return boolean + * + */ + public function comparePrivateDataHash(string $hash, string $privateData, string $privateKey): bool + { + return hash_equals( + $this->hashPrivateData($privateData, $privateKey), + $hash + ); } } diff --git a/src/Security/Authentication.php b/src/Security/Authentication.php index 5c6624a33..089035bb7 100644 --- a/src/Security/Authentication.php +++ b/src/Security/Authentication.php @@ -33,6 +33,7 @@ use Friendica\Database\DBA; use Friendica\DI; use Friendica\Model\User; use Friendica\Network\HTTPException; +use Friendica\Repository\TwoFactor\TrustedBrowser; use Friendica\Util\DateTimeFormat; use Friendica\Util\Network; use Friendica\Util\Strings; @@ -100,16 +101,13 @@ class Authentication */ public function withSession(App $a) { - $data = $this->cookie->getData(); - // When the "Friendica" cookie is set, take the value to authenticate and renew the cookie. - if (isset($data->uid)) { - + if ($this->cookie->get('uid')) { $user = $this->dba->selectFirst( 'user', [], [ - 'uid' => $data->uid, + 'uid' => $this->cookie->get('uid'), 'blocked' => false, 'account_expired' => false, 'account_removed' => false, @@ -117,24 +115,25 @@ class Authentication ] ); if ($this->dba->isResult($user)) { - if (!$this->cookie->check($data->hash, + if (!$this->cookie->comparePrivateDataHash($this->cookie->get('hash'), $user['password'] ?? '', - $user['prvkey'] ?? '')) { - $this->logger->notice("Hash doesn't fit.", ['user' => $data->uid]); + $user['prvkey'] ?? '') + ) { + $this->logger->notice("Hash doesn't fit.", ['user' => $this->cookie->get('uid')]); $this->session->clear(); $this->cookie->clear(); $this->baseUrl->redirect(); } // Renew the cookie - $this->cookie->set($user['uid'], $user['password'], $user['prvkey']); + $this->cookie->send(); // Do the authentification if not done by now if (!$this->session->get('authenticated')) { $this->setForUser($a, $user); if ($this->config->get('system', 'paranoia')) { - $this->session->set('addr', $data->ip); + $this->session->set('addr', $this->cookie->get('ip')); } } } @@ -377,12 +376,15 @@ class Authentication */ if ($this->session->get('remember')) { $this->logger->info('Injecting cookie for remembered user ' . $user_record['nickname']); - $this->cookie->set($user_record['uid'], $user_record['password'], $user_record['prvkey']); + $this->cookie->setMultiple([ + 'uid' => $user_record['uid'], + 'hash' => $this->cookie->hashPrivateData($user_record['password'], $user_record['prvkey']), + ]); $this->session->remove('remember'); } } - $this->twoFactorCheck($user_record['uid'], $a); + $this->redirectForTwoFactorAuthentication($user_record['uid'], $a); if ($interactive) { if ($user_record['login_date'] <= DBA::NULL_DATETIME) { @@ -404,19 +406,23 @@ class Authentication } /** + * Decides whether to redirect the user to two-factor authentication. + * All return calls in this method skip two-factor authentication + * * @param int $uid The User Identified * @param App $a The Friendica Application context * * @throws HTTPException\ForbiddenException In case the two factor authentication is forbidden (e.g. for AJAX calls) + * @throws HTTPException\InternalServerErrorException */ - private function twoFactorCheck(int $uid, App $a) + private function redirectForTwoFactorAuthentication(int $uid, App $a) { // Check user setting, if 2FA disabled return if (!$this->pConfig->get($uid, '2fa', 'verified')) { return; } - // Check current path, if 2fa authentication module return + // Check current path, if public or 2fa module return if ($a->argc > 0 && in_array($a->argv[0], ['2fa', 'view', 'help', 'api', 'proxy', 'logout'])) { return; } diff --git a/tests/Util/StaticCookie.php b/tests/Util/StaticCookie.php index 6cfbdc3ab..4bee43185 100644 --- a/tests/Util/StaticCookie.php +++ b/tests/Util/StaticCookie.php @@ -35,22 +35,24 @@ class StaticCookie extends Cookie /** * Send a cookie - protected, internal function for test-mocking possibility - * @see Cookie::setCookie() * - * @link https://php.net/manual/en/function.setcookie.php - * - * @param string $name * @param string $value [optional] * @param int $expire [optional] * @param bool $secure [optional] + * @return bool * * @noinspection PhpMissingParentCallCommonInspection * + * @link https://php.net/manual/en/function.setcookie.php + * + * @see Cookie::setCookie() */ - protected function setCookie(string $name, string $value = null, int $expire = null, bool $secure = null) + protected function setCookie(string $value = null, int $expire = null, bool $secure = null): bool { - self::$_COOKIE[$name] = $value; + self::$_COOKIE[self::NAME] = $value; self::$_EXPIRE = $expire; + + return true; } public static function clearStatic() diff --git a/tests/src/Model/User/CookieTest.php b/tests/src/Model/User/CookieTest.php index e6e29048d..a69577e6e 100644 --- a/tests/src/Model/User/CookieTest.php +++ b/tests/src/Model/User/CookieTest.php @@ -128,30 +128,20 @@ class CookieTest extends MockedTest $cookie = new Cookie($this->config, $this->baseUrl, [], $cookieData); self::assertInstanceOf(Cookie::class, $cookie); - $assertData = $cookie->getData(); - - if (!$hasValues) { - self::assertEmpty($assertData); + if (isset($uid)) { + self::assertEquals($uid, $cookie->get('uid')); } else { - self::assertNotEmpty($assertData); - if (isset($uid)) { - self::assertObjectHasAttribute('uid', $assertData); - self::assertEquals($uid, $assertData->uid); - } else { - self::assertObjectNotHasAttribute('uid', $assertData); - } - if (isset($hash)) { - self::assertObjectHasAttribute('hash', $assertData); - self::assertEquals($hash, $assertData->hash); - } else { - self::assertObjectNotHasAttribute('hash', $assertData); - } - if (isset($ip)) { - self::assertObjectHasAttribute('ip', $assertData); - self::assertEquals($ip, $assertData->ip); - } else { - self::assertObjectNotHasAttribute('ip', $assertData); - } + self::assertNull($cookie->get('uid')); + } + if (isset($hash)) { + self::assertEquals($hash, $cookie->get('hash')); + } else { + self::assertNull($cookie->get('hash')); + } + if (isset($ip)) { + self::assertEquals($ip, $cookie->get('ip')); + } else { + self::assertNull($cookie->get('ip')); } } @@ -196,7 +186,7 @@ class CookieTest extends MockedTest $cookie = new Cookie($this->config, $this->baseUrl); self::assertInstanceOf(Cookie::class, $cookie); - self::assertEquals($assertTrue, $cookie->check($assertHash, $password, $userPrivateKey)); + self::assertEquals($assertTrue, $cookie->comparePrivateDataHash($assertHash, $password, $userPrivateKey)); } public function dataSet() @@ -210,7 +200,6 @@ class CookieTest extends MockedTest 'assertHash' => 'b657a15cfe7ed1f7289c9aa51af14a9a26c966f4ddd74e495fba103d8e872a39', 'remoteIp' => '0.0.0.0', 'serverArray' => [], - 'lifetime' => null, ], 'withServerArray' => [ 'serverKey' => 23, @@ -220,32 +209,11 @@ class CookieTest extends MockedTest 'assertHash' => 'b657a15cfe7ed1f7289c9aa51af14a9a26c966f4ddd74e495fba103d8e872a39', 'remoteIp' => '1.2.3.4', 'serverArray' => ['REMOTE_ADDR' => '1.2.3.4',], - 'lifetime' => null, - ], - 'withLifetime0' => [ - 'serverKey' => 23, - 'uid' => 0, - 'password' => '234', - 'privateKey' => '124', - 'assertHash' => 'b657a15cfe7ed1f7289c9aa51af14a9a26c966f4ddd74e495fba103d8e872a39', - 'remoteIp' => '1.2.3.4', - 'serverArray' => ['REMOTE_ADDR' => '1.2.3.4',], - 'lifetime' => 0, - ], - 'withLifetime' => [ - 'serverKey' => 23, - 'uid' => 0, - 'password' => '234', - 'privateKey' => '124', - 'assertHash' => 'b657a15cfe7ed1f7289c9aa51af14a9a26c966f4ddd74e495fba103d8e872a39', - 'remoteIp' => '1.2.3.4', - 'serverArray' => ['REMOTE_ADDR' => '1.2.3.4',], - 'lifetime' => 2 * 24 * 60 * 60, ], ]; } - public function assertCookie($uid, $hash, $remoteIp, $lifetime) + public function assertCookie($uid, $hash, $remoteIp) { self::assertArrayHasKey(Cookie::NAME, StaticCookie::$_COOKIE); @@ -258,11 +226,7 @@ class CookieTest extends MockedTest self::assertObjectHasAttribute('ip', $data); self::assertEquals($remoteIp, $data->ip); - if (isset($lifetime) && $lifetime !== 0) { - self::assertLessThanOrEqual(time() + $lifetime, StaticCookie::$_EXPIRE); - } else { - self::assertLessThanOrEqual(time() + Cookie::DEFAULT_EXPIRE * 24 * 60 * 60, StaticCookie::$_EXPIRE); - } + self::assertLessThanOrEqual(time() + Cookie::DEFAULT_EXPIRE * 24 * 60 * 60, StaticCookie::$_EXPIRE); } /** @@ -270,7 +234,7 @@ class CookieTest extends MockedTest * * @dataProvider dataSet */ - public function testSet($serverKey, $uid, $password, $privateKey, $assertHash, $remoteIp, $serverArray, $lifetime) + public function testSet($serverKey, $uid, $password, $privateKey, $assertHash, $remoteIp, $serverArray) { $this->baseUrl->shouldReceive('getSSLPolicy')->andReturn(true)->once(); $this->config->shouldReceive('get')->with('system', 'site_prvkey')->andReturn($serverKey)->once(); @@ -279,17 +243,20 @@ class CookieTest extends MockedTest $cookie = new StaticCookie($this->config, $this->baseUrl, $serverArray); self::assertInstanceOf(Cookie::class, $cookie); - $cookie->set($uid, $password, $privateKey, $lifetime); + $cookie->setMultiple([ + 'uid' => $uid, + 'hash' => $assertHash, + ]); - self::assertCookie($uid, $assertHash, $remoteIp, $lifetime); + self::assertCookie($uid, $assertHash, $remoteIp); } /** - * Test two different set() of the cookie class (first set is invalid) + * Test the set() method of the cookie class * * @dataProvider dataSet */ - public function testDoubleSet($serverKey, $uid, $password, $privateKey, $assertHash, $remoteIp, $serverArray, $lifetime) + public function testDoubleSet($serverKey, $uid, $password, $privateKey, $assertHash, $remoteIp, $serverArray) { $this->baseUrl->shouldReceive('getSSLPolicy')->andReturn(true)->once(); $this->config->shouldReceive('get')->with('system', 'site_prvkey')->andReturn($serverKey)->once(); @@ -298,12 +265,10 @@ class CookieTest extends MockedTest $cookie = new StaticCookie($this->config, $this->baseUrl, $serverArray); self::assertInstanceOf(Cookie::class, $cookie); - // Invalid set, should get overwritten - $cookie->set(-1, 'invalid', 'nothing', -234); + $cookie->set('uid', $uid); + $cookie->set('hash', $assertHash); - $cookie->set($uid, $password, $privateKey, $lifetime); - - self::assertCookie($uid, $assertHash, $remoteIp, $lifetime); + self::assertCookie($uid, $assertHash, $remoteIp); } /**