diff --git a/snappymail/v/0.0.0/app/libraries/snappymail/crypt.php b/snappymail/v/0.0.0/app/libraries/snappymail/crypt.php index 77ea6f29a..42fa881b4 100644 --- a/snappymail/v/0.0.0/app/libraries/snappymail/crypt.php +++ b/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); diff --git a/test/crypt-forgery.php b/test/crypt-forgery.php new file mode 100644 index 000000000..59eedef78 --- /dev/null +++ b/test/crypt-forgery.php @@ -0,0 +1,77 @@ + '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"; +}