diff --git a/docs/guides/actions.rst b/docs/guides/actions.rst index f460bf5bf05..f247452da10 100644 --- a/docs/guides/actions.rst +++ b/docs/guides/actions.rst @@ -425,3 +425,35 @@ You can also access the tokens from javascript: These are refreshed periodically so should always be up-to-date. +Security Tokens +=============== +On occasion we need to pass data through an untrusted party or generate an "unguessable token" based on some data. +The industry-standard `HMAC `_ algorithm is the right tool for this. +It allows us to verify that received data were generated by our site, and were not tampered with. Note that even +strong hash functions like SHA-2 should *not* be used directly for these tasks. + +Elgg provides ``elgg_generate_mac()`` and ``elgg_validate_mac()`` to generate and validate HMAC message authentication +codes that are unguessable without the site's private key. + +.. code:: php + + // generate a querystring such that $a and $b can't be altered + $a = 1234; + $b = "hello"; + $query = http_build_query([ + 'a' => $a, + 'b' => $b, + 'mac' => elgg_generate_mac("$a|$b"), + ]); + $url = "action/foo?$query"; + + + // validate the querystring + $a = get_input('a', '', false); + $b = get_input('b', '', false); + $mac = get_input('mac', '', false); + + if (elgg_validate_mac($mac, "$a|$b")) { + // $a and $b have not been altered + } + diff --git a/engine/classes/Elgg/ActionsService.php b/engine/classes/Elgg/ActionsService.php index 6f5d5268b5d..cd103f318c3 100644 --- a/engine/classes/Elgg/ActionsService.php +++ b/engine/classes/Elgg/ActionsService.php @@ -144,7 +144,7 @@ public function validateActionToken($visible_errors = true, $token = null, $ts = if (($token) && ($ts) && ($session_id)) { // generate token, check with input and forward if invalid - $required_token = generate_action_token($ts); + $required_token = $this->generateActionToken($ts); // Validate token $token_matches = _elgg_services()->crypto->areEqual($token, $required_token); @@ -248,7 +248,8 @@ public function gatekeeper($action) { } // let the validator send an appropriate msg - validate_action_token(); + $this->validateActionToken(); + } else if ($this->validateActionToken()) { return true; } @@ -261,16 +262,14 @@ public function gatekeeper($action) { * @access private */ public function generateActionToken($timestamp) { - $site_secret = _elgg_services()->siteSecret->get(); $session_id = _elgg_services()->session->getId(); - // Session token - $st = _elgg_services()->session->get('__elgg_session'); - - if ($session_id && $site_secret) { - return _elgg_services()->crypto->getHmac($timestamp . $session_id . $st, $site_secret, 'md5'); + if (!$session_id) { + return false; } - - return false; + + $session_token = _elgg_services()->session->get('__elgg_session'); + + return _elgg_services()->crypto->getHmac("$timestamp|$session_id|$session_token", '', 'md5'); } /** diff --git a/engine/classes/Elgg/Database/SiteSecret.php b/engine/classes/Elgg/Database/SiteSecret.php index c652dd0e12e..9c27c5f2248 100644 --- a/engine/classes/Elgg/Database/SiteSecret.php +++ b/engine/classes/Elgg/Database/SiteSecret.php @@ -11,6 +11,7 @@ * @since 1.10.0 */ class SiteSecret { + /** * Initialise the site secret (32 bytes: "z" to indicate format + 186-bit key in Base64 URL). * @@ -30,20 +31,38 @@ function init() { return false; } - + /** * Returns the site secret. * * Used to generate difficult to guess hashes for sessions and action tokens. * + * @param bool $raw If true, a binary key will be returned + * * @return string Site secret. * @access private */ - function get() { + function get($raw = false) { $secret = _elgg_services()->datalist->get('__site_secret__'); if (!$secret) { $secret = init_site_secret(); } + + if ($raw) { + // try to return binary key + if ($secret[0] === 'z') { + // new keys are "z" + base64URL + $base64 = strtr(substr($secret, 1), '-_', '+/'); + $key = base64_decode($base64); + if ($key !== false) { + // on failure, at least return string key :/ + return $key; + } + } else { + // old keys are hex + return hex2bin($secret); + } + } return $secret; } @@ -51,6 +70,10 @@ function get() { /** * Get the strength of the site secret * + * 1.8.17 added the ability to generate a new site secret in Base64URL encoding. To allow + * reliably distinguishing the old hex-encoded string, the first byte of the new strings are + * set to "z". This is small but acceptable loss of entropy. + * * @return string "strong", "moderate", or "weak" * @access private */ diff --git a/engine/classes/Elgg/Database/UsersTable.php b/engine/classes/Elgg/Database/UsersTable.php index 2df0877d166..d855f43ed7a 100644 --- a/engine/classes/Elgg/Database/UsersTable.php +++ b/engine/classes/Elgg/Database/UsersTable.php @@ -446,7 +446,7 @@ function register($username, $password, $name, $email, $allow_multiple_emails = */ function generateInviteCode($username) { $time = time(); - return "{$time}." . _elgg_services()->crypto->getHmac($time . $username); + return "{$time}." . _elgg_services()->crypto->getHmac("$time|$username"); } /** @@ -467,7 +467,7 @@ function validateInviteCode($username, $code) { $mac = $m[2]; $crypto = _elgg_services()->crypto; - return $crypto->areEqual($mac, $crypto->getHmac($time . $username)); + return $crypto->areEqual($mac, $crypto->getHmac("$time|$username")); } /** diff --git a/engine/classes/ElggCrypto.php b/engine/classes/ElggCrypto.php index 8f974957b72..6b9da5508e2 100644 --- a/engine/classes/ElggCrypto.php +++ b/engine/classes/ElggCrypto.php @@ -165,9 +165,10 @@ public function getRandomBytes($length) { * @param string $algo HMAC hash algorithm * @return string */ - function getHmac($data, $key = '', $algo = 'sha256') { + public function getHmac($data, $key = '', $algo = 'sha256') { if (!$key) { - $key = _elgg_services()->siteSecret->get(); + // Prefer binary key for a couple good reasons: http://crypto.stackexchange.com/a/8662/7197 + $key = _elgg_services()->siteSecret->get(true); } $bytes = hash_hmac($algo, $data, $key, true); return strtr(rtrim(base64_encode($bytes), '='), '+/', '-_'); diff --git a/engine/lib/actions.php b/engine/lib/actions.php index c0854e347f2..fd41143ea7a 100644 --- a/engine/lib/actions.php +++ b/engine/lib/actions.php @@ -97,6 +97,34 @@ function elgg_unregister_action($action) { return _elgg_services()->actions->unregister($action); } +/** + * Generate an authentication code/token in Base64URL encoding, using the site secret as key. + * + * @param string $data Data passed through an untrusted channel + * + * @return string + * @since 1.11 + * @see elgg_validate_mac + */ +function elgg_generate_mac($data) { + return _elgg_services()->crypto->getHmac($data); +} + +/** + * Validate an authentication code generated via elgg_generate_mac(). + * + * @param string $mac MAC given in input, in Base64URL format + * @param string $data Data ostensibly unaltered by the user + * + * @return bool + * @since 1.10 + * @see elgg_generate_mac + */ +function elgg_validate_mac($mac, $data) { + $crypto = _elgg_services()->crypto; + return $crypto->areEqual($mac, $crypto->getHmac($data)); +} + /** * Validate an action token. * diff --git a/engine/tests/phpunit/ElggCryptoTest.php b/engine/tests/phpunit/ElggCryptoTest.php index a478b89912f..6f56503b0d8 100644 --- a/engine/tests/phpunit/ElggCryptoTest.php +++ b/engine/tests/phpunit/ElggCryptoTest.php @@ -7,6 +7,10 @@ class ElggCryptoTest extends \PHPUnit_Framework_TestCase { */ protected $stub; + /** + * @see ElggCrypto + * @see ElggCrypto::getRandomBytes + */ protected function setUp() { $this->stub = $this->getMockBuilder('\ElggCrypto') ->setMethods(array('getRandomBytes')) @@ -41,4 +45,36 @@ function provider() { function testGetRandomString($length, $chars, $expected) { $this->assertSame($expected, $this->stub->getRandomString($length, $chars)); } + + function testGeneratesMacInBase64Url() { + $crypto = new ElggCrypto(); + $key = 'a very bad key'; + $data = '1'; + $expected = 'nL0lgXrVWgGK0Cmr9_PjqQcR2_PzuAHH114AsPZk-AM'; + $algo = 'sha256'; + + $this->assertEquals($expected, $crypto->getHmac($data, $key, $algo)); + } + + function testStringCastDoesntAffectMacs() { + $crypto = new ElggCrypto(); + $key = 'a very bad key'; + + $this->assertEquals($crypto->getHmac(1234, $key), $crypto->getHmac('1234', $key)); + } + + function testMacAlteredByVaryingData() { + $crypto = new ElggCrypto(); + $key = 'a very bad key'; + + $this->assertNotEquals($crypto->getHmac('1234', $key), $crypto->getHmac('1235', $key)); + } + + function testMacAlteredByVaryingKey() { + $crypto = new ElggCrypto(); + $key1 = 'a very bad key'; + $key2 = 'b very bad key'; + + $this->assertNotEquals($crypto->getHmac('1234', $key1), $crypto->getHmac('1234', $key2)); + } } diff --git a/mod/uservalidationbyemail/lib/functions.php b/mod/uservalidationbyemail/lib/functions.php index c1320531f22..6e99a37d1d0 100644 --- a/mod/uservalidationbyemail/lib/functions.php +++ b/mod/uservalidationbyemail/lib/functions.php @@ -16,7 +16,7 @@ function uservalidationbyemail_generate_code($user_guid, $email_address) { // Note: binding to site URL for multisite. $site_url = elgg_get_site_url(); - return _elgg_services()->crypto->getHmac($user_guid . $email_address . $site_url); + return elgg_generate_mac("$user_guid|$email_address|$site_url"); } /** @@ -79,12 +79,13 @@ function uservalidationbyemail_request_validation($user_guid, $admin_requested = */ function uservalidationbyemail_validate_email($user_guid, $code) { $user = get_entity($user_guid); + $site_url = elgg_get_site_url(); - if ($code == uservalidationbyemail_generate_code($user_guid, $user->email)) { - return elgg_set_user_validation_status($user_guid, true, 'email'); + if (!elgg_validate_mac($code, "$user_guid|{$user->email}|$site_url")) { + return false; } - return false; + return elgg_set_user_validation_status($user_guid, true, 'email'); } /**