From 6ee13ababb99cbc74c57e78b00ab8cd92bdb5b32 Mon Sep 17 00:00:00 2001 From: SecureSAML Date: Mon, 26 May 2025 12:34:15 -0400 Subject: [PATCH 1/4] Refactor to support encrypted assertions - Update CHANGELOG.md --- CHANGELOG.md | 10 ++++++++++ package.json | 5 +++-- src/flow.ts | 47 ++++++++++++++++++++++++----------------------- src/libsaml.ts | 31 ++++++++++++++++++------------- test/flow.ts | 10 +++------- test/index.ts | 21 ++++++--------------- test/issues.ts | 8 ++------ yarn.lock | 20 ++++++++++++++++---- 8 files changed, 82 insertions(+), 70 deletions(-) create mode 100644 CHANGELOG.md diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 00000000..ded3427f --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,10 @@ +# 2.10.1 + +* Adds @authenio/samlify-xsd-schema-validator as dependency by default +This is to support running test cases + +* Changes to libsaml.ts verifySignature. This is an internal function, but we still document changes + - Does not raise error when signature is missing/invalid. Instead it now returns false. This is to simplify logic + - When there are encrypted assertions, returns the entire response, as the "verifiedAssertionNode" + +* Update logic around handling encrypted assertions diff --git a/package.json b/package.json index 8a8a9df8..135f78dd 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "samlify", - "version": "2.10.0", + "version": "2.10.1", "description": "High-level API for Single Sign On (SAML 2.0)", "main": "build/index.js", "keywords": [ @@ -31,6 +31,7 @@ }, "license": "MIT", "dependencies": { + "@authenio/samlify-xsd-schema-validator": "^1.0.5", "@authenio/xml-encryption": "^2.0.2", "@xmldom/xmldom": "^0.8.6", "camelcase": "^6.2.0", @@ -39,7 +40,7 @@ "pako": "^1.0.10", "uuid": "^8.3.2", "xml": "^1.0.1", - "xml-crypto": "^6.1.0", + "xml-crypto": "^6.1.1", "xml-escape": "^1.1.0", "xpath": "^0.0.32" }, diff --git a/src/flow.ts b/src/flow.ts index f58a5360..d154996a 100644 --- a/src/flow.ts +++ b/src/flow.ts @@ -208,34 +208,35 @@ async function postFlow(options): Promise { // verify the signatures (the response is encrypted then signed, then verify first then decrypt) if ( - checkSignature && - from.entitySetting.messageSigningOrder === MessageSignatureOrder.ETS + checkSignature ) { + // VerifiedAssertionNode is signed. Depending on use case, it may actually be a Response Node const [verified, verifiedAssertionNode] = libsaml.verifySignature(samlContent, verificationOptions); - if (!verified) { - return Promise.reject('ERR_FAIL_TO_VERIFY_ETS_SIGNATURE'); - } - if (!decryptRequired) { - extractorFields = getDefaultExtractorFields(parserType, verifiedAssertionNode); - } - } - if (parserType === 'SAMLResponse' && decryptRequired) { - const result = await libsaml.decryptAssertion(self, samlContent); - samlContent = result[0]; - extractorFields = getDefaultExtractorFields(parserType, result[1]); - } - - // verify the signatures (the response is signed then encrypted, then decrypt first then verify) - if ( - checkSignature && - from.entitySetting.messageSigningOrder === MessageSignatureOrder.STE - ) { - const [verified, verifiedAssertionNode] = libsaml.verifySignature(samlContent, verificationOptions); - if (verified) { + // First two cases are encrypted assertion cases + // This case the verifiedAssertionNode is actually a response + if (decryptRequired && verified && parserType === 'SAMLResponse' && verifiedAssertionNode) { + // now it is extracted from solely signed contents + const result = await libsaml.decryptAssertion(self, verifiedAssertionNode); + samlContent = result[0]; + // extractor depends on signed content + extractorFields = getDefaultExtractorFields(parserType, result[1]); + } else if (decryptRequired && !verified) { + // Encrypted Assertion, the assertion is signed + const result = await libsaml.decryptAssertion(self, samlContent); + const decryptedDoc = result[0]; + const [decryptedDocVerified, verifiedDecryptedAssertion] = libsaml.verifySignature(decryptedDoc, verificationOptions); + if (decryptedDocVerified) { + // extractor depends on signed content + extractorFields = getDefaultExtractorFields(parserType, verifiedDecryptedAssertion); + } else { + return Promise.reject('FAILED_TO_VERIFY_SIGNATURE'); + } + } else if (verified) { + // extractor depends on signed content extractorFields = getDefaultExtractorFields(parserType, verifiedAssertionNode); } else { - return Promise.reject('ERR_FAIL_TO_VERIFY_STE_SIGNATURE'); + return Promise.reject('FAILED_TO_VERIFY_SIGNATURE'); } } diff --git a/src/libsaml.ts b/src/libsaml.ts index aa9691a4..bdb48356 100644 --- a/src/libsaml.ts +++ b/src/libsaml.ts @@ -366,7 +366,7 @@ const libSaml = () => { * - The first element is `true` if the signature is valid, `false` otherwise. * - The second element is the cryptographically authenticated assertion node as a string, or `null` if not found. */ - verifySignature(xml: string, opts: SignatureVerifierOptions) { + verifySignature(xml: string, opts: SignatureVerifierOptions) : [boolean, string | null] { const { dom } = getContext(); const doc = dom.parseFromString(xml); @@ -395,10 +395,9 @@ const libSaml = () => { // guarantee to have a signature in saml response if (selection.length === 0) { - throw new Error('ERR_ZERO_SIGNATURE'); + return [false, null]; // we return false now } - // need to refactor later on for (const signatureNode of selection){ const sig = new SignedXml(); @@ -457,18 +456,15 @@ const libSaml = () => { sig.loadSignature(signatureNode); - doc.removeChild(signatureNode); - verified = sig.checkSignature(doc.toString()); // immediately throw error when any one of the signature is failed to get verified if (!verified) { - throw new Error('ERR_FAILED_TO_VERIFY_SIGNATURE'); + continue; + // throw new Error('ERR_FAILED_TO_VERIFY_SIGNATURE'); } - // attempt is made to get the signed Reference as a string(); - // note, we don't have access to the actual signedReferences API unfortunately - // mainly a sanity check here for SAML. (Although ours would still be secure, if multiple references are used) - if (!(sig.getReferences().length >= 1)) { + // Require there to be at least one reference that was signed + if (!(sig.getSignedReferences().length >= 1)) { throw new Error('NO_SIGNATURE_REFERENCES') } const signedVerifiedXML = sig.getSignedReferences()[0]; @@ -476,15 +472,25 @@ const libSaml = () => { // process the verified signature: // case 1, rootSignedDoc is a response: if (rootNode.localName === 'Response') { - // try getting the Xml from the first assertion const assertions = select( "./*[local-name()='Assertion']", rootNode ); + + const encryptedAssertions = select( + "./*[local-name()='EncryptedAssertion']", + rootNode + ); // now we can process the assertion as an assertion if (assertions.length === 1) { return [true, assertions[0].toString()]; + } else if (encryptedAssertions.length >= 1) { + return [true, rootNode.toString()]; // we need to return a Response node, which will be decrypted later + } else { + // something has gone seriously wrong here. + // we don't have any assertion to give back + return [true, null] } } else if (rootNode.localName === 'Assertion') { return [true, rootNode.toString()]; @@ -492,9 +498,8 @@ const libSaml = () => { return [true, null]; // signature is valid. But there is no assertion node here. It could be metadata node, hence return null } }; + return [false, null]; // we didn't verify anything, none of the signatures are valid - // something has gone seriously wrong if we are still here - throw new Error('ERR_ZERO_SIGNATURE'); /* // response must be signed, either entire document or assertion diff --git a/test/flow.ts b/test/flow.ts index b179cd91..75481710 100644 --- a/test/flow.ts +++ b/test/flow.ts @@ -1199,7 +1199,7 @@ test('should reject signature wrapped response - case 1', async t => { } }); -test('should reject signature wrapped response - case 2', async t => { +test('should use signed contents in signature wrapped response - case 2', async t => { // const user = { email: 'user@esaml2.com' }; const { id, context: SAMLResponse } = await idpNoEncrypt.createLoginResponse(sp, sampleRequestInfo, 'post', user, createTemplateCallback(idpNoEncrypt, sp, binding.post, user)); @@ -1216,12 +1216,8 @@ test('should reject signature wrapped response - case 2', async t => { //Put stripped version under SubjectConfirmationData of modified version const xmlWrapped = outer.replace(/<\/saml:Conditions>/, '' + stripped.replace('', '') + ''); const wrappedResponse = Buffer.from(xmlWrapped).toString('base64'); - try { - await sp.parseLoginResponse(idpNoEncrypt, 'post', { body: { SAMLResponse: wrappedResponse } }); - t.fail(); - } catch (e) { - t.is(e.message, 'ERR_POTENTIAL_WRAPPING_ATTACK'); - } + const {extract} = await sp.parseLoginResponse(idpNoEncrypt, 'post', { body: { SAMLResponse: wrappedResponse } }); + t.is(extract.nameID, 'user@esaml2.com'); }); test('should throw two-tiers code error when the response does not return success status', async t => { diff --git a/test/index.ts b/test/index.ts index e5ad77cc..296b2dc4 100644 --- a/test/index.ts +++ b/test/index.ts @@ -208,31 +208,22 @@ test('getAssertionConsumerService with two bindings', t => { t.is(libsaml.verifySignature(_decodedResponse, { metadata: IdPMetadata })[0], true); }); test('integrity check for request signed with RSA-SHA1', t => { - try { - libsaml.verifySignature(_falseDecodedRequestSHA1, { metadata: SPMetadata, signatureAlgorithm: signatureAlgorithms.RSA_SHA1 }); - } catch (e) { - t.is(e.message, 'ERR_FAILED_TO_VERIFY_SIGNATURE'); - } + const [verified, verifiedData] = libsaml.verifySignature(_falseDecodedRequestSHA1, { metadata: SPMetadata, signatureAlgorithm: signatureAlgorithms.RSA_SHA1 }); + t.is(verified, false); }); test('verify a XML signature signed by RSA-SHA256 with metadata', t => { t.is(libsaml.verifySignature(_decodedRequestSHA256, { metadata: SPMetadata, signatureAlgorithm: signatureAlgorithms.RSA_SHA256 })[0], true); }); test('integrity check for request signed with RSA-SHA256', t => { - try { - libsaml.verifySignature(_falseDecodedRequestSHA256, { metadata: SPMetadata, signatureAlgorithm: signatureAlgorithms.RSA_SHA256 }); - } catch (e) { - t.is(e.message, 'ERR_FAILED_TO_VERIFY_SIGNATURE'); - } + const [verified, verifiedData] = libsaml.verifySignature(_falseDecodedRequestSHA256, { metadata: SPMetadata, signatureAlgorithm: signatureAlgorithms.RSA_SHA256 }); + t.is(verified, false); }); test('verify a XML signature signed by RSA-SHA512 with metadata', t => { t.is(libsaml.verifySignature(_decodedRequestSHA512, { metadata: SPMetadata, signatureAlgorithm: signatureAlgorithms.RSA_SHA512 })[0], true); }); test('integrity check for request signed with RSA-SHA512', t => { - try { - libsaml.verifySignature(_falseDecodedRequestSHA512, { metadata: SPMetadata, signatureAlgorithm: signatureAlgorithms.RSA_SHA512 }); - } catch (e) { - t.is(e.message, 'ERR_FAILED_TO_VERIFY_SIGNATURE'); - } + const [verified, verifiedData] = libsaml.verifySignature(_falseDecodedRequestSHA512, { metadata: SPMetadata, signatureAlgorithm: signatureAlgorithms.RSA_SHA512 }); + t.is(verified, false); }); test('verify a XML signature with metadata but with rolling certificate', t => { diff --git a/test/issues.ts b/test/issues.ts index 90683e16..cbccad66 100644 --- a/test/issues.ts +++ b/test/issues.ts @@ -141,12 +141,8 @@ test('#31 query param for sso/slo returns error', t => { }); test('#87 add existence check for signature verification', t => { - try { - libsaml.verifySignature(readFileSync('./test/misc/response.xml').toString(), {}); - t.fail(); - } catch ({ message }) { - t.is(message, 'ERR_ZERO_SIGNATURE'); - } + const res = libsaml.verifySignature(readFileSync('./test/misc/response.xml').toString(), {}); + t.is(res[0], false) // signature is invalid because one doesn't exist }); test('#91 idp gets single sign on service from the metadata', t => { diff --git a/yarn.lock b/yarn.lock index 972399c2..5bd46fcc 100644 --- a/yarn.lock +++ b/yarn.lock @@ -10,6 +10,13 @@ "@jridgewell/gen-mapping" "^0.1.0" "@jridgewell/trace-mapping" "^0.3.9" +"@authenio/samlify-xsd-schema-validator@^1.0.5": + version "1.0.5" + resolved "https://registry.yarnpkg.com/@authenio/samlify-xsd-schema-validator/-/samlify-xsd-schema-validator-1.0.5.tgz#bef7fe43928714e473cd95e1577b46d028b945ed" + integrity sha512-HJjmjM1WbeB/z4nVbYEcmtIWTLPKqjrqRGEpC9lu7s03Usc4nxxfrJGjHgh3M8MvBJy4neVUoeM9rP4ym3GLgg== + dependencies: + "@authenio/xsd-schema-validator" "^0.7.3" + "@authenio/xml-encryption@^2.0.2": version "2.0.2" resolved "https://registry.yarnpkg.com/@authenio/xml-encryption/-/xml-encryption-2.0.2.tgz#df1f491dacb9b1f65bc7a9a554c189644f72bbe0" @@ -19,6 +26,11 @@ escape-html "^1.0.3" xpath "0.0.32" +"@authenio/xsd-schema-validator@^0.7.3": + version "0.7.3" + resolved "https://registry.yarnpkg.com/@authenio/xsd-schema-validator/-/xsd-schema-validator-0.7.3.tgz#abbf5710705bfab3394aca8b9d5a9e8429873897" + integrity sha512-Jhc/Hxv90bacZr0Fv+u+PEb440zPh4mO6rw+bzEAIBiFLKCtRa/BvKGRxPdCAwsGRPuwl2hFqQGF+Lfz6Q8kFg== + "@ava/typescript@^1.1.1": version "1.1.1" resolved "https://registry.yarnpkg.com/@ava/typescript/-/typescript-1.1.1.tgz#3dcaba3aced8026fdb584d927d809752854dc6e6" @@ -2522,10 +2534,10 @@ write-file-atomic@^4.0.1: imurmurhash "^0.1.4" signal-exit "^3.0.7" -xml-crypto@^6.1.0: - version "6.1.0" - resolved "https://registry.yarnpkg.com/xml-crypto/-/xml-crypto-6.1.0.tgz#c8224808525e5f15478c50b9fe706112a4e6ef1b" - integrity sha512-0TYPBRPwXLnRGc2F0f9Zc/H076YcP7tkCa2US4jpguuPTEx7TWFqSysIfJ1hP4r2KF82IYzhnzepnsUEsOjlOw== +xml-crypto@^6.1.1: + version "6.1.2" + resolved "https://registry.yarnpkg.com/xml-crypto/-/xml-crypto-6.1.2.tgz#ed93e87d9538f92ad1ad2db442e9ec586723d07d" + integrity sha512-leBOVQdVi8FvPJrMYoum7Ici9qyxfE4kVi+AkpUoYCSXaQF4IlBm1cneTK9oAxR61LpYxTx7lNcsnBIeRpGW2w== dependencies: "@xmldom/is-dom-node" "^1.0.1" "@xmldom/xmldom" "^0.8.10" From 43dbb40c6e72101006f1137e3571d6fd3ad6f3fa Mon Sep 17 00:00:00 2001 From: Alexander Tan Date: Tue, 3 Jun 2025 13:29:40 -0400 Subject: [PATCH 2/4] Refactor: move authenio to devDependencies --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 135f78dd..16216062 100644 --- a/package.json +++ b/package.json @@ -31,7 +31,6 @@ }, "license": "MIT", "dependencies": { - "@authenio/samlify-xsd-schema-validator": "^1.0.5", "@authenio/xml-encryption": "^2.0.2", "@xmldom/xmldom": "^0.8.6", "camelcase": "^6.2.0", @@ -45,6 +44,7 @@ "xpath": "^0.0.32" }, "devDependencies": { + "@authenio/samlify-xsd-schema-validator": "^1.0.5", "@ava/typescript": "^1.1.1", "@types/node": "^11.11.3", "@types/node-forge": "^1.0.1", From 0702e7eae335bbd528d31dab63bd3c52159532b1 Mon Sep 17 00:00:00 2001 From: Alexander Tan Date: Tue, 3 Jun 2025 13:31:24 -0400 Subject: [PATCH 3/4] Upgrade xml-crypto to 6.1.2 --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 16216062..0361dc41 100644 --- a/package.json +++ b/package.json @@ -39,7 +39,7 @@ "pako": "^1.0.10", "uuid": "^8.3.2", "xml": "^1.0.1", - "xml-crypto": "^6.1.1", + "xml-crypto": "^6.1.2", "xml-escape": "^1.1.0", "xpath": "^0.0.32" }, From 59bf3df3cf2c7882613c674f25ccb41e03f75472 Mon Sep 17 00:00:00 2001 From: Alexander Tan Date: Tue, 3 Jun 2025 13:32:22 -0400 Subject: [PATCH 4/4] Fix: CHANGELOG.md --- CHANGELOG.md | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ded3427f..45e7acde 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,10 +1,7 @@ # 2.10.1 -* Adds @authenio/samlify-xsd-schema-validator as dependency by default -This is to support running test cases - * Changes to libsaml.ts verifySignature. This is an internal function, but we still document changes - Does not raise error when signature is missing/invalid. Instead it now returns false. This is to simplify logic - When there are encrypted assertions, returns the entire response, as the "verifiedAssertionNode" -* Update logic around handling encrypted assertions +* Fix logic around handling encrypted assertions