From 7378e60225ccb8c586a2ab90b01d73699d216cf9 Mon Sep 17 00:00:00 2001 From: sultanhq Date: Wed, 30 Apr 2025 12:39:13 +0100 Subject: [PATCH] fix: TypeError when parsing login response #399 fix: throw when cert is null #397 Co-authored-by: rassul-findmypast Co-authored-by: JAParsons Co-authored-by: Jonathan Darrer Co-authored-by: Matt Krick --- src/extractor.ts | 25 ++++ src/libsaml.ts | 3 + src/metadata.ts | 14 ++- test/index.ts | 28 ++++- test/libsaml.ts | 111 ++++++++++++++++++ .../misc/idpmeta_multiple_anonymous_certs.xml | 41 +++++++ ...eta_one_signing_and_one_anonymous_cert.xml | 44 +++++++ 7 files changed, 262 insertions(+), 4 deletions(-) create mode 100644 test/libsaml.ts create mode 100644 test/misc/idpmeta_multiple_anonymous_certs.xml create mode 100644 test/misc/idpmeta_one_signing_and_one_anonymous_cert.xml diff --git a/src/extractor.ts b/src/extractor.ts index 2cbf8c24..fc97e11d 100644 --- a/src/extractor.ts +++ b/src/extractor.ts @@ -209,6 +209,7 @@ export function extract(context: string, fields) { const shortcut = field.shortcut; // get optional fields const index = field.index; + const excludeAttribute = field.excludeAttribute; const attributePath = field.attributePath; // set allowing overriding if there is a shortcut injected @@ -361,6 +362,30 @@ export function extract(context: string, fields) { [key]: attributeValues[0] }; } + + // case: single excluded attribute + /* + { + key: 'sharedCertificate', + excludeAttribute: 'use', + localPath: ['EntityDescriptor', '~SSODescriptor', 'KeyDescriptor'], + attributePath: ['KeyInfo', 'X509Data', 'X509Certificate'], + attributes: [], + } + */ + if (excludeAttribute) { + const fullPath = `${baseXPath}[not(@${excludeAttribute})]${buildAbsoluteXPath(attributePath)}`; + const node = select(fullPath, targetDoc); + return { + ...result, + [key]: node.length > 0 + ? node + .filter((n: Node) => n.firstChild) + .map((n: Node) => n.firstChild!.nodeValue) + : undefined + }; + } + // case: zero attribute /* { diff --git a/src/libsaml.ts b/src/libsaml.ts index 2ce6d217..36052c59 100644 --- a/src/libsaml.ts +++ b/src/libsaml.ts @@ -412,6 +412,9 @@ const libSaml = () => { const certificateNode = select(".//*[local-name(.)='X509Certificate']", signatureNode) as any; // certificate in metadata let metadataCert: any = opts.metadata.getX509Certificate(certUse.signing); + if (!metadataCert) { + throw new Error('INVALID_CERTIFICATE_PROVIDED') + } // flattens the nested array of Certificates from each KeyDescriptor if (Array.isArray(metadataCert)) { metadataCert = flattenDeep(metadataCert); diff --git a/src/metadata.ts b/src/metadata.ts index ec654887..601fa98d 100644 --- a/src/metadata.ts +++ b/src/metadata.ts @@ -45,7 +45,9 @@ export default class Metadata implements MetadataInterface { { // shared certificate for both encryption and signing key: 'sharedCertificate', - localPath: ['EntityDescriptor', '~SSODescriptor', 'KeyDescriptor', 'KeyInfo', 'X509Data', 'X509Certificate'], + excludeAttribute: 'use', + localPath: ['EntityDescriptor', '~SSODescriptor', 'KeyDescriptor'], + attributePath: ['KeyInfo', 'X509Data', 'X509Certificate'], attributes: [] }, { @@ -72,8 +74,14 @@ export default class Metadata implements MetadataInterface { const sharedCertificate = this.meta.sharedCertificate; if (typeof sharedCertificate === 'string') { this.meta.certificate = { - signing: sharedCertificate, - encryption: sharedCertificate + signing: this.meta.certificate.signing || sharedCertificate, + encryption: this.meta.certificate.encryption || sharedCertificate + }; + delete this.meta.sharedCertificate; + } else if (Array.isArray(sharedCertificate)) { + this.meta.certificate = { + signing: this.meta.certificate.signing || sharedCertificate[0], + encryption: this.meta.certificate.encryption || sharedCertificate[0] }; delete this.meta.sharedCertificate; } diff --git a/test/index.ts b/test/index.ts index e5ad77cc..8eff96d1 100644 --- a/test/index.ts +++ b/test/index.ts @@ -243,7 +243,13 @@ test('getAssertionConsumerService with two bindings', t => { t.is(libsaml.verifySignature(responseSignedByCert2, { metadata: idpRollingCert.entityMeta, signatureAlgorithm: signatureAlgorithms.RSA_SHA256 })[0], true); }); - + test('verify a XML signature with metadata but with multiple anonymous certificates, where some of them are incorrect', t => { + const responseSignedByCert1 = String(readFileSync('./test/misc/response_signed_cert1.xml')); + const metadata = idpMetadata( + readFileSync("./test/misc/idpmeta_multiple_anonymous_certs.xml") + ); + t.is(libsaml.verifySignature(responseSignedByCert1, { metadata: metadata, signatureAlgorithm: signatureAlgorithms.RSA_SHA256 })[0], true); + }); test('verify a XML signature signed by RSA-SHA1 with .cer keyFile', t => { const xml = String(readFileSync('./test/misc/signed_request_sha1.xml')); t.is(libsaml.verifySignature(xml, { keyFile: './test/key/sp/cert.cer' })[0], true); @@ -441,3 +447,23 @@ test('contains explicit certificate declaration for signing and encryption in me t.not(encryptionCertificate, null); t.not(signingCertificate, encryptionCertificate); }); + +test('returns different certificates for signing and encryption when metadata contains one signing and one anonymous certificate', t => { + const metadata = idpMetadata( + readFileSync("./test/misc/idpmeta_one_signing_and_one_anonymous_cert.xml") + ); + const signingCertificate = metadata.getX509Certificate("signing"); + const encryptionCertificate = metadata.getX509Certificate("encryption"); + t.not(signingCertificate, encryptionCertificate); +}); + +test('returns the same certificate for signing and encryption when multiple anonymous certificates are present', t => { + const metadata = idpMetadata( + readFileSync("./test/misc/idpmeta_multiple_anonymous_certs.xml") + ); + const signingCertificate = metadata.getX509Certificate("signing"); + const encryptionCertificate = metadata.getX509Certificate("encryption"); + t.not(signingCertificate, null); + t.not(encryptionCertificate, null); + t.is(signingCertificate, encryptionCertificate); +}); \ No newline at end of file diff --git a/test/libsaml.ts b/test/libsaml.ts new file mode 100644 index 00000000..1539a908 --- /dev/null +++ b/test/libsaml.ts @@ -0,0 +1,111 @@ +import test from "ava"; +import { SignedXml } from "xml-crypto"; +import libSaml from "../src/libsaml"; + +const fakeMetadataInvalid = { + getX509Certificate: (_usage: string) => null, +}; + +const fakeMetadataValid = { + getX509Certificate: (_usage: string) => "VALIDCERT", +}; + +test("throws ERR_UNDEFINED_SIGNATURE_VERIFIER_OPTIONS when neither keyFile nor metadata provided", (t) => { + const xml = ""; + const error = t.throws(() => { + libSaml.verifySignature(xml, { signatureAlgorithm: "dummy" } as any); + }); + t.is(error!.message, "ERR_UNDEFINED_SIGNATURE_VERIFIER_OPTIONS"); +}); + +test("throws ERR_ZERO_SIGNATURE when no signature element exists", (t) => { + const xml = ""; + const error = t.throws(() => { + libSaml.verifySignature(xml, { + keyFile: "dummy.pem", + signatureAlgorithm: "dummy", + } as any); + }); + t.is(error!.message, "ERR_ZERO_SIGNATURE"); +}); + +test("throws ERR_POTENTIAL_WRAPPING_ATTACK when wrapping element is present", (t) => { + // Construct XML with a wrapping assertion inside SubjectConfirmationData + const xml = ` + + + + VALIDCERT + + + + + + + + + + + + + `; + const error = t.throws(() => { + libSaml.verifySignature(xml, { + keyFile: "dummy.pem", + signatureAlgorithm: "dummy", + } as any); + }); + t.is(error!.message, "ERR_POTENTIAL_WRAPPING_ATTACK"); +}); + +test("throws INVALID_CERTIFICATE_PROVIDED when metadata returns no certificate", (t) => { + // Signature element is present and metadata returns null certificate. + const xml = ` + + + + ANOTHERCERT + + + Content + + `; + const error = t.throws(() => { + libSaml.verifySignature(xml, { + metadata: fakeMetadataInvalid, + signatureAlgorithm: "dummy", + } as any); + }); + t.is(error!.message, "INVALID_CERTIFICATE_PROVIDED"); +}); + +test("returns valid verification result when signature is valid", (t) => { + // Override SignedXml methods to simulate valid signature checking + const origCheckSignature = SignedXml.prototype.checkSignature; + const origLoadSignature = SignedXml.prototype.loadSignature; + SignedXml.prototype.checkSignature = () => true; + SignedXml.prototype.loadSignature = () => {}; + + // Create a minimal XML with a Signature element containing a valid certificate node + const xml = ` + + + + VALIDCERT + + + AssertionContent + + `; + const result = libSaml.verifySignature(xml, { + metadata: fakeMetadataValid, + signatureAlgorithm: "dummy", + } as any); + t.true(Array.isArray(result)); + t.true(result[0] === true); + t.regex(typeof result[1] === "string" ? result[1] : "", /Assertion/); + + // Restore the original methods + SignedXml.prototype.checkSignature = origCheckSignature; + SignedXml.prototype.loadSignature = origLoadSignature; +}); diff --git a/test/misc/idpmeta_multiple_anonymous_certs.xml b/test/misc/idpmeta_multiple_anonymous_certs.xml new file mode 100644 index 00000000..84f9adb0 --- /dev/null +++ b/test/misc/idpmeta_multiple_anonymous_certs.xml @@ -0,0 +1,41 @@ + + + + + + MIIDlzCCAn+gAwIBAgIJAO1ymQc33+bWMA0GCSqGSIb3DQEBCwUAMGIxCzAJBgNVBAYTAkhLMRMwEQYDVQQIDApTb21lLVN0YXRlMRowGAYDVQQKDBFJZGVudGl0eSBQcm92aWRlcjEUMBIGA1UECwwLRGV2ZWxvcG1lbnQxDDAKBgNVBAMMA0lEUDAeFw0xNTA3MDUxODAyMjdaFw0xODA3MDQxODAyMjdaMGIxCzAJBgNVBAYTAkhLMRMwEQYDVQQIDApTb21lLVN0YXRlMRowGAYDVQQKDBFJZGVudGl0eSBQcm92aWRlcjEUMBIGA1UECwwLRGV2ZWxvcG1lbnQxDDAKBgNVBAMMA0lEUDCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAODZsWhCe+yG0PalQPTUoD7yko5MTWMCRxJ8hSm2k7mG3Eg/Y2v0EBdCmTw7iDCevRqUmbmFnq7MROyV4eriJzh0KabAdZf7/k6koghst3ZUtWOwzshyxkBtWDwGmBpQGTGsKxJ8M1js3aSqNRXBT4OBWM9w2Glt1+8ty30RhYv3pSF+/HHLH7Ac+vLSIAlokaFW34RWTcJ/8rADuRWlXih4GfnIu0W/ncm5nTSaJiRAvr3dGDRO/khiXoJdbbOj7dHPULxVGbH9IbPK76TCwLbF7ikIMsPovVbTrpyL6vsbVUKeEl/5GKppTwp9DLAOeoSYpCYkkDkYKu9TRQjF02MCAwEAAaNQME4wHQYDVR0OBBYEFP2ut2AQdy6D1dwdwK740IHmbh38MB8GA1UdIwQYMBaAFP2ut2AQdy6D1dwdwK740IHmbh38MAwGA1UdEwQFMAMBAf8wDQYJKoZIhvcNAQELBQADggEBANMZUoPNmHzgja2PYkbvBYMHmpvUkVoiuvQ9cJPlqGTB2CRfG68BNNs/Clz8P7cIrAdkhCUwi1rSBhDuslGFNrSaIpv6B10FpBuKwef3G7YrPWFNEN6khY7aHNWSTHqKgs1DrGef2B9hvkrnHWbQVSVXrBFKe1wTCqcgGcOpYoSK7L8C6iX6uIA/uZYnVQ4NgBrizJ0azkjdegz3hwO/gt4malEURy8D85/AAVt6PAzhpb9VJUGxSXr/EfntVUEz3L2gUFWWk1CnZFyz0rIOEt/zPmeAY8BLyd/Tjxm4Y+gwNazKq5y9AJS+m858b/nM4QdCnUE4yyoWAJDUHiAmvFA= + + + + + + + MIIFLjCCAxYCCQCqGHhTssya9jANBgkqhkiG9w0BAQsFADBZMQswCQYDVQQGEwJISzESMBAGA1UECAwJSG9uZyBLb25nMRIwEAYDVQQHDAlIb25nIEtvbmcxEDAOBgNVBAoMB3NhbWxpZnkxEDAOBgNVBAMMB3NhbWxpZnkwHhcNMjAwNTEwMTUyNjIzWhcNMzAwNTA4MTUyNjIzWjBZMQswCQYDVQQGEwJISzESMBAGA1UECAwJSG9uZyBLb25nMRIwEAYDVQQHDAlIb25nIEtvbmcxEDAOBgNVBAoMB3NhbWxpZnkxEDAOBgNVBAMMB3NhbWxpZnkwggIiMA0GCSqGSIb3DQEBAQUAA4ICDwAwggIKAoICAQDQG+abAeeWjwsOZt5SkcNcw/XSJcjSyJykEbEU2iguErRuOIyBfgj0p1UVBv33uL2igeYJT3OSXmSjvMO8KvqtYN2tJAjoFjghGr8NbIEZjYS4ukMZUbwxd2bRycD9OMI9g44AUB1sfQ0UyFwzEOseW3lcW1FnhcizA8TgI0GN4NpdVruNlpgoWdP3w+Syhtq0rWebY8g/HGFruEKn8VwbUblOZdP7jNVXsd1aUMScpuMa0khzzXPDN+Q0rwl79fO4ychSeKAAERdPXA1UfDfbh9W7pcYBP0ABXd91Bf9akplmbbVOIsNbuRIcVS7WvLwCr613JuJ+EtGDcUkrSpbuRvDW85DQRHBGuoKlcSG+imHQtHqRwMwMc8P54hIEBvaFW0RfwPfzdFNe8wARtmvIeX84iwq5Yey15Ly1rdopi7t2g7qyF7C/B9gZ3tJ/gPKp2NrdCGFBcahl93Lj56WWmI0jNHn7+7Y3x6isJ3KTRXIliSrAwiK7/7UezOlWzs1k8mGQWZTD3AGGKu1cBVwuC+rh4wkLsDeHfzxavbXxVEok9p/1P28M4GiHfS0POE3Hl4RT3Q6AiYWnmFYyZ+smY97SgPwB4tTNYFjC6+9d/BllNoQb8wsPjqp6ZDn1OeY668hp+ZAcE13AFdiTBMVrcdEECCPLxg1kFk5wZdHrGwIDAQABMA0GCSqGSIb3DQEBCwUAA4ICAQCyA/14hKTqfdeOVl+MQ2SLPWi7pC/t/Zv4kc361xP26FAVSSrxgXq9kVLZeJIAqCwjGHkl/DTUlA8hfLfuZx5z+NI/qIodsXAjCzsCe7paEbjvR6OQjYkR0UY4u/AOO7x2op2KDFKNuWT9KZNm8bh1mxwNKep1fJP2O5M0nMYAGYbPsLAOn7mzZyufQl8hsJwIV2s8sbft7s8vmEYZbuueQDOJCMTt+eC08LONrovYChyYmj3i5RIk8kcaodeSDo811F1B1gDvO/dmVxgrHEgoai7X6LUoiAiLkigP7udNEZxbXsRlOhBRv9w+rRXFurVFlUPkQ9UF+QB0BoyIcUxo+fZ8vCA4xEVBenVBadpFbwum6+XeTkvDoRc4sSCpm8v2qtprc8aU/0F82EzxSybYvstc5lDv7wuwCwNwfoAQ+/16kTpJvoYbOXUPv5yCA3mIuqYeA1woaWPXsE4jNOzTqv1qOZQTvXProEgK5B0FR5ILc4mfNrD2p9VGbiYf2GjCfeEzDFg174dvSn2MMp1yK5pvZEp7yFE8z1eduYN6W/7qdtss9BGpnyS5X7LuYfDvd1dHP6/JuqJDbfSVG9prYWcaMRd3FzSC7jBeetJgMyj4dunfqw8R16aONhwvICtzdFa93hYrDvTyo3ae80KFi0WGgApKeoqO5t3l1PAcaA== + + + + + + + MIID6TCCAtGgAwIBAgIJAPQQPsolUypeMA0GCSqGSIb3DQEBCwUAMFYxCzAJBgNVBAYTAkhLMRIwEAYDVQQIEwlIb25nIEtvbmcxFTATBgNVBAoTDGV4cHJlc3Mtc2FtbDEMMAoGA1UECxMDZGV2MQ4wDAYDVQQDEwVlc2FtbDAeFw0xNTEwMDMwMzU3MzRaFw0xODEwMDIwMzU3MzRaMFYxCzAJBgNVBAYTAkhLMRIwEAYDVQQIEwlIb25nIEtvbmcxFTATBgNVBAoTDGV4cHJlc3Mtc2FtbDEMMAoGA1UECxMDZGV2MQ4wDAYDVQQDEwVlc2FtbDCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAL7dF1gUNu8en0fHMSbzf192uB8m2CTeHeEeYrmq5rau6t1WzaHwbSStd9tJ/11Arm8f8zfefFqEBA0EYbp/DMqHb9ZiLGgIff08679NOYeK/d9EAs5DzvTMTR6QqG7a4vH3jKOksIbjM35h5RVitVDxo+xWDKyvOpuNE64bJlWHOEiNxvwmcHfJ2hAd1EozaRLcJOojFHg51alUqiNIZ+vpkMAM8s3lUlcYETKqTpcnsE7c1QX60cCrFN4m3SNS98HGBEdotch8+2Myzz957cBiwg9CR05PtEfjH0gGXJbL56JmpPyY+TkEiNMtMqJ7RNkK92gZfoY2i3RdjLKOHDUCAwEAAaOBuTCBtjAdBgNVHQ4EFgQUm4zK2qBtDMICekupt3LnRBdbP9UwgYYGA1UdIwR/MH2AFJuMytqgbQzCAnpLqbdy50QXWz/VoVqkWDBWMQswCQYDVQQGEwJISzESMBAGA1UECBMJSG9uZyBLb25nMRUwEwYDVQQKEwxleHByZXNzLXNhbWwxDDAKBgNVBAsTA2RldjEOMAwGA1UEAxMFZXNhbWyCCQD0ED7KJVMqXjAMBgNVHRMEBTADAQH/MA0GCSqGSIb3DQEBCwUAA4IBAQA9t7VMtX93yIYIGFC20GCsMYZeZpTedxpxpjqom2dOuOUaDQgrZcGF3FVbFqTEpPtOnsKXYaCg7FJvUjxv7FIuix5H7JO6DALoJ792pfG2wwS2PvDiGFxMfGnNvb3aLnB/s6wTyWBpDYRdwlB5nj37KPk6kpFJj3N9x5BD1oTdmQqeVuacjoiemIulkc33P28tGl6Datth4WpE0LwmrwREQ1NWixi2j1Ti3mjYkyqGVY8XphWKEIIWmheqLnYCXRXhbxZ4E+FGg81ZYG8TKYC/IjzV8p0rLnAI1qS7wdwv5UJ9vQJt6KcxdHHZsUlpIfaJC6N5DvAL/qUY8DoIymgz + + + + urn:oasis:names:tc:SAML:2.0:nameid-format:persistent + urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress + urn:oasis:names:tc:SAML:2.0:nameid-format:transient + urn:oasis:names:tc:SAML:2.0:nameid-format:entity + urn:oasis:names:tc:SAML:1.1:nameid-format:unspecified + urn:oasis:names:tc:SAML:2.0:nameid-format:kerberos + urn:oasis:names:tc:SAML:1.1:nameid-format:WindowsDomainQualifiedName + urn:oasis:names:tc:SAML:1.1:nameid-format:X509SubjectName + + + + + + + \ No newline at end of file diff --git a/test/misc/idpmeta_one_signing_and_one_anonymous_cert.xml b/test/misc/idpmeta_one_signing_and_one_anonymous_cert.xml new file mode 100644 index 00000000..3964a1f5 --- /dev/null +++ b/test/misc/idpmeta_one_signing_and_one_anonymous_cert.xml @@ -0,0 +1,44 @@ + + + + + + + cert 1 + + + + + + + + + cert 2 + + + + + urn:oasis:names:tc:SAML:2.0:nameid-format:persistent + urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress + urn:oasis:names:tc:SAML:2.0:nameid-format:transient + urn:oasis:names:tc:SAML:2.0:nameid-format:entity + urn:oasis:names:tc:SAML:1.1:nameid-format:unspecified + urn:oasis:names:tc:SAML:2.0:nameid-format:kerberos + urn:oasis:names:tc:SAML:1.1:nameid-format:WindowsDomainQualifiedName + urn:oasis:names:tc:SAML:1.1:nameid-format:X509SubjectName + + + + + + + \ No newline at end of file