fix: validate ciphertext length before CBC/GCM decryption (Sentry W9)#9
Conversation
Add input validation to DecryptBytes to prevent panics from malformed or truncated SAML responses. crypto/cipher.CBCDecrypter.CryptBlocks panics with 'input not full blocks' if the ciphertext length is not a multiple of the block size. A malformed SAML response (e.g., truncated by a reverse proxy, corrupted in transit, or from a misconfigured IdP) can trigger this. Validations added: - CBC: ciphertext must be at least 2*blockSize (IV + one block minimum) - CBC: data after IV must be a multiple of the block size - CBC: PKCS#7 padding byte must be valid (0 < pad <= blockSize) - CBC: decrypted data must not be empty after zero-byte trimming - GCM: ciphertext must be at least nonceSize bytes Sentry: https://mattermost-mr.sentry.io/issues/7286219464/ (W9, SAML login crash) Co-authored-by: Claude <claude@anthropic.com>
The idpCertificate in test_constants.go expired 2026-02-09, causing TestSAML to fail with 'Cert is not valid at this time' instead of testing actual SAML validation logic. Fix: Pin the test clock to 2025-01-01 (within the cert's validity window) and replace dsig.RandomKeyStoreForTest() with a custom key store whose certificate is also valid at the pinned time. The two must overlap because the cert store contains both the idpCertificate and the test signing key's certificate. This is a workaround. The proper fix is to regenerate the test certificate with a longer validity period (see PR description). Co-authored-by: Claude <claude@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds strict input and padding validation to encrypted-assertion decryption paths (AES-GCM and CBC) and new tests covering truncated/invalid ciphertexts. Updates a SAML test to use a pinned-time keystore and fake clock for deterministic certificate/signature validity checks. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment Tip CodeRabbit can use Trivy to scan for security misconfigurations and secrets in Infrastructure as Code files.Add a .trivyignore file to your project to customize which findings Trivy reports. |
Distinct error messages for different CBC failure modes (truncated ciphertext, misaligned blocks, invalid PKCS#7 padding) would allow an attacker to perform a padding oracle attack by distinguishing padding failures from other errors. Consolidate all CBC decryption errors into a single generic message. Made-with: Cursor
esarafianou
left a comment
esarafianou
left a comment
There was a problem hiding this comment.
The distinct CBC error messages ("ciphertext too short", "not a multiple of block size", "invalid PKCS#7 padding") let an attacker distinguish padding failures from other errors, enabling block-by-block decryption of the assertion (padding oracle attack). This is exploitable when the SAMLResponse element is unsigned and only the SAMLAssertion is signed, since decryption runs before assertion signature validation.
Mattermost Server strips these errors in production, so Mattermost wouldn't be vulnerable to this attack but as gosaml2 under our Github org is an open source library, it should be secure by default.
Mattermost Server doesn't log the detailed error either, so having separate error messages wouldn't provide debugging value either.
I pushed a commit to consolidate the errors.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@saml_test.go`:
- Around line 178-180: The call to testKS.GetKeyPair() returns an error that is
ignored and then overwritten by the next assignment to err; after calling
GetKeyPair() (where you destructure to "_, _cert, err := testKS.GetKeyPair()")
check err immediately and handle it (e.g., t.Fatalf or return) before proceeding
to x509.ParseCertificate; ensure you do not reuse/overwrite the err without
validating it so that any failure from GetKeyPair() is surfaced in the test.
In `@types/encrypted_assertion_test.go`:
- Around line 17-29: Replace the flaky random-byte case in the test in
types/encrypted_assertion_test.go with a deterministic, known-bad PKCS#7 payload
and assert that DecryptBytes returns an error containing "failed to decrypt CBC
ciphertext"; specifically, construct a malformed PKCS#7/CMS block (invalid PKCS7
padding/CBC ciphertext) instead of crypto/rand-generated bytes, call
DecryptBytes (or the test helper that invokes it) with that payload, and change
the assertion to require the exact CBC decryption failure string so the
regression is deterministic and fails if DecryptBytes silently strips a
malformed trailer.
In `@types/encrypted_assertion.go`:
- Around line 90-104: The CBC unpadding currently trims trailing NULs and only
range-checks the final pad byte, which lets malformed PKCS#7 trailers through;
modify the logic in the unpadding code (working with variables data, padLength,
blockSize, cbcErr) to NOT call bytes.TrimRight before validation, then after
computing padLength validate that padLength > 0, padLength <= blockSize and
padLength <= len(data), and verify that the last padLength bytes all equal
byte(padLength); if any check fails return nil, cbcErr, otherwise return
data[:len(data)-padLength].
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d1c98147-cf45-4198-81e6-0ef319eb8cd5
📒 Files selected for processing (3)
saml_test.gotypes/encrypted_assertion.gotypes/encrypted_assertion_test.go
Made-with: Cursor
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai resolve |
✅ Actions performedComments resolved and changes approved. |
2 participants
Summary
Sentry: W9 — SAML login crash · 11 crashes · 1 affected instance (
anselm-mm.devproxy.linotp.de) · first seen 2026-02-23A malformed encrypted SAML assertion causes
crypto/cipher.CBCDecrypter.CryptBlocksto panic withinput not full blocks, crashing the server process. Every SAML login attempt from the affected IdP triggers the crash — users cannot log in and the server must be restarted.Changes
1. Input validation for DecryptBytes
Add bounds checks before calling
CryptBlocks/GCM.Openso malformed ciphertext returns a descriptive error instead of panicking.When does this happen?
client_max_body_size, proxy timeouts)CipherValue(algorithm mismatch, key size confusion, broken XML encryption)+chars URL-encoded inconsistently in SAML POST bindingValidations added
len(data) >= 2*blockSizelen(data) % blockSize == 0CryptBlockspanics otherwise0 < pad <= blockSizelen(data) >= nonceSizeOpen2. Fix expired test certificate (clock pinning)
The
idpCertificateintest_constants.goexpired 2026-02-09, silently breakingTestSAML. The last CI run onmasterwas 2025-06-02 — before the expiry. No commits since = no CI runs = undetected failure.Fix: Pin the test clock to
2025-01-01(within the cert's validity window) and use a custom key store whose certificate is also valid at the pinned time.The clock-pinning is a workaround. The proper fix is to regenerate the test certificate with a longer validity period.
Steps
openssl req -x509 -newkey rsa:2048 -keyout /dev/null -nodes \ -out idp_test_cert.pem -days 36500 \ -subj "/C=US/ST=California/L=San Francisco/O=Okta/OU=SSOProvider/CN=dev-116807/emailAddress=info@okta.com"Replace
idpCertificateintest_constants.gowith the new PEM.Audit tamper-detection tests —
manInTheMiddledResponseand similar embed the old cert's signature. Most tests re-sign at runtime so they'll work, but tamper tests may need their embeddedX509Certificateelements updated.Regenerate
testdata/test.crt+testdata/test.key(also expired, May 2016) and re-encrypttestdata/saml.post:openssl req -x509 -newkey rsa:2048 -keyout testdata/test.key -nodes \ -out testdata/test.crt -days 36500 -subj "/CN=test"saml_test.goand revert todsig.RandomKeyStoreForTest().Complexity estimate
Release Note
Fixed a server crash when processing malformed or truncated encrypted SAML assertions during login.
Co-authored-by: Claude claude@anthropic.com
Release Note