From 63decefced9df1d16778431edaa9329b8774960e Mon Sep 17 00:00:00 2001 From: Leon Klingele Date: Thu, 27 Jul 2017 12:12:28 +0200 Subject: [PATCH] Store / retrieve temporary passwords from database This makes them much shorter. Downside: They're no longer stateless :( --- appinfo/database.xml | 41 +++++++ appinfo/info.xml | 4 +- appinfo/routes.php | 10 +- config/config.php.in | 4 - controller/apicontroller.php | 64 ----------- controller/temporarypasswordcontroller.php | 75 ++++++++++++ debug/debug.php | 10 -- doc/API.txt | 14 +-- errors/errorcodes.php | 1 + helper/helper.php | 6 +- js/settings-admin.js | 8 -- security/security.php | 128 +-------------------- security/temporarypasswordmanager.php | 127 ++++++++++++++++++++ templates/settings-admin.php | 8 -- 14 files changed, 258 insertions(+), 242 deletions(-) create mode 100644 appinfo/database.xml create mode 100644 controller/temporarypasswordcontroller.php create mode 100644 security/temporarypasswordmanager.php diff --git a/appinfo/database.xml b/appinfo/database.xml new file mode 100644 index 0000000..2e171c5 --- /dev/null +++ b/appinfo/database.xml @@ -0,0 +1,41 @@ + + + *dbname* + true + false + utf8 + + *dbprefix*spreedme_tps + + + id + integer + 1 + + + tp + text + true + 64 + + + userid + text + true + 64 + + + expiration + timestamp + true + + + spreedme_tps_tp + true + + tp + + + +
+ diff --git a/appinfo/info.xml b/appinfo/info.xml index ab56dcf..d3e7b4a 100644 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -10,7 +10,7 @@ https://github.com/strukturag/nextcloud-spreedme/blob/master/README.md https://github.com/strukturag/nextcloud-spreedme/blob/master/README.md - 0.3.10 + 0.3.11 SpreedME tools https://github.com/strukturag/nextcloud-spreedme/issues @@ -18,7 +18,7 @@ 174436 - + https://raw.githubusercontent.com/strukturag/nextcloud-spreedme/master/screenshots/appstore/conference.gif https://raw.githubusercontent.com/strukturag/nextcloud-spreedme/master/screenshots/appstore/presentation.png diff --git a/appinfo/routes.php b/appinfo/routes.php index 60b48a0..a61cf15 100644 --- a/appinfo/routes.php +++ b/appinfo/routes.php @@ -19,18 +19,18 @@ ['name' => 'page#debug', 'url' => '/admin/debug', 'verb' => 'GET'], ['name' => 'page#generate_temporary_password', 'url' => '/admin/tp', 'verb' => 'GET'], - // API + // General API ['name' => 'api#get_config', 'url' => '/api/v1/config', 'verb' => 'GET'], ['name' => 'api#get_user_config', 'url' => '/api/v1/user/config', 'verb' => 'GET'], ['name' => 'api#get_token', 'url' => '/api/v1/user/token', 'verb' => 'GET'], - ['name' => 'api#get_token_with_temporary_password', 'url' => '/api/v1/token/withtp', 'verb' => 'POST'], - ['name' => 'api#generate_temporary_password', 'url' => '/api/v1/admin/tp', 'verb' => 'POST'], ['name' => 'api#save_config', 'url' => '/api/v1/admin/config', 'verb' => 'PATCH'], ['name' => 'api#regenerate_shared_secret', 'url' => '/api/v1/admin/config/regenerate/sharedsecret', 'verb' => 'POST'], - ['name' => 'api#regenerate_temporary_password_signing_key', 'url' => '/api/v1/admin/config/regenerate/tp-key', 'verb' => 'POST'], ['name' => 'api#generate_spreed_webrtc_config', 'url' => '/api/v1/admin/config/generate/spreed-webrtc-config', 'verb' => 'POST'], ['name' => 'api#download_file', 'url' => '/api/v1/file/download', 'verb' => 'GET'], - // File Transfer + // Temporary password API + ['name' => 'temporarypassword#generate_temporary_password', 'url' => '/api/v1/admin/tp', 'verb' => 'POST'], + ['name' => 'temporarypassword#get_token_with_temporary_password', 'url' => '/api/v1/token/withtp', 'verb' => 'POST'], + // File transfer API ['name' => 'filesharing#uploadAndShare', 'url' => '/api/v1/filetransfers', 'verb' => 'POST'], ['name' => 'filesharing#listShares', 'url' => '/api/v1/filetransfers', 'verb' => 'GET'], ], diff --git a/config/config.php.in b/config/config.php.in index 357ea4d..a66859f 100644 --- a/config/config.php.in +++ b/config/config.php.in @@ -42,10 +42,6 @@ class Config { // You can generate such a temporary password at: /index.php/apps/spreedme/admin/tp (Nextcloud admin user account required) const OWNCLOUD_TEMPORARY_PASSWORD_LOGIN_ENABLED = false; - // If 'OWNCLOUD_TEMPORARY_PASSWORD_LOGIN_ENABLED' is set to true, you also have to provide a signing key here (64-character HEX string) - // Generate it using `xxd -ps -l 32 -c 32 /dev/random` (better) or `openssl rand -hex 32` - const OWNCLOUD_TEMPORARY_PASSWORD_SIGNING_KEY = 'f20e1b84781d80570fef6e2969f61ba91ccb56922398a45eXXXXXXXXXXXXXXXX'; - private function __construct() { } diff --git a/controller/apicontroller.php b/controller/apicontroller.php index 48323fc..226468c 100644 --- a/controller/apicontroller.php +++ b/controller/apicontroller.php @@ -87,51 +87,6 @@ public function getToken() { return new DataResponse($_response); } - /** - * @NoAdminRequired - * @NoCSRFRequired - */ - public function generateTemporaryPassword($userid, $expiration) { - $_response = array('success' => false); - // TODO(leon): Move this to user.php - if ($this->user->isSpreedMeAdmin() && $userid !== null && $expiration !== null) { - try { - $_response['tp'] = base64_encode(Security::generateTemporaryPassword($userid, $expiration)); - $_response['success'] = true; - } catch (\Exception $e) { - $_response['error'] = $e->getCode(); - } - } - - return new DataResponse($_response); - } - - /** - * @NoAdminRequired - * @NoCSRFRequired - * @PublicPage - */ - public function getTokenWithTemporaryPassword($tp) { - $tmp = base64_decode($tp, true); - // We support both base64 encoded and unencoded TPs - if ($tmp !== false) { - $tp = $tmp; - } - - $_response = array('success' => false); - if ($tp) { - try { - $token = Security::getSignedComboFromTemporaryPassword($tp); - $_response = array_merge($_response, $token); - $_response['success'] = true; - } catch (\Exception $e) { - $_response['error'] = $e->getCode(); - } - } - - return new DataResponse($_response); - } - public function saveConfig($config) { $allowedKeys = array( 'SPREED_WEBRTC_ORIGIN', @@ -155,12 +110,6 @@ public function saveConfig($config) { Helper::createServiceUserUnlessExists(); } break; - case 'OWNCLOUD_TEMPORARY_PASSWORD_LOGIN_ENABLED': - if ($value === 'true' && Helper::getDatabaseConfigValue('OWNCLOUD_TEMPORARY_PASSWORD_SIGNING_KEY') === '') { - // Also generate a 'Temporary Password signing key' - Security::regenerateTemporaryPasswordSigningKey(); - } - break; } } } @@ -186,19 +135,6 @@ public function regenerateSharedSecret() { return new DataResponse($_response); } - public function regenerateTemporaryPasswordSigningKey() { - // TODO(leon): Should we also allow Spreed.ME group admins to regenerate the signing key? - $_response = array('success' => false); - try { - Security::regenerateTemporaryPasswordSigningKey(); - $_response['success'] = true; - } catch (\Exception $e) { - $_response['error'] = $e->getCode(); - } - - return new DataResponse($_response); - } - public function generateSpreedWebRTCConfig() { $_response = array('success' => false); try { diff --git a/controller/temporarypasswordcontroller.php b/controller/temporarypasswordcontroller.php new file mode 100644 index 0000000..949b9dd --- /dev/null +++ b/controller/temporarypasswordcontroller.php @@ -0,0 +1,75 @@ + + * @copyright struktur AG 2016 + */ + +namespace OCA\SpreedME\Controller; + +use OCA\SpreedME\Security\TemporaryPasswordManager; +use OCA\SpreedME\User\User; +use OCP\AppFramework\Controller; +use OCP\AppFramework\Http\DataResponse; +use OCP\IDBConnection; +use OCP\IRequest; + +class TemporaryPasswordController extends Controller { + + private $user; + private $temporaryPasswordManager; + + public function __construct($appName, IRequest $request, IDBConnection $db) { + parent::__construct($appName, $request); + + if (!empty($userId)) { + $this->user = new User($userId); + } else { + $this->user = new User(); + } + + $this->temporaryPasswordManager = new TemporaryPasswordManager($db); + } + + /** + * @NoAdminRequired + */ + public function generateTemporaryPassword($userid, $expiration) { + $_response = array('success' => false); + if ($this->user->isSpreedMeAdmin() && $userid !== null && $expiration !== null) { + try { + $_response['tp'] = $this->temporaryPasswordManager->generateTemporaryPassword($userid, $expiration); + $_response['success'] = true; + } catch (\Exception $e) { + $_response['error'] = $e->getCode(); + } + } + + return new DataResponse($_response); + } + + /** + * @NoAdminRequired + * @NoCSRFRequired + * @PublicPage + */ + public function getTokenWithTemporaryPassword($tp) { + $_response = array('success' => false); + if ($tp) { + try { + $token = $this->temporaryPasswordManager->getSignedComboFromTemporaryPassword($tp); + $_response = array_merge($_response, $token); + $_response['success'] = true; + } catch (\Exception $e) { + $_response['error'] = $e->getCode(); + } + } + + return new DataResponse($_response); + } + +} diff --git a/debug/debug.php b/debug/debug.php index 1df2452..fddfcf2 100644 --- a/debug/debug.php +++ b/debug/debug.php @@ -57,16 +57,6 @@ private static function testOwncloudPhpConfigFile() { if (!ctype_xdigit(Helper::getConfigValue('SPREED_WEBRTC_SHAREDSECRET'))) { return 'Invalid SPREED_WEBRTC_SHAREDSECRET in config/config.php. Secret may only contain hexadecimal characters.'; } - - if (Helper::getConfigValue('OWNCLOUD_TEMPORARY_PASSWORD_LOGIN_ENABLED') === true) { - if (strlen(Helper::getConfigValue('OWNCLOUD_TEMPORARY_PASSWORD_SIGNING_KEY')) !== 64) { - return 'OWNCLOUD_TEMPORARY_PASSWORD_SIGNING_KEY in config/config.php must be a 64 character hexadecimal string.'; - } - - if (!ctype_xdigit(Helper::getConfigValue('OWNCLOUD_TEMPORARY_PASSWORD_SIGNING_KEY'))) { - return 'Invalid OWNCLOUD_TEMPORARY_PASSWORD_SIGNING_KEY in config/config.php. Key may only contain hexadecimal characters.'; - } - } } private static function testOwncloudJavascriptConfigFile() { diff --git a/doc/API.txt b/doc/API.txt index 96c0b7e..a333384 100644 --- a/doc/API.txt +++ b/doc/API.txt @@ -96,7 +96,7 @@ Available endpoints with request methods and content-type: Response 200: { "success": true, - "tp": "MTQ3MTk0ODUwMDpleHQvdGVzdC81N2JjMGFjMTgzNWQ2NS4xNzIwNTQ2NjoyOmlxYUphS2duYVRVdUhySHZjR1lkY0xmaVNxMk5ZMGR6OVZVSU5oOU1sMzg9" + "tp": "Sp4hgnQRXm" } @@ -144,18 +144,6 @@ Available endpoints with request methods and content-type: } - /api/v1/admin/config/regenerate/tp-key - - The admin/config/regenerate/tp-key endpoint generates and stores a new OWNCLOUD_TEMPORARY_PASSWORD_SIGNING_KEY. - This endpoint requires admin authentication + a valid Nextcloud CSRF requesttoken. - - POST application/x-www-form-urlencoded - Response 200: - { - "success": true - } - - /api/v1/filetransfers The filetransfers endpoint allows admins list existing and create new shares. diff --git a/errors/errorcodes.php b/errors/errorcodes.php index e862675..27a65f0 100644 --- a/errors/errorcodes.php +++ b/errors/errorcodes.php @@ -17,6 +17,7 @@ class ErrorCodes { const TEMPORARY_PASSWORD_NOT_ENABLED = 50101; const TEMPORARY_PASSWORD_INVALID = 50102; const TEMPORARY_PASSWORD_INVALID_USERID = 50103; + const TEMPORARY_PASSWORD_USERID_TOO_LONG = 50104; const DB_CONFIG_ERROR_CONFIG_PHP_EXISTS = 50201; const REMOTE_CONFIG_EMPTY = 50301; const REMOTE_CONFIG_INVALID_JSON = 50302; diff --git a/helper/helper.php b/helper/helper.php index 1e0ba31..ab17d48 100644 --- a/helper/helper.php +++ b/helper/helper.php @@ -224,9 +224,9 @@ public static function generateSpreedWebRTCConfig() { } $replace = array( '/webrtc/' => self::getDatabaseConfigValueOrDefault('SPREED_WEBRTC_BASEPATH'), - 'the-default-secret-do-not-keep-me' => Security::getRandomHexString(256 / 4), // 256 bit - 'the-default-encryption-block-key' => Security::getRandomHexString(256 / 4), // 256 bit - 'i-did-not-change-the-public-token-boo' => Security::getRandomHexString(256 / 4), // 256 bit + 'the-default-secret-do-not-keep-me' => Security::getRandomString(256 / 4), // 256 bit + 'the-default-encryption-block-key' => Security::getRandomString(256 / 4), // 256 bit + 'i-did-not-change-the-public-token-boo' => Security::getRandomString(256 / 4), // 256 bit '/absolute/path/to/nextcloud/apps/spreedme/extra' => self::getOwnAppPath() . 'extra', 'some-secret-do-not-keep' => self::getDatabaseConfigValue('SPREED_WEBRTC_SHAREDSECRET'), ); diff --git a/js/settings-admin.js b/js/settings-admin.js index 4999636..3a64d37 100644 --- a/js/settings-admin.js +++ b/js/settings-admin.js @@ -123,14 +123,6 @@ $(document).ready(function() { }); }); - $c.find('[name="REGENERATE_OWNCLOUD_TEMPORARY_PASSWORD_SIGNING_KEY"]').click(function(e) { - regenerateTemporaryPasswordSigningKey(function() { - $c.find('.OWNCLOUD_TEMPORARY_PASSWORD_SIGNING_KEY') - .removeClass('hidden'); - }, function(error) { - - }); - }); $c.find('[name="GENERATE_SPREED_WEBRTC_CONFIG"]').click(function(e) { generateSpreedWebRTCConfig(function(config) { $c.find('.SPREED_WEBRTC_CONFIG') diff --git a/security/security.php b/security/security.php index f0ddec6..02a2995 100644 --- a/security/security.php +++ b/security/security.php @@ -11,7 +11,6 @@ namespace OCA\SpreedME\Security; -use OCA\SpreedME\Errors\ErrorCodes; use OCA\SpreedME\Helper\Helper; use OCA\SpreedME\Settings\Settings; @@ -22,7 +21,6 @@ class Security { // Increase this with every major update of the signing process to invalidate old signatures const COMBO_VERSION = 2; - const TP_VERSION = 2; private function __construct() { @@ -65,136 +63,16 @@ public static function getSignedCombo($userid, $expiration = 0) { } } - private static function requireEnabledTemporaryPassword() { - if (Helper::getConfigValue('OWNCLOUD_TEMPORARY_PASSWORD_LOGIN_ENABLED') !== true) { - throw new \Exception('Temporary Passwords not enabled in config/config.php', ErrorCodes::TEMPORARY_PASSWORD_NOT_ENABLED); - } - } - - private static function decorateUserId($userid, $prefix) { - // Prefix userid with ext/ or int/ and append /uniqueid - $uniqueid = uniqid('', true); - return sprintf('%s/%s/%s', $prefix, $userid, $uniqueid); - } - - // TODO(leon): Refactor, this is shitty - public static function generateTemporaryPassword($userid, $expiration = 0, $version = self::TP_VERSION, $forValidation = false) { - if (!$forValidation) { - self::requireEnabledTemporaryPassword(); - } - - // Only prevent certain characters and decorate userid if we don't want to validate a given TP - if (!$forValidation) { - $disallowed = array(':', '/'); - foreach ($disallowed as $char) { - if (strpos($userid, $char) !== false) { - throw new \Exception('userid may not contain one of these symbols: ' . join(' or ', $disallowed), ErrorCodes::TEMPORARY_PASSWORD_INVALID_USERID); - } - } - - $userid = self::decorateUserId($userid, 'ext'); - } - - $key = Helper::getConfigValue('OWNCLOUD_TEMPORARY_PASSWORD_SIGNING_KEY'); - $max_age = 60 * 60 * 2; - - if ($expiration > 0) { - // Use a fixed expiration date - $signed_combo_array = self::getSignedUsercomboArray( - $version, - $userid, - $key, - false, - $expiration - ); - } else { - // Dynamically expire after x seconds - $signed_combo_array = self::getSignedUsercomboArray( - $version, - $userid, - $key, - $max_age - ); - } - - return $signed_combo_array['useridcombo'] . ':' . $signed_combo_array['secret']; - } - - public static function validateTemporaryPassword($tp) { - $split = explode(':', $tp); - - // The 4 parts: expiraton, userid, version, hmac (signature of previous 3 parts) - if (count($split) !== 4) { - // Invalid tp part length - return false; - } - - list($expiration, $userid, $version, $hmac) = $split; - - if (time() > $expiration) { - // Expired - return false; - } - - if (intval($version) !== self::TP_VERSION) { - // Unsupported / outdated version - return false; - } - - $calctp = self::generateTemporaryPassword($userid, $expiration, $version, true); // Set forValidation flag - if (self::constantTimeEquals($tp, $calctp) !== true) { - // Incorrect hmac - return false; - } - - return true; - } - - public static function getSignedComboFromTemporaryPassword($tp) { - self::requireEnabledTemporaryPassword(); - - if (self::validateTemporaryPassword($tp) !== true) { - throw new \Exception('Invalid Temporary Password', ErrorCodes::TEMPORARY_PASSWORD_INVALID); - } - - // TODO(leon): Do we need to split again? - $split = explode(':', $tp); - list($expiration, $userid, $version, $hmac) = $split; - - return self::getSignedCombo($userid); - } - - public static function getRandomHexString($length) { - return \OC::$server->getSecureRandom()->getMediumStrengthGenerator()->generate($length, '0123456789abcdef'); + public static function getRandomString($length, $charset = '0123456789abcdef') { + return \OC::$server->getSecureRandom()->getMediumStrengthGenerator()->generate($length, $charset); } public static function regenerateSharedSecret() { - $key = Security::getRandomHexString(256 / 4); // 256 bit + $key = Security::getRandomString(256 / 4); // 256 bit Helper::setDatabaseConfigValueIfEnabled('SPREED_WEBRTC_SHAREDSECRET', $key); return $key; } - public static function regenerateTemporaryPasswordSigningKey() { - $key = Security::getRandomHexString(256 / 4); // 256 bit - Helper::setDatabaseConfigValueIfEnabled('OWNCLOUD_TEMPORARY_PASSWORD_SIGNING_KEY', $key); - } - - public static function constantTimeEquals($a, $b) { - $alen = mb_strlen($a, '8bit'); - $blen = mb_strlen($b, '8bit'); - - if ($alen !== $blen) { - return false; - } - - $result = 0; - for ($i = 0; $i < $alen; $i++) { - $result |= (ord($a[$i]) ^ ord($b[$i])); - } - - return $result === 0; - } - public static function getAllowedIframeDomains() { $origin = Helper::getSpreedWebRtcOrigin(); diff --git a/security/temporarypasswordmanager.php b/security/temporarypasswordmanager.php new file mode 100644 index 0000000..53a431c --- /dev/null +++ b/security/temporarypasswordmanager.php @@ -0,0 +1,127 @@ + + * @copyright struktur AG 2016 + */ + +namespace OCA\SpreedME\Security; + +use OCA\SpreedME\Errors\ErrorCodes; +use OCA\SpreedME\Helper\Helper; +use OCA\SpreedME\Security\Security; +use OCP\IDBConnection; + +class TemporaryPasswordManager { + + private $db; + private $tableName = 'spreedme_tps'; + private $hashFuncName = 'sha256'; + private $maxUserLength = 64; // Keep in sync with database.xml + private $disallowedUserChars = array(':', '/'); + private $temporaryPasswordLength = 10; // ld(55^10) ≈ 58 bit of entropy + private $temporaryPasswordAllowedChars = 'abcdefghjkmnpqrstuvwxyzABCDEFGHJKMNPQRSTUVWXYZ123456789'; + + public function __construct(IDBConnection $db) { + $this->db = $db; + } + + // So we don't leak anything due to timing side channels + private function hashTemporaryPassword($tp) { + return hash($this->hashFuncName, $tp); + } + + private function getNewTemporaryPassword() { + return Security::getRandomString($this->temporaryPasswordLength, $this->temporaryPasswordAllowedChars); + } + + private function requireEnabledTemporaryPassword() { + if (Helper::getConfigValue('OWNCLOUD_TEMPORARY_PASSWORD_LOGIN_ENABLED') !== true) { + throw new \Exception('Temporary Passwords not enabled in config/config.php', ErrorCodes::TEMPORARY_PASSWORD_NOT_ENABLED); + } + } + + private function decorateUserId($userid, $prefix) { + // Prefix userid with ext/ or int/ and append /uniqueid + $uniqueid = uniqid('', true); + return sprintf('%s/%s/%s', $prefix, $userid, $uniqueid); + } + + private function getFormattedDate($date) { + return $date->format('Y-m-d H:i:s'); + } + + public function generateTemporaryPassword($userid, $expirationUnix) { + // Validate userid length + if (strlen($userid) > $this->maxUserLength) { + throw new \Exception('userid too long', ErrorCodes::TEMPORARY_PASSWORD_USERID_TOO_LONG); + } + + // Prevent certain characters in userid + foreach ($this->disallowedUserChars as $char) { + if (strpos($userid, $char) !== false) { + throw new \Exception('userid may not contain one of these symbols: ' . join(' or ', $this->disallowedUserChars), ErrorCodes::TEMPORARY_PASSWORD_INVALID_USERID); + } + } + + $expiration = new \DateTime(); + $expiration->setTimestamp($expirationUnix); + + // Generate TP and insert it to the DB in hashed form + // Repeat until we inserted an unique one + while (true) { + $tp = $this->getNewTemporaryPassword(); + $query = $this->db->getQueryBuilder(); + try { + $query->insert($this->tableName)->values(array( + 'tp' => $query->createNamedParameter($this->hashTemporaryPassword($tp)), + 'userid' => $query->createNamedParameter($userid), + 'expiration' => $query->createNamedParameter($this->getFormattedDate($expiration)), + ))->execute(); + return $tp; + } catch (UniqueConstraintViolationException $e) { + // Whoops, hash collision + // You are likely one of the first people to find a $this->hashFuncName collision + // Unfortunately I don't tell you about it ;) Fame = vanished + } + } + } + + // Returns false or the TP's decorated userid + public function validateTemporaryPassword($tp) { + $now = new \DateTime(); + $query = $this->db->getQueryBuilder(); + $useridField = 'userid'; + $query + ->select($useridField) + ->from($this->tableName) + ->where($query->expr()->eq('tp', $query->createNamedParameter($this->hashTemporaryPassword($tp)))) + ->andWhere($query->expr()->gte('expiration', $query->createNamedParameter($this->getFormattedDate($now)))) + ->execute(); + + $result = $query->execute(); + $user = false; + // TODO(leon): Why do we loop? + while ($row = $result->fetch()) { + $user = $this->decorateUserId($row[$useridField], 'ext'); + } + $result->closeCursor(); + return $user; + } + + public function getSignedComboFromTemporaryPassword($tp) { + $this->requireEnabledTemporaryPassword(); + + $userid = $this->validateTemporaryPassword($tp); + if ($this->validateTemporaryPassword($tp) === false) { + throw new \Exception('Invalid Temporary Password', ErrorCodes::TEMPORARY_PASSWORD_INVALID); + } + + return Security::getSignedCombo($userid); + } + +} diff --git a/templates/settings-admin.php b/templates/settings-admin.php index e9ba692..85a4f80 100644 --- a/templates/settings-admin.php +++ b/templates/settings-admin.php @@ -78,14 +78,6 @@ class="needs-confirmation" data-confirmation-message="Do you really want to gene />

- -

- - -