Commit45ef109
fix(security): SAML verification was fail-open and wrapping-vulnerable
fix(security): SAML verification was fail-open and wrapping-vulnerable Two defects, each allowing takeover of any account in an SSO-enabled org. 1. Fail-open. Signature verification ran only inside `if (idpCertPem && idpCertPem.trim())`. cfg.idpCertificate is nullable and is passed as `cfg.idpCertificate || ""`, so an org whose SAML config had no certificate skipped verification entirely and accepted any forged SAMLResponse. The assertion's email selects which account to log in as, so this was unauthenticated takeover. A missing cert is a broken config, never grounds to trust an assertion — now refused. 2. Signature wrapping. verifySamlSignature authenticates SignedInfo only; the Reference DigestValue is never recomputed against the element it covers. A genuinely-signed assertion captured from any real login could have its email rewritten, or a second forged Assertion added beside the signed one (attribute extraction regexes over the whole document), and still verify. Doing (2) properly needs exclusive XML canonicalisation. That is not implemented here and must not be hand-rolled — hand-rolled XML-DSig is how this bug class arises. No vetted library is currently a dependency, so this adds structural rejections of the shapes wrapping depends on, running BEFORE signature verification: more than one Assertion, no Assertion, a missing Reference, or a Reference URI covering neither the Response nor the Assertion present. verifySamlSignature is now labelled INCOMPLETE BY DESIGN so it is not mistaken for full XML-DSig. Also: the algorithm regex /Algorithm="([^"]+)"/ matched the first Algorithm attribute in the document, which in a normal response is CanonicalizationMethod — the real SignatureMethod was never read and it silently fell back to SHA1. Now scoped to SignatureMethod inside SignedInfo, and an unrecognised algorithm is refused rather than defaulting to SHA1. FOLLOW-UP REQUIRED: adopt xml-crypto (or equivalent) for real DigestValue verification with c14n. Until then SAML SSO should be considered mitigated, not fixed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 files changed+203−1245ef1095079ac229b42565a6e9529d1144abd0f2
2 changed files+203−12
Addedsrc/__tests__/saml-signature.test.ts+82−0View fileUnifiedSplit
@@ -0,0 +1,82 @@
1/**
2 * SAML assertion verification.
3 *
4 * Two defects, both allowing account takeover:
5 *
6 * 1. Fail-open. Signature verification ran only `if (idpCertPem && trim())`,
7 * so an org whose SAML config had a blank idp_certificate accepted ANY
8 * forged SAMLResponse. The assertion's email selects the account to log
9 * in as, so that is unauthenticated takeover of any account in the org.
10 *
11 * 2. Signature wrapping. verifySamlSignature checks only that SignedInfo
12 * carries a valid RSA signature; the Reference DigestValue is never
13 * recomputed. A genuinely-signed assertion could be edited — or a second
14 * forged Assertion added beside the signed one — and still verify.
15 *
16 * Full XML-DSig needs exclusive c14n and a vetted library. These lock in the
17 * fail-closed behaviour and the structural anti-wrapping checks that stand in
18 * until then.
19 */
20
21import { describe, expect, it } from "bun:test";
22import { readFileSync } from "fs";
23
24const SRC = readFileSync("src/routes/saml-sso.tsx", "utf8");
25
26describe("verification is mandatory (fail-closed)", () => {
27 it("refuses when no IdP certificate is configured", () => {
28 expect(SRC).toContain("if (!idpCertPem || !idpCertPem.trim())");
29 expect(SRC).toContain("Refusing to accept an unverified assertion");
30 });
31
32 it("no longer makes verification conditional on the cert being present", () => {
33 // The original bug shape.
34 expect(SRC).not.toMatch(/if \(idpCertPem && idpCertPem\.trim\(\)\) \{\s*\n\s*const sigValid/);
35 });
36});
37
38describe("anti-wrapping structural checks", () => {
39 it("runs before signature verification", () => {
40 const coverage = SRC.indexOf("assertSignatureCoversAssertion(xml)");
41 const verify = SRC.indexOf("if (!verifySamlSignature(xml, idpCertPem))");
42 expect(coverage).toBeGreaterThan(-1);
43 expect(verify).toBeGreaterThan(-1);
44 // A rewritten-but-validly-signed document must not reach attribute
45 // extraction just because SignedInfo checks out.
46 expect(coverage).toBeLessThan(verify);
47 });
48
49 it("rejects multiple Assertions", () => {
50 expect(SRC).toContain("assertionIds.length > 1");
51 expect(SRC).toContain("signature-wrapping attempt");
52 });
53
54 it("rejects a Reference that covers neither the Response nor the Assertion", () => {
55 expect(SRC).toContain("referenced !== assertionIds[0] && referenced !== responseId");
56 });
57
58 it("rejects a response with no Assertion at all", () => {
59 expect(SRC).toContain("assertionIds.length === 0");
60 });
61});
62
63describe("signature algorithm selection", () => {
64 it("reads Algorithm from SignatureMethod, not the first match in the doc", () => {
65 // The old regex /Algorithm="([^"]+)"/ matched CanonicalizationMethod,
66 // which appears first in a normal response — so the real algorithm was
67 // never read and it silently fell back to SHA1.
68 expect(SRC).toContain("SignatureMethod[^>]*\\bAlgorithm=");
69 expect(SRC).not.toMatch(/const algMatch = xml\.match\(\/Algorithm=/);
70 });
71
72 it("refuses an unrecognised algorithm instead of defaulting to SHA1", () => {
73 expect(SRC).toContain("unrecognised SignatureMethod");
74 });
75});
76
77describe("the incompleteness is documented, not hidden", () => {
78 it("verifySamlSignature is marked as not proving assertion integrity", () => {
79 // Whoever reads this next must not mistake it for full XML-DSig.
80 expect(SRC).toContain("INCOMPLETE BY DESIGN");
81 });
82});
Modifiedsrc/routes/saml-sso.tsx+121−12View fileUnifiedSplit
@@ -522,12 +522,31 @@ function parseSamlResponse(
522522 return { ok: false, error: `SAML error: ${msg}` };
523523 }
524524
525 // Signature verification — only if IdP cert provided
526 if (idpCertPem && idpCertPem.trim()) {
527 const sigValid = verifySamlSignature(xml, idpCertPem);
528 if (!sigValid) {
529 return { ok: false, error: "SAML signature verification failed" };
530 }
525 // Signature verification — MANDATORY.
526 //
527 // This was `if (idpCertPem && idpCertPem.trim())`, i.e. an org whose SAML
528 // config had a blank idp_certificate skipped verification entirely and any
529 // forged SAMLResponse was accepted. Since the assertion's email is what
530 // selects the account to log in as, that was unauthenticated takeover of
531 // any account in the org. An unconfigured cert is a broken configuration,
532 // never a reason to trust the assertion.
533 if (!idpCertPem || !idpCertPem.trim()) {
534 return {
535 ok: false,
536 error:
537 "SAML is not fully configured for this organization (no IdP certificate). Refusing to accept an unverified assertion.",
538 };
539 }
540
541 // Reject signature-wrapping shapes BEFORE checking the signature, so a
542 // validly-signed-but-rewritten document cannot reach attribute extraction.
543 const coverage = assertSignatureCoversAssertion(xml);
544 if (!coverage.ok) {
545 return { ok: false, error: coverage.error };
546 }
547
548 if (!verifySamlSignature(xml, idpCertPem)) {
549 return { ok: false, error: "SAML signature verification failed" };
531550 }
532551
533552 // Extract attributes
@@ -596,13 +615,91 @@ function extractSamlAttributes(xml: string): Record<string, string> {
596615 return attrs;
597616}
598617
618/**
619 * Structural defence against XML signature wrapping.
620 *
621 * verifySamlSignature() below checks only that SignedInfo carries a valid
622 * RSA signature. It never verifies the Reference DigestValue against the
623 * element the digest is supposed to cover — so a genuinely-signed assertion
624 * captured from any legitimate login could be edited (swap the email for a
625 * victim's, or bolt a second forged Assertion alongside the signed one) and
626 * still pass, because SignedInfo itself was untouched.
627 *
628 * Validating the digest properly requires exclusive XML canonicalisation
629 * (c14n), which is not implemented here and must not be hand-rolled — doing
630 * so is precisely how this bug class arises. Until a vetted library such as
631 * `xml-crypto` is adopted, reject the document shapes wrapping attacks
632 * depend on:
633 *
634 * 1. More than one Assertion element. Attribute extraction below regexes
635 * over the whole document, so a second assertion can shadow the signed
636 * one. A legitimate SSO response carries exactly one.
637 * 2. A Reference URI that does not point at the Response or the Assertion
638 * actually present, i.e. the signature covers something else entirely.
639 *
640 * This is a mitigation, not a proof of integrity. See the SAML section of the
641 * audit notes: full XML-DSig verification is still outstanding.
642 */
643function assertSignatureCoversAssertion(
644 xml: string
645): { ok: true } | { ok: false; error: string } {
646 const assertionIds = [
647 ...xml.matchAll(/<(?:saml2?:)?Assertion\s[^>]*\bID="([^"]+)"/gi),
648 ].map((m) => m[1]);
649
650 if (assertionIds.length === 0) {
651 return { ok: false, error: "SAML response contains no Assertion" };
652 }
653 if (assertionIds.length > 1) {
654 return {
655 ok: false,
656 error:
657 "SAML response contains multiple Assertions — rejected as a signature-wrapping attempt",
658 };
659 }
660
661 const responseIdMatch = xml.match(
662 /<(?:samlp?2?:)?Response\s[^>]*\bID="([^"]+)"/i
663 );
664 const responseId = responseIdMatch ? responseIdMatch[1] : null;
665
666 // The Reference inside SignedInfo names what the digest covers.
667 const signedInfo = xml.match(
668 /<(?:ds:)?SignedInfo[\s\S]*?<\/(?:ds:)?SignedInfo>/i
669 );
670 if (!signedInfo) {
671 return { ok: false, error: "SAML signature has no SignedInfo" };
672 }
673 const refMatch = signedInfo[0].match(
674 /<(?:ds:)?Reference[^>]*\bURI="#([^"]*)"/i
675 );
676 if (!refMatch) {
677 return {
678 ok: false,
679 error: "SAML signature Reference does not identify a signed element",
680 };
681 }
682 const referenced = refMatch[1];
683
684 if (referenced !== assertionIds[0] && referenced !== responseId) {
685 return {
686 ok: false,
687 error:
688 "SAML signature covers an element other than the Response or Assertion — rejected",
689 };
690 }
691
692 return { ok: true };
693}
694
599695/**
600696 * Verify the XML-DSig signature in the SAML response against the IdP certificate.
601697 * Uses Node's built-in `crypto.createVerify` to avoid heavy deps.
602698 *
603 * This is a best-effort verification: it handles the common case where the
604 * entire Response or Assertion element is signed. For full enveloped-signature
605 * support, use a library like `xml-crypto`.
699 * INCOMPLETE BY DESIGN — read assertSignatureCoversAssertion() above before
700 * relying on this. It authenticates SignedInfo only; the Reference
701 * DigestValue is never recomputed, so this proves an IdP signed *something*,
702 * not that it signed the assertion being consumed.
606703 */
607704function verifySamlSignature(xml: string, certPem: string): boolean {
608705 try {
@@ -623,13 +720,25 @@ function verifySamlSignature(xml: string, certPem: string): boolean {
623720 if (!signedInfoMatch) return false;
624721 const signedInfoXml = signedInfoMatch[1];
625722
626 // Detect algorithm from SignatureMethod
627 const algMatch = xml.match(/Algorithm="([^"]+)"/);
723 // Detect the algorithm from SignatureMethod specifically. This matched
724 // the first Algorithm attribute anywhere in the document, which in a
725 // normal SAML response is CanonicalizationMethod — so the real signature
726 // algorithm was usually never read and it silently fell back to SHA1.
727 const algMatch = signedInfoXml.match(
728 /<(?:ds:)?SignatureMethod[^>]*\bAlgorithm="([^"]+)"/i
729 );
628730 const alg = algMatch ? algMatch[1] : "";
629731 let hashAlg: string;
630732 if (alg.includes("sha256")) hashAlg = "SHA256";
631733 else if (alg.includes("sha512")) hashAlg = "SHA512";
632 else hashAlg = "SHA1";
734 else if (alg.includes("sha384")) hashAlg = "SHA384";
735 else if (alg.includes("sha1")) hashAlg = "SHA1";
736 else {
737 // Unknown or absent: refuse rather than guessing SHA1, which would
738 // accept a downgraded signature.
739 console.warn("[saml-sso] unrecognised SignatureMethod:", alg);
740 return false;
741 }
633742
634743 // Normalise the PEM cert
635744 let pem = certPem.trim();
636745