From 3776ac6b32f9c6f7bac2f67595fdcf2e973d0095 Mon Sep 17 00:00:00 2001 From: the-djmaze <> Date: Sat, 24 Oct 2026 11:57:38 +0200 Subject: [PATCH] @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";