Merge pull request #8057 from mrclay/token_svc_7824

feature(security): Adds functions to create and validate HMAC tokens

docs/guides/actions.rst

@@ -425,3 +425,34 @@ 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 <http://security.stackexchange.com/a/20301/4982>`_ 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_build_hmac()`` 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_build_hmac([$a, $b])->getToken(),
+    ]);
+    $url = "action/foo?$query";
+
+
+    // validate the querystring
+    $a = get_input('a', '', false);
+    $b = get_input('b', '', false);
+    $mac = get_input('mac', '', false);
+
+    if (elgg_build_hmac([$a, $b])->matchesToken($mac)) {
+        // $a and $b have not been altered
+    }

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,15 @@ 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')
+			->getToken();
 	}
 
 	/**

engine/classes/Elgg/Database/SiteSecret.php

@@ -2,7 +2,16 @@
 namespace Elgg\Database;
 
 /**
- * WARNING: API IN FLUX. DO NOT USE DIRECTLY.
+ * Manages a site-specific secret key, encoded as a 32 byte string "secret"
+ *
+ * The key can have two formats:
+ *   - Since 1.8.17 all keys generated are Base64URL-encoded with the 1st character set to "z" so that
+ *     the format can be recognized. With one character lost, this makes the keys effectively 186 bits.
+ *   - Before 1.8.17 keys were hex-encoded (128 bits) but created from insufficiently random sources.
+ *
+ * The hex keys were created with rand() as the only decent source of entropy (the site's creation time
+ * is not too difficult to find). As such, systems with a low getrandmax() value created particularly
+ * weak keys. You can check key string using getStrength().
  *
  * @access private
  *
@@ -11,6 +20,7 @@
  * @since      1.10.0
  */
 class SiteSecret {
+
 	/**
 	 * Initialise the site secret (32 bytes: "z" to indicate format + 186-bit key in Base64 URL).
 	 *
@@ -23,34 +33,55 @@ class SiteSecret {
 	 */
 	function init() {
 		$secret = 'z' . _elgg_services()->crypto->getRandomString(31);
-	
+
 		if (_elgg_services()->datalist->set('__site_secret__', $secret)) {
 			return $secret;
 		}
-	
+
 		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;
 	}
-	
+
 	/**
 	 * Get the strength of the site secret
 	 *
+	 * If "weak" or "moderate" is returned, this assumes we're running on the same system that created
+	 * the key.
+	 *
 	 * @return string "strong", "moderate", or "weak"
 	 * @access private
 	 */
@@ -67,5 +98,5 @@ function getStrength() {
 		}
 		return 'strong';
 	}
-		
-}
+
+}

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])->getToken();
 	}
 
 	/**
@@ -466,8 +466,7 @@ function validateInviteCode($username, $code) {
 		$time = $m[1];
 		$mac = $m[2];
 
-		$crypto = _elgg_services()->crypto;
-		return $crypto->areEqual($mac, $crypto->getHmac($time . $username));
+		return _elgg_services()->crypto->getHmac([$time, $username])->matchesToken($mac);
 	}
 
 	/**

engine/classes/Elgg/Security/Hmac.php

@@ -0,0 +1,72 @@
+<?php
+namespace Elgg\Security;
+
+/**
+ * Component for creating HMAC tokens
+ */
+class Hmac {
+
+	/**
+	 * @var string
+	 */
+	private $key;
+
+	/**
+	 * @var callable
+	 */
+	private $comparator;
+
+	/**
+	 * @var string
+	 */
+	private $data;
+
+	/**
+	 * @var string
+	 */
+	private $algo;
+
+	/**
+	 * Constructor
+	 *
+	 * @param string          $key        HMAC key
+	 * @param callable        $comparator Function that returns true if given two equal strings, else false
+	 * @param string[]|string $data       HMAC data string (or array of strings)
+	 * @param string          $algo       Hash algorithm
+	 */
+	public function __construct($key, callable $comparator, $data, $algo = 'sha256') {
+		$this->key = $key;
+		$this->comparator = $comparator;
+		if (!$data) {
+			throw new \InvalidArgumentException('$data cannot be empty');
+		}
+		if (is_array($data)) {
+			$data = json_encode(array_map('strval', $data));
+		} else {
+			$data = (string)$data;
+		}
+		$this->data = $data;
+		$this->algo = $algo;
+	}
+
+	/**
+	 * Get the HMAC token in Base64URL encoding
+	 *
+	 * @return string
+	 */
+	public function getToken() {
+		$bytes = hash_hmac($this->algo, $this->data, $this->key, true);
+		return strtr(rtrim(base64_encode($bytes), '='), '+/', '-_');
+	}
+
+	/**
+	 * Does the MAC match the given token?
+	 *
+	 * @param string $token HMAC token in Base64URL encoding
+	 * @return bool
+	 */
+	public function matchesToken($token) {
+		$expected_token = $this->getToken();
+		return call_user_func($this->comparator, $expected_token, $token);
+	}
+}

engine/classes/ElggCrypto.php

@@ -158,19 +158,19 @@ public function getRandomBytes($length) {
 	}
 
 	/**
-	 * Generate a MAC with output in Base64URL encoding
+	 * Get an HMAC token builder/validator object
 	 *
-	 * @param string $data Data we're creating a MAC for
-	 * @param string $key  HMAC key. uses elgg site secret if none given
-	 * @param string $algo HMAC hash algorithm
-	 * @return string
+	 * @param string[]|string $data HMAC data, or array of strings to use as data
+	 * @param string          $algo Hash algorithm
+	 * @param string          $key  Optional key (default uses site secret)
+	 *
+	 * @return \Elgg\Security\Hmac
 	 */
-	function getHmac($data, $key = '', $algo = 'sha256') {
+	public function getHmac($data, $algo = 'sha256', $key = '') {
 		if (!$key) {
-			$key = _elgg_services()->siteSecret->get();
+			$key = _elgg_services()->siteSecret->get(true);
 		}
-		$bytes = hash_hmac($algo, $data, $key, true);
-		return strtr(rtrim(base64_encode($bytes), '='), '+/', '-_');
+		return new Elgg\Security\Hmac($key, [$this, 'areEqual'], $data, $algo);
 	}
 
 	/**

engine/lib/actions.php

@@ -97,6 +97,17 @@ function elgg_unregister_action($action) {
 	return _elgg_services()->actions->unregister($action);
 }
 
+/**
+ * Get an HMAC token builder/validator object
+ *
+ * @param string[]|string $data HMAC data, or array of strings to use as data
+ * @return \Elgg\Security\Hmac
+ * @since 1.11
+ */
+function elgg_build_hmac($data) {
+	return _elgg_services()->crypto->getHmac($data);
+}
+
 /**
  * Validate an action token.
  *

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,65 @@ 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, $algo, $key)->getToken());
+	}
+
+	function testStringCastDoesntAffectMacs() {
+		$crypto = new ElggCrypto();
+		$key = 'a very bad key';
+
+		$t1 = $crypto->getHmac(1234, 'sha256', $key)->getToken();
+		$t2 = $crypto->getHmac('1234', 'sha256', $key)->getToken();
+
+		$this->assertEquals($t1, $t2);
+	}
+
+	function testMacAlteredByVaryingData() {
+		$crypto = new ElggCrypto();
+		$key = 'a very bad key';
+
+		$t1 = $crypto->getHmac('1234', 'sha256', $key)->getToken();
+		$t2 = $crypto->getHmac('1235', 'sha256', $key)->getToken();
+
+		$this->assertNotEquals($t1, $t2);
+	}
+
+	function testMacAlteredByVaryingKey() {
+		$crypto = new ElggCrypto();
+		$key1 = 'a very bad key';
+		$key2 = 'b very bad key';
+
+		$t1 = $crypto->getHmac('1234', 'sha256', $key1)->getToken();
+		$t2 = $crypto->getHmac('1234', 'sha256', $key2)->getToken();
+
+		$this->assertNotEquals($t1, $t2);
+	}
+
+	function testCanAcceptDataAsArray() {
+		$crypto = new ElggCrypto();
+		$key = 'a very bad key';
+
+		$token = $crypto->getHmac([12, 34], 'sha256', $key)->getToken();
+		$matches = $crypto->getHmac([12, 34], 'sha256', $key)->matchesToken($token);
+
+		$this->assertTrue($matches);
+	}
+
+	function testMacAlteredByArrayModification() {
+		$crypto = new ElggCrypto();
+		$key = 'a very bad key';
+
+		$t1 = $crypto->getHmac([12, 34], 'sha256', $key)->getToken();
+		$t2 = $crypto->getHmac([123, 4], 'sha256', $key)->getToken();
+
+		$this->assertNotEquals($t1, $t2);
+	}
 }