@ZaphodB fix(security): tokens could be forged by choosing the xxtea algorithm

Crypt::Decrypt() calls "{name}Decrypt" with the algorithm name taken from the
token itself, so a client could ask for xxtea on any server. XxteaDecrypt used
$salt . Passphrase() as the key, but XXTEA reads only the first 16 bytes of its
key and the salt is 16 bytes stored in the token: the server secret never took
part. Anyone could therefore make a token every Crypt::Decrypt* consumer would
accept as genuine (remember-me, secure cookies, DecodeKeyValuesQ raw keys, ...),
and any real xxtea token could be read. The consumers traced so far still
require a server-side record or a real IMAP login, so this is not a known
account takeover, but the integrity check they rely on was not there.

- Decrypt refuses xxtea whenever sodium or openssl is available, i.e. whenever
  Encrypt() would never have produced it, compares the name case-insensitively
  (PHP method names are), and dispatches only sodium, openssl and xxtea rather
  than any method name ending in Decrypt.
- The xxtea key is now the first 16 bytes of sha256(salt . passphrase), so on a
  host left with nothing but xxtea it depends on the secret. Tokens written under
  the old derivation stop decrypting there; they were never keyed by the server.

test/crypt-forgery.php forges a token with an attacker-chosen salt (accepted
before this change), checks it and an arbitrary method name are refused, and
that sodium, url-safe and xxtea round trips still work only with the right key.

snappymail/v/0.0.0/app/libraries/snappymail/crypt.php

@@ -64,8 +64,21 @@ abstract class Crypt
 	) /* : mixed */
 	{
 		if (3 === \count($data) && isset($data[0], $data[1], $data[2]) && \strlen($data[0])) {
-			$fn = "{$data[0]}Decrypt";
-			if (!\method_exists(__CLASS__, $fn)) {
+			// The algorithm name comes from the token, so it must not be able to
+			// pick a weaker one than Encrypt() would have used. Xxtea is only
+			// written when neither sodium nor openssl is available; anywhere else
+			// an xxtea token was not made by this server, and before the key
+			// derivation fix below its 16 byte salt was the entire key, so anyone
+			// could mint one.
+			$algo = \strtolower($data[0]);
+			if ('xxtea' === $algo
+			 && (\is_callable('sodium_crypto_aead_xchacha20poly1305_ietf_decrypt') || \is_callable('openssl_decrypt'))
+			) {
+				Log::warning('Crypt', 'xxtea token refused, a stronger algorithm is available');
+				return null;
+			}
+			$fn = "{$algo}Decrypt";
+			if (!\in_array($algo, ['sodium', 'openssl', 'xxtea'], true) || !\method_exists(__CLASS__, $fn)) {
 				Log::warning('Crypt', "{$fn} does not exists");
 			} else {
 				try {
@@ -261,6 +274,22 @@ abstract class Crypt
 		return $result;
 	}
 
+	/**
+	 * XXTEA takes a 128 bit key and reads only the first 16 bytes of what it is
+	 * given. The key used to be $salt . Passphrase(), and the salt is 16 random
+	 * bytes stored in the token, so the server secret never took part: xxtea
+	 * tokens could be read and forged by anyone. Hash the two together instead.
+	 * Tokens made under the old derivation no longer decrypt; they were never
+	 * keyed by this server.
+	 */
+	private static function XxteaKey(string $salt,
+		#[\SensitiveParameter]
+		?string $key
+	) : string
+	{
+		return \substr(\hash('sha256', $salt . static::Passphrase($key), true), 0, 16);
+	}
+
 	public static function XxteaDecrypt(string $data, string $salt,
 		#[\SensitiveParameter]
 		?string $key = null
@@ -269,7 +298,7 @@ abstract class Crypt
 		if (!$data || !$salt) {
 			throw new \ValueError('$data or $salt is empty string');
 		}
-		$key = $salt . static::Passphrase($key);
+		$key = static::XxteaKey($salt, $key);
 		return \is_callable('xxtea_decrypt')
 			? \xxtea_decrypt($data, $key)
 			: \MailSo\Base\Xxtea::decrypt($data, $key);
@@ -286,7 +315,7 @@ abstract class Crypt
 		if (!$data || !$salt) {
 			throw new \ValueError('$data or $salt is empty string');
 		}
-		$key = $salt . static::Passphrase($key);
+		$key = static::XxteaKey($salt, $key);
 		$result = \is_callable('xxtea_encrypt')
 			? \xxtea_encrypt($data, $key)
 			: \MailSo\Base\Xxtea::encrypt($data, $key);

test/crypt-forgery.php

@@ -0,0 +1,77 @@
+<?php
+
+// Run with: php test/crypt-forgery.php
+
+// Crypt logs through Tachyon\Util\Log and reads its cipher from Tachyon\Api at
+// load time, both of which need the whole application. Stand-ins are declared
+// before the autoloader could load the real classes; Config returns defaults.
+namespace Tachyon\Util {
+	abstract class Log
+	{
+		public static function __callStatic(string $name, array $arguments) : void {}
+	}
+}
+
+namespace Tachyon {
+	abstract class Api
+	{
+		public static function Config() : object
+		{
+			return new class {
+				public function Get(string $section, string $name, $default = null) { return $default; }
+			};
+		}
+	}
+}
+
+namespace {
+	define('APP_SALT', 'test-salt-'.bin2hex(random_bytes(8)));
+	define('APP_VERSION', 'test');
+	$_COOKIE['smctoken'] = base64_encode(random_bytes(16));
+	spl_autoload_register(static function (string $class): void {
+		$base = dirname(__DIR__).'/tachyon/v/0.0.0/app/libraries/';
+		$file = str_starts_with($class, 'Tachyon\\Util\\')
+			? $base.'tachyon_util/'.strtolower(str_replace('\\', '/', substr($class, 13))).'.php'
+			: $base.str_replace('\\', '/', $class).'.php';
+		if (is_file($file)) {
+			require_once $file;
+		}
+	});
+
+	function check(bool $condition, string $message): void
+	{
+		if (!$condition) {
+			throw new RuntimeException($message);
+		}
+	}
+
+	$Crypt = 'Tachyon\\Util\\Crypt';
+
+	check(is_callable('sodium_crypto_aead_xchacha20poly1305_ietf_decrypt'), 'test needs sodium');
+
+	// Forge a token without knowing APP_SALT: the attacker picks the 16 byte
+	// salt, which under the old derivation was the whole XXTEA key.
+	$salt = random_bytes(16);
+	$forged = \MailSo\Base\Xxtea::encrypt(json_encode(['e' => 'victim@example.com', 'admin' => true]), $salt);
+	foreach (['xxtea', 'XXTEA', 'Xxtea'] as $algo) {
+		check(null === $Crypt::Decrypt([$algo, $salt, $forged], 'server-key'), "forged {$algo} token accepted");
+	}
+
+	// Unknown algorithm names are refused rather than dispatched.
+	check(null === $Crypt::Decrypt(['jsonDecode', $salt, $forged], 'server-key'), 'arbitrary method name dispatched');
+
+	// Normal tokens still round-trip, and only with the right key.
+	$data = ['e' => 'user@example.com', 'n' => 42];
+	check($data === $Crypt::Decrypt($Crypt::Encrypt($data, 'server-key'), 'server-key'), 'sodium round trip broke');
+	check($data === $Crypt::DecryptUrlSafe($Crypt::EncryptUrlSafe($data, 'server-key'), 'server-key'), 'url-safe round trip broke');
+	check(null === $Crypt::Decrypt($Crypt::Encrypt($data, 'server-key'), 'other-key'), 'decrypted with the wrong key');
+
+	// The xxtea primitives themselves now depend on the server secret.
+	$salt = random_bytes(16);
+	$cipher = $Crypt::XxteaEncrypt('{"a":1}', $salt, 'server-key');
+	check('{"a":1}' === $Crypt::XxteaDecrypt($cipher, $salt, 'server-key'), 'xxtea round trip broke');
+	check('{"a":1}' !== $Crypt::XxteaDecrypt($cipher, $salt, 'other-key'), 'xxtea key does not depend on the secret');
+	check('{"a":1}' !== \MailSo\Base\Xxtea::decrypt($cipher, $salt), 'xxtea still keyed by the salt alone');
+
+	echo "crypt-forgery: ok\n";
+}