From b6607bf6e16955fab34bbbcc877f7418be190e57 Mon Sep 17 00:00:00 2001 From: the-djmaze <> Date: Sat, 24 Oct 2026 11:50:10 +0200 Subject: [PATCH 01/11] =?UTF-8?q?@ZaphodB=20fix(security):=20strip=20line?= =?UTF-8?q?=20breaks=20from=20decoded=20and=20outgoing=20header=20values?= =?UTF-8?q?=20DecodeHeaderValue=20removed=20CR=20and=20LF=20before=20decod?= =?UTF-8?q?ing=20encoded-words,=20so=20an=20encoded-word=20could=20still?= =?UTF-8?q?=20decode=20to=20a=20line=20break.=20A=20message=20whose=20Mess?= =?UTF-8?q?age-ID=20was=20=3D=3Futf-8=3Fb=3F...=3F=3D=20with=20CRLF=20insi?= =?UTF-8?q?de=20reached=20the=20client=20with=20the=20break=20intact,=20th?= =?UTF-8?q?e=20client=20copies=20Message-ID=20into=20In-Reply-To=20when=20?= =?UTF-8?q?replying=20or=20forwarding,=20and=20SetInReplyTo=20only=20trimm?= =?UTF-8?q?ed=20it.=20The=20reply=20then=20carried=20header=20lines=20chos?= =?UTF-8?q?en=20by=20the=20sender=20of=20the=20original,=20or=20a=20body?= =?UTF-8?q?=20if=20the=20payload=20held=20a=20blank=20line,=20sent=20and?= =?UTF-8?q?=20DKIM-signed=20as=20the=20user=20and=20saved=20to=20Sent.=20S?= =?UTF-8?q?ubject=20on=20Edit-as-new=20had=20the=20same=20shape=20through?= =?UTF-8?q?=20the=20ASCII=20path.?= Strip CR, LF and NUL in two places: after decoding in DecodeHeaderValue, and in Header itself, which every outgoing header is built through, so a value that arrives by another route (client parameters, plugins) cannot break the line either. Folding is unaffected; __toString adds it back. test/header-injection.php covers B and Q encoded-words, parsing, outgoing In-Reply-To and Subject, and that long values still fold. --- .../0.0.0/app/libraries/MailSo/Base/Utils.php | 6 ++- .../app/libraries/MailSo/Mime/Header.php | 9 +++- test/header-injection.php | 52 +++++++++++++++++++ 3 files changed, 64 insertions(+), 3 deletions(-) create mode 100644 test/header-injection.php diff --git a/snappymail/v/0.0.0/app/libraries/MailSo/Base/Utils.php b/snappymail/v/0.0.0/app/libraries/MailSo/Base/Utils.php index f16598be8..d96e2f74c 100644 --- a/snappymail/v/0.0.0/app/libraries/MailSo/Base/Utils.php +++ b/snappymail/v/0.0.0/app/libraries/MailSo/Base/Utils.php @@ -300,7 +300,11 @@ abstract class Utils $sValue = static::ConvertEncoding($sValue, $sMainCharset, Enumerations\Charset::UTF_8); } - return $sValue; + // The strip above runs before decoding, so an encoded-word such as + // =?utf-8?b?...?= can still decode to a CR or LF. A decoded header value + // is one line; a line break here would be carried into the headers of a + // reply (In-Reply-To is copied from Message-ID) and inject new ones. + return \preg_replace('/[\r\n\0]+/', ' ', $sValue); } public static function RemoveHeaderFromHeaders(string $sIncHeaders, array $aHeadersToRemove = array()) : string diff --git a/snappymail/v/0.0.0/app/libraries/MailSo/Mime/Header.php b/snappymail/v/0.0.0/app/libraries/MailSo/Mime/Header.php index f48cdf0f4..1abbb989e 100644 --- a/snappymail/v/0.0.0/app/libraries/MailSo/Mime/Header.php +++ b/snappymail/v/0.0.0/app/libraries/MailSo/Mime/Header.php @@ -37,8 +37,13 @@ class Header implements \JsonSerializable private function initInputData(string $sName, string $sValue, string $sEncodedValueForReparse) : void { - $this->sName = \trim($sName); - $this->sFullValue = \trim($sValue); + // A header is one logical line. A CR or LF in a name or value lets the + // value end its own header and start another (or the body), and values + // reach here from decoded encoded-words and from client parameters, so + // no caller can be relied on to have stripped them. Folding is added + // back by __toString(). + $this->sName = \trim(\preg_replace('/[\r\n\0]+/', ' ', $sName)); + $this->sFullValue = \trim(\preg_replace('/[\r\n\0]+/', ' ', $sValue)); $this->sEncodedValue = ''; if (\strlen($sEncodedValueForReparse) && ($this->IsEmail() || $this->IsSubject() || $this->IsParameterized())) { diff --git a/test/header-injection.php b/test/header-injection.php new file mode 100644 index 000000000..a1115151d --- /dev/null +++ b/test/header-injection.php @@ -0,0 +1,52 @@ +\r\nBcc: attacker@example.net"; +$encoded = '=?utf-8?b?'.base64_encode($payload).'?='; +$decoded = \MailSo\Base\Utils::DecodeHeaderValue($encoded); +check(!preg_match('/[\r\n]/', $decoded), 'B-encoded CRLF survived DecodeHeaderValue'); +check(str_contains($decoded, 'Bcc: attacker@example.net'), 'decoded text was lost, not just its line break'); + +$decoded = \MailSo\Base\Utils::DecodeHeaderValue('=?utf-8?q?a=0D=0AX-Injected:_1?='); +check(!preg_match('/[\r\n]/', $decoded), 'Q-encoded CRLF survived DecodeHeaderValue'); + +// The same through header parsing, as for an incoming Message-ID. +$header = \MailSo\Mime\Header::NewInstanceFromEncodedString("Message-ID: {$encoded}"); +check(!preg_match('/[\r\n]/', $header->Value()), 'parsed Message-ID kept a line break'); + +// Outgoing: a value handed straight to the message (In-Reply-To and Subject come +// from client parameters) must serialise as one header, whatever it contains. +$message = new \MailSo\Mime\Message(); +$message->SetInReplyTo("\r\nBcc: attacker@example.net"); +$message->SetSubject("Re: hello\nX-Injected: 1\r\n\r\nbody text"); +$raw = stream_get_contents($message->ToStream()); +[$headers] = explode("\r\n\r\n", $raw, 2); +foreach (explode("\r\n", $headers) as $line) { + check(!preg_match('/^(Bcc|X-Injected):/i', $line), "injected header line: {$line}"); +} +check(!str_contains($headers, "\n\n"), 'blank line injected into the header block'); +check(str_contains($raw, 'In-Reply-To: Bcc: attacker@example.net'), 'In-Reply-To was not kept as one line'); + +// Folding must still work: a long value is wrapped with CRLF + space only. +$header = new \MailSo\Mime\Header('Subject', str_repeat('word ', 40)); +foreach (array_slice(explode("\r\n", (string) $header), 1) as $continuation) { + check(str_starts_with($continuation, ' '), 'folded line does not start with whitespace'); +} + +echo "header-injection: ok\n"; From 3776ac6b32f9c6f7bac2f67595fdcf2e973d0095 Mon Sep 17 00:00:00 2001 From: the-djmaze <> Date: Sat, 24 Oct 2026 11:57:38 +0200 Subject: [PATCH 02/11] @ZaphodB fix(security): S/MIME verify reported altered messages as validly signed OpenSSL::verify() called openssl_pkcs7_verify with PKCS7_NOSIGS, which skips the signature check, and returned success whenever that call did. Any well-formed signed message therefore came back as success: true, including one whose content had been changed after signing, and the client shows that as a good signature (Message.js smimeSigned). Applies to the message view, the explicit verify action and signed content inside decrypted mail. The call is also how the content of an opaque signed message is extracted for display, so it stays as it is for that, and a second openssl_pkcs7_verify without PKCS7_NOSIGS now decides success and does not throw on failure. A message with a broken signature remains readable but is no longer reported as signed. PKCS7_NOVERIFY is kept on purpose: the signer certificate is not checked against a CA, so success means the content is what the certificate holder signed, not that the certificate is trusted; the comment says so. test/smime-verify.php signs detached and opaque messages with a throwaway key, checks the intact ones verify, and that altering one byte of signed content flips success to false while the content stays readable. Before this change the altered detached message was reported as validly signed. --- .gitignore | 1 - .../libraries/snappymail/smime/openssl.php | 19 ++++- test/smime-verify.php | 69 +++++++++++++++++++ 3 files changed, 86 insertions(+), 3 deletions(-) create mode 100644 test/smime-verify.php diff --git a/.gitignore b/.gitignore index 5f49d65d5..18da1fe05 100644 --- a/.gitignore +++ b/.gitignore @@ -20,7 +20,6 @@ /issues .idea/ .env -/test /public_html /vendors/knockout/spec /vendors/openpgp-5 diff --git a/snappymail/v/0.0.0/app/libraries/snappymail/smime/openssl.php b/snappymail/v/0.0.0/app/libraries/snappymail/smime/openssl.php index 3af3b7f25..c7bc0c059 100644 --- a/snappymail/v/0.0.0/app/libraries/snappymail/smime/openssl.php +++ b/snappymail/v/0.0.0/app/libraries/snappymail/smime/openssl.php @@ -324,10 +324,11 @@ class OpenSSL $opaque |= \str_contains($input, 'application/pkcs7-mime') || \str_contains($input, 'application/x-pkcs7-mime'); $input = $tmp; } + // First pass extracts the content and nothing else: PKCS7_NOSIGS skips the + // signature check, so a message with a broken signature can still be read. $output = $opaque ? new Temporary('smimeout-') : null; if (true !== \openssl_pkcs7_verify( $input->filename(), -// $flags = 0, // \PKCS7_NOVERIFY | \PKCS7_NOCHAIN | \PKCS7_NOSIGS \PKCS7_NOVERIFY | \PKCS7_NOCHAIN | \PKCS7_NOSIGS, $signers_certificates_filename ?: null, $ca_info = [], @@ -337,9 +338,23 @@ class OpenSSL )) { throw new \RuntimeException('OpenSSL verify: ' . \openssl_error_string()); } + // Second pass is the verdict. It used to be the first pass returning true, + // which with PKCS7_NOSIGS it does for any well-formed message, so altered + // or forged content was reported as validly signed. This checks the + // signature against the signer certificate in the message. PKCS7_NOVERIFY + // stays: the signer's chain is not checked against any CA, so 'success' + // means the content is what the certificate holder signed, not that the + // certificate itself is trusted. + $verified = \openssl_pkcs7_verify( + $input->filename(), + \PKCS7_NOVERIFY, + null, + [], + $this->untrusted_certificates_filename + ); return [ 'body' => $output ? $output->getContents() : null, - 'success' => true + 'success' => true === $verified ]; } } diff --git a/test/smime-verify.php b/test/smime-verify.php new file mode 100644 index 000000000..8529b14d2 --- /dev/null +++ b/test/smime-verify.php @@ -0,0 +1,69 @@ + 2048, 'private_key_type' => OPENSSL_KEYTYPE_RSA]); +$cert = openssl_csr_sign(openssl_csr_new(['commonName' => 'signer@example.com'], $key), null, $key, 1); +openssl_x509_export_to_file($cert, "{$dir}/cert.pem"); +openssl_pkey_export_to_file($key, "{$dir}/key.pem"); + +$content = "Content-Type: text/plain\r\n\r\nPay 100 EUR to account A.\r\n"; +file_put_contents("{$dir}/in.txt", $content); + +$smime = new \Tachyon\Util\SMime\OpenSSL($dir); + +// Detached (multipart/signed) and opaque (application/pkcs7-mime) signatures. +foreach (['detached' => PKCS7_DETACHED, 'opaque' => 0] as $kind => $flags) { + check(openssl_pkcs7_sign("{$dir}/in.txt", "{$dir}/signed.eml", "file://{$dir}/cert.pem", "file://{$dir}/key.pem", [], $flags), + "{$kind}: signing failed: ".openssl_error_string()); + $signed = file_get_contents("{$dir}/signed.eml"); + $opaque = 'opaque' === $kind; + + $result = $smime->verify($signed, null, $opaque); + check(true === $result['success'], "{$kind}: intact message not reported as validly signed"); + if ($opaque) { + check(str_contains($result['body'], 'Pay 100 EUR to account A.'), "{$kind}: content not extracted"); + } + + // Alter the signed content. Detached: the clear text part. Opaque: a byte of + // the base64 payload, chosen inside the encapsulated content. + if ($opaque) { + [$head, $b64] = explode("\n\n", str_replace("\r\n", "\n", $signed), 2); + $der = base64_decode($b64); + $pos = strpos($der, 'account A'); + check(false !== $pos, "{$kind}: test could not find the content to alter"); + $der[$pos + 8] = 'B'; + $tampered = $head."\n\n".chunk_split(base64_encode($der), 64, "\n"); + } else { + $tampered = str_replace('account A', 'account B', $signed); + } + check($tampered !== $signed, "{$kind}: test failed to alter the message"); + + $result = $smime->verify($tampered, null, $opaque); + check(false === $result['success'], "{$kind}: ALTERED message reported as validly signed"); + if ($opaque) { + check(str_contains($result['body'], 'account B'), "{$kind}: altered content no longer readable"); + } +} + +array_map('unlink', glob("{$dir}/*")); +rmdir($dir); +echo "smime-verify: ok\n"; From 08f34b99546f636434c00525fb4f3123f86a4a08 Mon Sep 17 00:00:00 2001 From: djmaze <> Date: Sun, 25 Oct 2026 23:28:14 +0100 Subject: [PATCH 03/11] @ZaphodB fix(security): message images loaded without consent through srcset and CSS The sanitizer sends img src, and url() in background-image, list-style-image and content, through the remote content handling: blocked until the user allows it, rewritten to the proxy when it is on. Two other ways to load an image were left alone: - srcset (and sizes) were on the attribute allowlist and never rewritten, so loaded when the message opened. With the proxy on, the CSP keeps img-src to 'self', but a relative srcset="?/ProxyExternal/" is same-origin and made the server fetch an attacker URL at render time: a read receipt with no click. With the proxy off the browser fetched directly. - url() in any other style property (border-image-source, mask-image, filter, ...) kept its value, with the same two outcomes. Drop srcset and sizes from the allowlist (attributes not on it are deleted), and remove any other style property whose value holds url(). Relative img src was already safe: anything not cid:, http(s) or data:image/ is parked as data-x-src-broken. Checked the style sweep in Chromium: border-image-source, mask-image (both spellings), filter and cursor lose the url(), other declarations survive, and the background/list-style shorthands are left to the existing handling. --- dev/Common/Html.js | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/dev/Common/Html.js b/dev/Common/Html.js index 072045f9a..ebaa26bb0 100644 --- a/dev/Common/Html.js +++ b/dev/Common/Html.js @@ -283,7 +283,11 @@ export const // hr 'noshade', // img - 'hspace', 'sizes', 'srcset', 'vspace', + // srcset and sizes are not allowed: only src goes through the + // remote content consent and proxy rewrite below, so a srcset + // URL loaded without asking (directly, or relative to this + // origin through ?/ProxyExternal as a read receipt). + 'hspace', 'vspace', // meter 'low', 'high', 'optimum', 'value', // ol @@ -578,6 +582,19 @@ export const oStyle.color = delAttribute('color'); } + // Only the three properties below go through the remote content and + // cid: handling. A url() in any other property (border-image-source, + // mask-image, filter, ...) would load without consent, a relative one + // through ?/ProxyExternal on this origin, so drop those outright. + let i = oStyle.length; + while (i--) { + const property = oStyle[i]; + if (!['background-image', 'list-style-image', 'content'].includes(property) + && /url\s*\(/i.test(oStyle.getPropertyValue(property))) { + oStyle.removeProperty(property); + } + } + if (!skipStyle) { /* if ('fixed' === oStyle.position) { From 7f1b1e6288b16f843f4a9c277b38000170b76df3 Mon Sep 17 00:00:00 2001 From: djmaze <> Date: Sun, 25 Oct 2026 23:36:57 +0100 Subject: [PATCH 04/11] @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. --- .../0.0.0/app/libraries/snappymail/crypt.php | 37 ++++++++- test/crypt-forgery.php | 77 +++++++++++++++++++ 2 files changed, 110 insertions(+), 4 deletions(-) create mode 100644 test/crypt-forgery.php 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"; +} From bf0aa84fe77251868515b4497c30dbb2daeb0dbd Mon Sep 17 00:00:00 2001 From: djmaze <> Date: Sun, 25 Oct 2026 23:42:42 +0100 Subject: [PATCH 05/11] @ZaphodB fix(security): ?/Test integrity diagnostics answered without a login ServiceTest() ran Integrity::test() for any request. The test builds its base URL from $_SERVER['HTTP_HOST'], sends HEAD requests to it and prints the status and every response header. The client therefore chose where the server sent requests and read the result. Behind nginx the vhost is picked by server_name with the port ignored, so Host: : reaches the app and turns the diagnostic into a probe of the server's own TLS ports and their headers. Answer 404 unless an admin session is present. An admin's browser sends the real Host, so the diagnostic still works for the people it is for. Checked end to end with php -S (4 workers, no network): without a session the old code returned 200 with the diagnostics, this one returns 404 and nothing. --- .../v/0.0.0/app/libraries/snappymail/integrity.php | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/snappymail/v/0.0.0/app/libraries/snappymail/integrity.php b/snappymail/v/0.0.0/app/libraries/snappymail/integrity.php index e6d25e6bd..2397b9e37 100644 --- a/snappymail/v/0.0.0/app/libraries/snappymail/integrity.php +++ b/snappymail/v/0.0.0/app/libraries/snappymail/integrity.php @@ -10,6 +10,16 @@ abstract class Integrity */ public static function test() { + // The integrity test sends HEAD requests to a URL built from the request's + // Host header and prints back every response header. Open to anyone, that + // lets a client choose the target (nginx ignores the port when matching + // server_name, so Host: : reaches it) and read the result. + // It is an admin diagnostic; an admin's own browser sends the real Host. + if (!\RainLoop\Api::Actions()->IsAdminLoggined(false)) { + \MailSo\Base\Http::StatusHeader(404); + return ''; + } + $result = static::phpVersion(); if ($result) { echo '

' . $result . '

'; From 49dd9a7c7c55532d322237ff1fa63d4193959df7 Mon Sep 17 00:00:00 2001 From: djmaze <> Date: Mon, 26 Oct 2026 00:29:19 +0100 Subject: [PATCH 06/11] @ZaphodB fix(security): attachments rendered inline as the type their sender declared Raw view served an attachment inline with the Content-Type from the message, only forcing PDF by file name. A part named photo.png declared as text/html is previewed as an image by extension, and its link opened an HTML page on the webmail origin. The raw CSP (script-src 'none'; child-src 'none') stopped scripts, but replaced the global policy, so forms, meta refresh, base and remote styles and images all worked: a convincing phishing page under the real hostname. - inlineContentType() allows only raster images, PDF, audio, video and text/plain to be shown as declared. Other text/* is shown as text/plain (an HTML attachment displays its source); anything else, SVG and XML included, is sent as an application/octet-stream download. - The raw CSP adds default-src 'none', form-action 'none' and base-uri 'none', with explicit img, media, style and object sources for what raw view legitimately shows. object-src 'self' keeps the browser PDF viewer usable (not verified in a browser here). test/raw-inline-type.php covers the type decision, parameters and case. --- .../app/libraries/RainLoop/Actions/Raw.php | 36 ++++++++++++++ .../app/libraries/RainLoop/ServiceActions.php | 8 +++- test/raw-inline-type.php | 48 +++++++++++++++++++ 3 files changed, 90 insertions(+), 2 deletions(-) create mode 100644 test/raw-inline-type.php diff --git a/snappymail/v/0.0.0/app/libraries/RainLoop/Actions/Raw.php b/snappymail/v/0.0.0/app/libraries/RainLoop/Actions/Raw.php index ef73f9c7f..d7f1c7cdb 100644 --- a/snappymail/v/0.0.0/app/libraries/RainLoop/Actions/Raw.php +++ b/snappymail/v/0.0.0/app/libraries/RainLoop/Actions/Raw.php @@ -212,6 +212,21 @@ trait Raw } } + // The type comes from the message: whoever sent the mail decides it. + // Viewed inline under this origin, text/html (or SVG, XML, ...) + // becomes a page of the sender's choosing; the CSP stops scripts + // but not forms, meta refresh or remote styles. Only a few types + // are shown inline as declared. + if (!$bDownload) { + $sInlineType = static::inlineContentType($sContentType); + if (null === $sInlineType) { + $bDownload = true; + $sContentType = 'application/octet-stream'; + } else { + $sContentType = $sInlineType; + } + } + if (!\headers_sent()) { \header('Content-Type: '.$sContentType); \MailSo\Base\Http::setContentDisposition($bDownload ? 'attachment' : 'inline', ['filename' => $sFileName]); @@ -262,6 +277,27 @@ trait Raw ); } + /** + * The Content-Type to view an attachment inline with, or null to make it a + * download. Raster images, PDF, audio, video and plain text are shown as + * declared; any other text/* is shown as text/plain, so an HTML attachment + * displays its source instead of rendering as a page on this origin. + */ + public static function inlineContentType(string $sContentType) : ?string + { + $sType = \strtolower(\trim(\explode(';', $sContentType, 2)[0])); + if (\in_array($sType, ['image/jpeg', 'image/png', 'image/gif', 'image/webp', 'image/avif', 'image/bmp', + 'application/pdf', 'text/plain'], true) + || \preg_match('#^(audio|video)/[a-z0-9.+-]+$#', $sType) + ) { + return $sType; + } + if (\str_starts_with($sType, 'text/')) { + return 'text/plain'; + } + return null; + } + private static function loadImage($resource, bool $bDetectImageOrientation = false, int $iThumbnailBoxSize = 0) : \SnappyMail\Image { if (\extension_loaded('gmagick')) { $handler = 'gmagick'; } diff --git a/snappymail/v/0.0.0/app/libraries/RainLoop/ServiceActions.php b/snappymail/v/0.0.0/app/libraries/RainLoop/ServiceActions.php index 372c0b462..2392a9cbb 100644 --- a/snappymail/v/0.0.0/app/libraries/RainLoop/ServiceActions.php +++ b/snappymail/v/0.0.0/app/libraries/RainLoop/ServiceActions.php @@ -339,8 +339,12 @@ class ServiceActions $sMethodName = 'Raw'.$sAction; if (\method_exists($this->oActions, $sMethodName)) { \header('X-Raw-Action: '.$sMethodName); - \header('Content-Security-Policy: script-src \'none\'; child-src \'none\'; object-src \'none\''); - + // Raw responses carry content from the message. Besides scripts and + // frames, deny forms, base and everything not needed to show an + // image, media, a PDF or plain text. + \header('Content-Security-Policy: default-src \'none\'; script-src \'none\'; child-src \'none\'; ' + . 'form-action \'none\'; base-uri \'none\'; img-src \'self\' data:; media-src \'self\'; ' + . 'style-src \'self\' \'unsafe-inline\'; object-src \'self\''); $sRawError = ''; $this->oActions->SetActionParams(array( 'RawKey' => empty($this->aPaths[3]) ? '' : $this->aPaths[3], diff --git a/test/raw-inline-type.php b/test/raw-inline-type.php new file mode 100644 index 000000000..7d06eab75 --- /dev/null +++ b/test/raw-inline-type.php @@ -0,0 +1,48 @@ + 'image/png', + 'IMAGE/JPEG; name="x.jpg"' => 'image/jpeg', + 'application/pdf' => 'application/pdf', + 'video/mp4' => 'video/mp4', + 'audio/ogg; codecs=opus' => 'audio/ogg', + 'text/plain; charset=utf-8' => 'text/plain', + // shown as source, never rendered + 'text/html' => 'text/plain', + 'Text/HTML; charset=utf-8' => 'text/plain', + 'text/xml' => 'text/plain', + // downloaded + 'image/svg+xml' => null, + 'application/xhtml+xml' => null, + 'application/xml' => null, + 'application/octet-stream' => null, + 'multipart/related' => null, + '' => null, +]; +foreach ($cases as $in => $expected) { + $got = RawInlineType::inlineContentType($in); + check($expected === $got, "'{$in}': expected ".var_export($expected, true).', got '.var_export($got, true)); +} + +echo "raw-inline-type: ok\n"; From 8678c0e8fd83ddb08c33031c1753db0074f1662b Mon Sep 17 00:00:00 2001 From: djmaze <> Date: Mon, 26 Oct 2026 00:39:16 +0100 Subject: [PATCH 07/11] Not Tachyon --- test/crypt-forgery.php | 24 +++++++++--------------- test/header-injection.php | 10 +++------- test/raw-inline-type.php | 11 ++++------- test/smime-verify.php | 16 ++++------------ 4 files changed, 20 insertions(+), 41 deletions(-) diff --git a/test/crypt-forgery.php b/test/crypt-forgery.php index 59eedef78..a5802cf78 100644 --- a/test/crypt-forgery.php +++ b/test/crypt-forgery.php @@ -2,17 +2,17 @@ // Run with: php test/crypt-forgery.php -// Crypt logs through Tachyon\Util\Log and reads its cipher from Tachyon\Api at +// Crypt logs through SnappyMail\Log and reads its cipher from RainLoop\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 { +namespace SnappyMail { abstract class Log { public static function __callStatic(string $name, array $arguments) : void {} } } -namespace Tachyon { +namespace RainLoop { abstract class Api { public static function Config() : object @@ -25,18 +25,12 @@ namespace Tachyon { } namespace { - define('APP_SALT', 'test-salt-'.bin2hex(random_bytes(8))); - define('APP_VERSION', 'test'); + // Enable SnappyMail Api and include index file + $_ENV['SNAPPYMAIL_INCLUDE_AS_API'] = true; + require '../index.php'; +// define('APP_SALT', 'test-salt-'.bin2hex(random_bytes(8))); + $_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 { @@ -45,7 +39,7 @@ namespace { } } - $Crypt = 'Tachyon\\Util\\Crypt'; + $Crypt = 'SnappyMail\\Crypt'; check(is_callable('sodium_crypto_aead_xchacha20poly1305_ietf_decrypt'), 'test needs sodium'); diff --git a/test/header-injection.php b/test/header-injection.php index a1115151d..187b6a346 100644 --- a/test/header-injection.php +++ b/test/header-injection.php @@ -1,13 +1,9 @@ PKCS7_DETACHED, 'opaque' => 0] as $kind => $flags) { From 650133ae8a5c14c50c05b1582ce56c3128c1a318 Mon Sep 17 00:00:00 2001 From: djmaze <> Date: Tue, 27 Oct 2026 01:30:31 +0100 Subject: [PATCH 08/11] @ZaphodB fix(security): SSRF gate let through CGNAT, multicast and the server's own IPs URIHasPublicHost() relied on FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE alone. On PHP 8.4 that passes 100.64.0.0/10 (shared space: CGNAT, Tailscale, Alibaba metadata at 100.100.100.200), 192.0.0.0/24, 198.18.0.0/15, 224.0.0.0/4, fec0::/10 and ff00::/8. It also passed the server's own public addresses: a fetch to them never leaves the host, so it reaches services that listen on every address but are firewalled from outside (on our host: grafana, forgejo, prometheus and the exporters). Both checks, on the literal host and on every resolved address, now go through IsPublicIP(): PHP's flags, then an explicit list of the ranges above, then the addresses net_get_interfaces() reports for this host (cached per request). test/ssrf-ranges.php checks the new ranges and their edges, that ordinary public addresses still pass, and the own-address rule via the cache. Before this change 100.64.0.1 was treated as public. DNS rebinding between check and connect is still not covered; that needs the connection pinned to the checked address. --- .../v/0.0.0/app/libraries/snappymail/host.php | 312 ++++++++++++++++++ test/ssrf-ranges.php | 40 +++ 2 files changed, 352 insertions(+) create mode 100644 snappymail/v/0.0.0/app/libraries/snappymail/host.php create mode 100644 test/ssrf-ranges.php diff --git a/snappymail/v/0.0.0/app/libraries/snappymail/host.php b/snappymail/v/0.0.0/app/libraries/snappymail/host.php new file mode 100644 index 000000000..18e119593 --- /dev/null +++ b/snappymail/v/0.0.0/app/libraries/snappymail/host.php @@ -0,0 +1,312 @@ + self::UnwrapEmbeddedIPv4($ip) ?: $ip, $ips); + + // Numeric IPv4 literals in non-standard forms (http://2130706433/, + // http://017700000001/, http://0x7f000001/, http://127.1/, + // http://1.2.3.04/): the HTTP client parses these as IPs directly + // (verified: curl connects to 127.0.0.1 for the hex forms with no + // DNS involved) while dns_get_record() below would query them as + // hostnames, so normalize to dotted decimal before validation. + $ips = \array_map(fn($ip) => self::NormalizeNumericIPv4($ip) ?: $ip, $ips); + + // Check them all + if (\array_all($ips, fn($ip) => self::IsPublicIP($ip))) { + return true; + } + + return false; + } + + private static ?array $aLocalAddresses = null; + + private static function PrefixMatches(string $bin, string $net, int $bits) : bool + { + $bytes = \intdiv($bits, 8); + if (\substr($bin, 0, $bytes) !== \substr($net, 0, $bytes)) { + return false; + } + $rest = $bits % 8; + if (!$rest) { + return true; + } + $mask = (0xff << (8 - $rest)) & 0xff; + return (\ord($bin[$bytes]) & $mask) === (\ord($net[$bytes]) & $mask); + } + + /** + * Normalize a numeric IPv4 literal in non-standard form to dotted + * decimal, following inet_aton semantics: 1-4 dot-separated parts, + * each decimal, octal (leading zero) or hex (0x prefix); the last + * part takes all remaining bits (32/24/16/8). Returns null when the + * host is not such a literal. A part like "08" is not parsed as + * numeric by the resolver, so it is left for the hostname path below. + */ + private static function NormalizeNumericIPv4(string $host) : ?string + { + $aParts = \explode('.', $host); + $iCount = \count($aParts); + if ($iCount < 1 || $iCount > 4) { + return null; + } + $aNums = array(); + foreach ($aParts as $sPart) { + if ('' === $sPart + || !\preg_match('/^(0[xX][0-9a-fA-F]+|[0-9]+)$/', $sPart)) { + return null; + } + if (\preg_match('/^0[xX]/', $sPart)) { + if (10 < \strlen($sPart)) { + return null; // longer than 0x + 8 digits: not a 32-bit value + } + $aNums[] = \hexdec($sPart); + continue; + } + if (1 < \strlen($sPart) && '0' === $sPart[0] + && !\preg_match('/^0[0-7]+$/', $sPart)) { + return null; + } + if (12 < \strlen($sPart)) { + return null; // longer than 037777777777: not a 32-bit value + } + $aNums[] = 1 < \strlen($sPart) && '0' === $sPart[0] + ? \octdec($sPart) : (int) $sPart; + } + $iMax = array(4294967295, 16777215, 65535, 255); + for ($i = 0; $i < $iCount - 1; $i++) { + if ($aNums[$i] > 255) { + return null; + } + } + if ($aNums[$iCount - 1] > $iMax[$iCount - 1]) { + return null; + } + switch ($iCount) { + case 1: + $long = $aNums[0]; + break; + case 2: + $long = ($aNums[0] << 24) | $aNums[1]; + break; + case 3: + $long = ($aNums[0] << 24) | ($aNums[1] << 16) | $aNums[2]; + break; + default: + $long = ($aNums[0] << 24) | ($aNums[1] << 16) | ($aNums[2] << 8) | $aNums[3]; + break; + } + return \long2ip($long); + } + + /** + * Unwrap an IPv6 literal with an embedded IPv4 address to dotted + * decimal, or null when the host is not such a literal. Covers the + * IPv4-mapped range ::ffff:0:0/96 (dotted or hex tail), the NAT64 + * well-known prefix 64:ff9b::/96, and the deprecated + * IPv4-compatible form ::a.b.c.d (textual dotted-quad tail only, + * so ::1 stays loopback and is never mistaken for ::0.0.0.1). + * Needed because FILTER_FLAG_NO_RES_RANGE does not treat these + * ranges as reserved even though they reach IPv4 addresses in + * practice. + */ + private static function UnwrapEmbeddedIPv4(string $ip) : ?string + { + $sBin = \inet_pton($ip); + if (false === $sBin || 16 !== \strlen($sBin)) { + return null; // not an IPv6 literal + } + $sPrefix = \substr($sBin, 0, 12); + if ("\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\xff\xff" === $sPrefix // ::ffff:0:0/96 + || "\x00\x64\xff\x9b\x00\x00\x00\x00\x00\x00\x00\x00" === $sPrefix) { // 64:ff9b::/96 + return (string) \inet_ntop(\substr($sBin, 12, 4)); + } + // 6to4, 2002:V4ADDR::/48 (RFC 3056): the address sits in bytes 2 to 5, so + // 2002:7f00:1:: is 127.0.0.1. Deprecated by RFC 7526 and unreachable + // without a configured tunnel, but a host that has one would route it. + if ("\x20\x02" === \substr($sBin, 0, 2)) { + return (string) \inet_ntop(\substr($sBin, 2, 4)); + } + $sLower = \strtolower($ip); + if (\str_starts_with($sLower, '::') + && false !== \filter_var(\substr($sLower, 2), \FILTER_VALIDATE_IP, \FILTER_FLAG_IPV4)) { + return \substr($sLower, 2); // deprecated ::a.b.c.d form + } + return null; + } +} diff --git a/test/ssrf-ranges.php b/test/ssrf-ranges.php new file mode 100644 index 000000000..83c12f6b7 --- /dev/null +++ b/test/ssrf-ranges.php @@ -0,0 +1,40 @@ + $Request::URIHasPublicHost("http://{$host}/x.png"); + +foreach (['100.64.0.1', '100.100.100.200', '100.127.255.254', '192.0.0.1', '198.18.0.1', + '198.19.255.254', '224.0.0.1', '239.255.255.250', '[fec0::1]', '[ff02::1]', + '127.0.0.1', '10.0.0.1', '169.254.169.254', '[::1]', '[fc00::1]', '[::ffff:100.64.0.1]'] as $host) { + check(!$public($host), "{$host} treated as public"); +} +foreach (['100.63.255.255', '100.128.0.1', '198.17.255.255', '198.20.0.1', '8.8.8.8', + '[2001:4860:4860::8888]'] as $host) { + check($public($host), "{$host} wrongly blocked"); +} + +// The server's own addresses. With --network=none only loopback exists, so +// hand the cache an address to stand for this host's public IP. +$prop = new ReflectionProperty($Request, 'aLocalAddresses'); +$prop->setValue(null, [inet_pton('203.0.113.7'), inet_pton('2001:db8:1::53')]); +check(!$public('203.0.113.7'), "the server's own IPv4 address treated as public"); +check(!$public('[2001:db8:1::53]'), "the server's own IPv6 address treated as public"); +check($public('203.0.113.8'), 'a neighbouring address wrongly blocked'); + +check($public('snappymail.eu'), 'snapymail.eu wrongly blocked'); +check(!$public('localhost'), 'localhost should be blocked'); + +echo "ssrf-ranges: ok\n"; From a827f8aded89f897bb594ba9f2c557bca5615572 Mon Sep 17 00:00:00 2001 From: djmaze <> Date: Tue, 27 Oct 2026 01:33:48 +0100 Subject: [PATCH 09/11] Resolve test load path --- test/crypt-forgery.php | 2 +- test/header-injection.php | 2 +- test/raw-inline-type.php | 2 +- test/smime-verify.php | 2 +- test/sso.php | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/test/crypt-forgery.php b/test/crypt-forgery.php index a5802cf78..9ef7ebf19 100644 --- a/test/crypt-forgery.php +++ b/test/crypt-forgery.php @@ -27,7 +27,7 @@ namespace RainLoop { namespace { // Enable SnappyMail Api and include index file $_ENV['SNAPPYMAIL_INCLUDE_AS_API'] = true; - require '../index.php'; + require __DIR__ . '/../index.php'; // define('APP_SALT', 'test-salt-'.bin2hex(random_bytes(8))); $_COOKIE['smctoken'] = base64_encode(random_bytes(16)); diff --git a/test/header-injection.php b/test/header-injection.php index 187b6a346..88943ca5c 100644 --- a/test/header-injection.php +++ b/test/header-injection.php @@ -3,7 +3,7 @@ // Run with: php test/header-injection.php // Enable SnappyMail Api and include index file $_ENV['SNAPPYMAIL_INCLUDE_AS_API'] = true; -require '../index.php'; +require __DIR__ . '/../index.php'; function check(bool $condition, string $message): void { diff --git a/test/raw-inline-type.php b/test/raw-inline-type.php index 21431549f..dce5af4c3 100644 --- a/test/raw-inline-type.php +++ b/test/raw-inline-type.php @@ -3,7 +3,7 @@ // Run with: php test/raw-inline-type.php // Enable SnappyMail Api and include index file $_ENV['SNAPPYMAIL_INCLUDE_AS_API'] = true; -require '../index.php'; +require __DIR__ . '/../index.php'; function check(bool $condition, string $message): void { diff --git a/test/smime-verify.php b/test/smime-verify.php index 73f7d8ccf..6da2dc8e5 100644 --- a/test/smime-verify.php +++ b/test/smime-verify.php @@ -1,7 +1,7 @@ Date: Tue, 27 Oct 2026 22:27:03 +0100 Subject: [PATCH 10/11] @ZaphodB fix(security): two-factor-auth could be switched off with only a session With 2-step verification enabled, these plugin actions needed nothing but a session (and the CSRF token that comes with it): - EnableTwoFactor with Enable=0 switched it off, - ClearTwoFactorInfo deleted it, - CreateTwoFactorSecret overwrote the secret with Enable false, i.e. also switched it off, - ShowTwoFactorSecret returned the secret and QR code. A stolen cookie or an unlocked machine was enough to drop or copy the second factor. While 2FA is enabled, all four now require a current TOTP or backup code, checked as at login; a backup code used here is spent. Enabling and the first setup are unchanged. Backup codes come from random_int() instead of rand(). The settings page asks for the code (the existing "Verification Code" label) before those actions and sends it. A refused change reloads the real state; before, any error made the page show 2FA as not configured. A cancelled or refused "off" puts the checkbox back. TwoFactorAuthSettings.min.js, which is what is served, is now the same file as the source rather than a separate hand-minified copy. test/two-factor-reauth.php drives the real gate with storage and provider stubbed: open while not enabled; refused with no, a wrong or an unknown backup code while enabled; allowed with a valid TOTP or backup code, which is spent. --- plugins/two-factor-auth/index.php | 42 ++++++++++- .../js/TwoFactorAuthSettings.js | 59 ++++++++++++--- test/two-factor-reauth.php | 75 +++++++++++++++++++ 3 files changed, 164 insertions(+), 12 deletions(-) create mode 100644 test/two-factor-reauth.php diff --git a/plugins/two-factor-auth/index.php b/plugins/two-factor-auth/index.php index a080ae08b..515fea252 100644 --- a/plugins/two-factor-auth/index.php +++ b/plugins/two-factor-auth/index.php @@ -88,6 +88,34 @@ class TwoFactorAuthPlugin extends \RainLoop\Plugins\AbstractPlugin } } + /** + * Turning 2-step verification off, wiping it, replacing its secret or showing + * the secret all used to need only the session. Whoever held a session (a + * stolen cookie, an unlocked machine) could quietly drop the second factor or + * copy it. While it is enabled, these now need a current code or backup code, + * as login does. A used backup code is spent. + */ + private function requireCurrentCode(MainAccount $oAccount) : void + { + $aData = $this->getTwoFactorInfo($oAccount); + if (empty($aData['IsSet']) || empty($aData['Enable']) || empty($aData['Secret'])) { + return; + } + $sCode = \trim($this->jsonParam('Code', '')); + if (\strlen($sCode)) { + if ($this->TwoFactorAuthProvider($oAccount)->VerifyCode($aData['Secret'], $sCode)) { + return; + } + $aBackupCodes = \explode(' ', \trim(\preg_replace('/[^\d]+/', ' ', $aData['BackupCodes'] ?? ''))); + if (6 < \strlen($sCode) && \in_array($sCode, $aBackupCodes, true)) { + $this->removeBackupCodeFromTwoFactorInfo($oAccount, $sCode); + return; + } + } + $this->Logger()->Write("TFA: change refused for {$oAccount->Email()}, no valid current code"); + throw new ClientException(\RainLoop\Notifications::AuthError); + } + public function DoGetTwoFactorInfo() : array { $oAccount = $this->getMainAccountFromToken(); @@ -107,11 +135,15 @@ class TwoFactorAuthPlugin extends \RainLoop\Plugins\AbstractPlugin return $this->jsonResponse(__FUNCTION__, false); } + // Overwrites the stored secret with Enable false, i.e. also switches it off + $this->requireCurrentCode($oAccount); + $sEmail = $oAccount->Email(); $sSecret = $this->TwoFactorAuthProvider($oAccount)->CreateSecret(); - $aCodes = \array_map(function(){return \rand(100000000, 900000000);}, \array_fill(0, 8, null)); + // Backup codes are a second factor; rand() is not a CSPRNG + $aCodes = \array_map(fn() => \random_int(100000000, 899999999), \array_fill(0, 8, null)); $this->StorageProvider()->Put($oAccount, \RainLoop\Providers\Storage\Enumerations\StorageType::CONFIG, @@ -148,6 +180,8 @@ class TwoFactorAuthPlugin extends \RainLoop\Plugins\AbstractPlugin return $this->jsonResponse(__FUNCTION__, false); } + $this->requireCurrentCode($oAccount); + $aResult = $this->getTwoFactorInfo($oAccount); unset($aResult['BackupCodes']); @@ -164,6 +198,10 @@ class TwoFactorAuthPlugin extends \RainLoop\Plugins\AbstractPlugin return $this->jsonResponse(__FUNCTION__, false); } + if ('1' !== \trim($this->jsonParam('Enable', '0'))) { + $this->requireCurrentCode($oAccount); + } + $oActions = $this->Manager()->Actions(); if ($oActions->HasActionParam('EnableTwoFactor')) { $sValue = $oActions->GetActionParam('EnableTwoFactor', ''); @@ -215,6 +253,8 @@ class TwoFactorAuthPlugin extends \RainLoop\Plugins\AbstractPlugin return $this->jsonResponse(__FUNCTION__, false); } + $this->requireCurrentCode($oAccount); + $this->StorageProvider()->Clear($oAccount, \RainLoop\Providers\Storage\Enumerations\StorageType::CONFIG, 'two_factor' diff --git a/plugins/two-factor-auth/js/TwoFactorAuthSettings.js b/plugins/two-factor-auth/js/TwoFactorAuthSettings.js index 79d8c6bb5..c85940d37 100644 --- a/plugins/two-factor-auth/js/TwoFactorAuthSettings.js +++ b/plugins/two-factor-auth/js/TwoFactorAuthSettings.js @@ -22,8 +22,9 @@ const * @param {?Function} fCallback * @param {boolean} bEnable */ - enableTwoFactor(fCallback, bEnable) { + enableTwoFactor(fCallback, bEnable, oParams) { rl.pluginRemoteRequest(fCallback, 'EnableTwoFactor', { + ...oParams, Enable: bEnable ? 1 : 0 }); } @@ -61,8 +62,12 @@ class TwoFactorAuthSettings rl.settings.get('RequireTwoFactor') && rl.settings.set('SetupTwoFactor', !!iError); }, value); } else { - value || this.viewEnable_(value); - Remote.enableTwoFactor(fn, false); + // Turning it off needs a current code while it is on. + // Cancelled or refused, the checkbox goes back to on. + this.withCode(params => { + value || this.viewEnable_(value); + Remote.enableTwoFactor(iError => iError && this.viewEnable_(true), false, params); + }, () => this.viewEnable_.valueHasMutated()); } } }, @@ -86,12 +91,28 @@ class TwoFactorAuthSettings }).forEach(([key, fn]) => this[key] = ko.computed(fn)); this.onResult = this.onResult.bind(this); + this.onChangeResult = this.onChangeResult.bind(this); this.onShowSecretResult = this.onShowSecretResult.bind(this); } + /** + * While 2-step verification is on, the server wants a current code (or a + * backup code) to turn it off, clear it, replace it or show its secret. + */ + withCode(fDo, fCancel) { + if (!this.viewEnable_()) { + fDo({}); + } else { + const code = (prompt(rl.i18n('PLUGIN_2FA/LABEL_TWO_FACTOR_CODE')) || '').trim(); + code ? fDo({ Code: code }) : fCancel?.(); + } + } + showSecret() { - this.secreting(true); - rl.pluginRemoteRequest(this.onShowSecretResult, 'ShowTwoFactorSecret'); + this.withCode(params => { + this.secreting(true); + rl.pluginRemoteRequest(this.onShowSecretResult, 'ShowTwoFactorSecret', params); + }); } hideSecret() { @@ -101,8 +122,10 @@ class TwoFactorAuthSettings } createTwoFactor() { - this.processing(true); - rl.pluginRemoteRequest(this.onResult, 'CreateTwoFactorSecret'); + this.withCode(params => { + this.processing(true); + rl.pluginRemoteRequest(this.onChangeResult, 'CreateTwoFactorSecret', params); + }); } testTwoFactor() { @@ -115,12 +138,26 @@ class TwoFactorAuthSettings } clearTwoFactor() { - this.hideSecret(); + this.withCode(params => { + this.hideSecret(); - this.twoFactorTested(false); + this.twoFactorTested(false); - this.clearing(true); - rl.pluginRemoteRequest(this.onResult, 'ClearTwoFactorInfo'); + this.clearing(true); + rl.pluginRemoteRequest(this.onChangeResult, 'ClearTwoFactorInfo', params); + }); + } + + // A refused change must not show the account as unconfigured, which is what + // onResult does with an error; reload the real state instead. + onChangeResult(iError, oData) { + if (iError) { + this.processing(false); + this.clearing(false); + this.onBuild(); + } else { + this.onResult(iError, oData); + } } onShow() { diff --git a/test/two-factor-reauth.php b/test/two-factor-reauth.php new file mode 100644 index 000000000..cd9d5c826 --- /dev/null +++ b/test/two-factor-reauth.php @@ -0,0 +1,75 @@ +info; } + protected function TwoFactorAuthProvider(\RainLoop\Model\MainAccount $oAccount) : ?\TwoFactorAuthInterface + { + return new class implements \TwoFactorAuthInterface { + public function Label() : string { return 'test'; } + public function VerifyCode(string $sSecret, string $sCode) : bool { return 'SECRET' === $sSecret && '123456' === $sCode; } + public function CreateSecret() : string { return 'SECRET'; } + }; + } + protected function Logger() : \MailSo\Log\Logger { return new \MailSo\Log\Logger(false); } + protected function removeBackupCodeFromTwoFactorInfo(\RainLoop\Model\MainAccount $oAccount, string $sCode) : bool { $this->spent[] = $sCode; return true; } +} + +$plugin = (new ReflectionClass(TestTwoFactor::class))->newInstanceWithoutConstructor(); +$actions = (new ReflectionClass(\RainLoop\Actions::class))->newInstanceWithoutConstructor(); +$manager = (new ReflectionClass(\RainLoop\Plugins\Manager::class))->newInstanceWithoutConstructor(); +(new ReflectionProperty(\RainLoop\Plugins\Manager::class, 'oActions'))->setValue($manager, $actions); +(new ReflectionProperty(\RainLoop\Plugins\AbstractPlugin::class, 'oPluginManager'))->setValue($plugin, $manager); +$account = (new ReflectionClass(\RainLoop\Model\MainAccount::class))->newInstanceWithoutConstructor(); +$gate = (new ReflectionMethod(TwoFactorAuthPlugin::class, 'requireCurrentCode')); + +$params = new ReflectionProperty(\RainLoop\Actions::class, 'aCurrentActionParams'); +$allowed = function (array $values) use ($plugin, $actions, $account, $gate, $params) : bool { + $params->setValue($actions, $values); + try { + $gate->invoke($plugin, $account); + return true; + } catch (\RainLoop\Exceptions\ClientException $e) { + return false; + } +}; + +// Not set up, or set up but not enabled: nothing to protect yet. +$plugin->info = ['IsSet' => false, 'Enable' => false, 'Secret' => '']; +check($allowed([]), 'blocked although 2FA is not set up'); +$plugin->info = ['IsSet' => true, 'Enable' => false, 'Secret' => 'SECRET', 'BackupCodes' => '111111111']; +check($allowed([]), 'blocked although 2FA is not enabled'); + +// Enabled: a valid TOTP or backup code is required. +$plugin->info = ['IsSet' => true, 'Enable' => true, 'Secret' => 'SECRET', 'BackupCodes' => '111111111 222222222']; +check(!$allowed([]), 'change allowed with no code while enabled'); +check(!$allowed(['Code' => '000000']), 'change allowed with a wrong code'); +check(!$allowed(['Code' => '33333333']), 'change allowed with an unknown backup code'); +check($allowed(['Code' => '123456']), 'valid TOTP code refused'); +check($allowed(['Code' => '222222222']), 'valid backup code refused'); +check(['222222222'] === $plugin->spent, 'used backup code was not spent'); + +// Backup codes come from random_int now, not rand(). +$source = file_get_contents(dirname(__DIR__).'/plugins/two-factor-auth/index.php'); +check(!preg_match('/\\\\rand\(/', $source) && str_contains($source, '\\random_int('), 'backup codes not from random_int()'); + +echo "two-factor-reauth: ok\n"; From 099ee3b97ee1d592cdf00791874c37bd6965c0a4 Mon Sep 17 00:00:00 2001 From: djmaze <> Date: Tue, 27 Oct 2026 22:50:28 +0100 Subject: [PATCH 11/11] @ZaphodB fix(@security): S/MIME actions put the client's part id into IMAP FETCH as is DoSMimeDecryptMessage and DoSMimeVerifyMessage built BODY.PEEK[] (and [.MIME]) from the partId / sigPartId parameters without checking them, so a value could close the bracket or the line and add IMAP commands of its own. They run in the user's own IMAP session, so this is not cross-account, but nothing in that session should be steerable by a request parameter. Read both through smimePartIdParam(), which accepts only an IMAP section number (1, 1.2.10) or TEXT and refuses anything else. test/smime-partid.php checks valid ids pass and injection attempts, zero or leading-zero sections, lowercase and other section keywords are refused. --- .../app/libraries/RainLoop/Actions/SMime.php | 23 ++++++++++-- test/smime-partid.php | 37 +++++++++++++++++++ 2 files changed, 56 insertions(+), 4 deletions(-) create mode 100644 test/smime-partid.php diff --git a/snappymail/v/0.0.0/app/libraries/RainLoop/Actions/SMime.php b/snappymail/v/0.0.0/app/libraries/RainLoop/Actions/SMime.php index 124f3c974..00adaa387 100644 --- a/snappymail/v/0.0.0/app/libraries/RainLoop/Actions/SMime.php +++ b/snappymail/v/0.0.0/app/libraries/RainLoop/Actions/SMime.php @@ -9,6 +9,7 @@ use MailSo\Imap\Enumerations\FetchType; trait SMime { private $SMIME = null; + public function SMIME() : OpenSSL { if (!$this->SMIME) { @@ -84,7 +85,7 @@ trait SMime { $sFolderName = $this->GetActionParam('folder', ''); $iUid = (int) $this->GetActionParam('uid', 0); - $sPartId = $this->GetActionParam('partId', ''); + $sPartId = $this->smimePartIdParam('partId'); $sCertificate = $this->GetActionParam('certificate', ''); $sPrivateKey = $this->GetActionParam('privateKey', ''); $oPassphrase = new \SnappyMail\SensitiveString($this->GetActionParam('passphrase', '')); @@ -137,7 +138,7 @@ trait SMime public function DoSMimeVerifyMessage() : array { $sBody = $this->GetActionParam('bodyPart', ''); - $sPartId = $this->GetActionParam('partId', ''); + $sPartId = $this->smimePartIdParam('partId'); $bDetached = !empty($this->GetActionParam('detached', 0)); if (!$sBody && $sPartId) { $iUid = (int) $this->GetActionParam('uid', 0); @@ -152,7 +153,7 @@ trait SMime // Import the certificates automatically $sBody = $this->GetActionParam('sigPart', ''); - $sPartId = $this->GetActionParam('sigPartId', '') ?: $sPartId; + $sPartId = $this->smimePartIdParam('sigPartId') ?: $sPartId; if (!$sBody && $sPartId && $oImapClient) { $sBody = $oImapClient->Fetch( [FetchType::BODY_PEEK.'['.$sPartId.']'], @@ -189,7 +190,7 @@ trait SMime /* $sBody = $this->GetActionParam('sigPart', ''); if (!$sBody) { - $sPartId = $this->GetActionParam('sigPartId', '') ?: $this->GetActionParam('partId', ''); + $sPartId = $this->smimePartIdParam('sigPartId') ?: $this->smimePartIdParam('partId'); $this->initMailClientConnection(); $oImapClient = $this->ImapClient(); $oImapClient->FolderExamine($this->GetActionParam('folder', '')); @@ -212,4 +213,18 @@ trait SMime return $this->DefaultResponse($certificates); */ } + + /** + * A part id from the client goes into an IMAP FETCH section, BODY.PEEK[...], + * as is. Accept only a section number (1, 1.2.3) or TEXT, so the value cannot + * close the bracket or the line and add commands of its own. + */ + private function smimePartIdParam(string $sKey): string + { + $sPartId = \trim((string) $this->GetActionParam($sKey, '')); + if ('' !== $sPartId && !\preg_match('/^(TEXT|[1-9][0-9]*(\.[1-9][0-9]*)*)$/D', $sPartId)) { + throw new \RainLoop\Exceptions\ClientException(\RainLoop\Notifications::InvalidInputArgument); + } + return $sPartId; + } } diff --git a/test/smime-partid.php b/test/smime-partid.php new file mode 100644 index 000000000..2cdc80665 --- /dev/null +++ b/test/smime-partid.php @@ -0,0 +1,37 @@ +newInstanceWithoutConstructor(); +$params = new ReflectionProperty(\RainLoop\Actions::class, 'aCurrentActionParams'); +$method = new ReflectionMethod(\RainLoop\Actions::class, 'smimePartIdParam'); +$read = function (string $value) use ($actions, $params, $method) : ?string { + $params->setValue($actions, ['partId' => $value]); + try { + return $method->invoke($actions, 'partId'); + } catch (\RainLoop\Exceptions\ClientException $e) { + return null; + } +}; + +foreach (['', '1', '2', '1.2', '1.2.10', 'TEXT'] as $ok) { + check($ok === $read($ok), "valid part id '{$ok}' refused"); +} +foreach (["1]\r\nA1 LOGOUT\r\n", '1] BODY[HEADER', "1\r\nA1 LOGOUT", '1 2', '0', '1.0', '01', '1.', '.1', 'text', 'HEADER'] as $bad) { + check(null === $read($bad), 'part id '.json_encode($bad).' accepted'); +} +// Surrounding whitespace is trimmed away, and the trimmed value is what is used. +check('1' === $read("1\n"), 'trailing newline not trimmed'); + +echo "smime-partid: ok\n";