diff --git a/.changeset/sso-samlify-security-update.md b/.changeset/sso-samlify-security-update.md new file mode 100644 index 0000000000..66aeb6bd50 --- /dev/null +++ b/.changeset/sso-samlify-security-update.md @@ -0,0 +1,9 @@ +--- +"@better-auth/sso": patch +--- + +Fix a high-severity XML injection in signed SAML assertions (GHSA-34r5-q4jw-r36m) by updating `samlify` from 2.10.2 to 2.13.1. A crafted `AttributeValue` could escalate privileges. + +samlify 2.11 replaced `node-forge` with Node's native crypto, which parses private keys through OpenSSL 3 and rejects PEM blocks that carry leading whitespace. SAML private keys are now normalized before they reach samlify, so a key pasted with indentation (for example from an indented YAML or JSON config) keeps loading. + +IdP-initiated Single Logout now derives its response from the parsed logout request, which fixes response generation under samlify 2.13. When mapping SAML attributes to user fields, a multi-valued attribute is read by its first value. diff --git a/packages/sso/package.json b/packages/sso/package.json index 930999c44a..b3d071f1ba 100644 --- a/packages/sso/package.json +++ b/packages/sso/package.json @@ -67,7 +67,7 @@ "dependencies": { "fast-xml-parser": "^5.8.0", "jose": "^6.1.3", - "samlify": "~2.10.2", + "samlify": "^2.13.1", "tldts": "^6.1.0", "zod": "^4.3.6" }, diff --git a/packages/sso/src/routes/helpers.ts b/packages/sso/src/routes/helpers.ts index f4f624ba29..094e3693b4 100644 --- a/packages/sso/src/routes/helpers.ts +++ b/packages/sso/src/routes/helpers.ts @@ -1,7 +1,7 @@ import type { DBAdapter } from "@better-auth/core/db/adapter"; import { saml } from "../samlify"; import type { SAMLConfig, SSOOptions, SSOProvider } from "../types"; -import { safeJsonParse } from "../utils"; +import { normalizePem, safeJsonParse } from "../utils"; export async function findSAMLProvider( providerId: string, @@ -83,10 +83,10 @@ export function createSP( wantLogoutResponseSigned: opts?.sloOptions?.wantLogoutResponseSigned ?? false, metadata: spData?.metadata, - privateKey: spData?.privateKey || config.privateKey, + privateKey: normalizePem(spData?.privateKey || config.privateKey), privateKeyPass: spData?.privateKeyPass, isAssertionEncrypted: spData?.isAssertionEncrypted || false, - encPrivateKey: spData?.encPrivateKey, + encPrivateKey: normalizePem(spData?.encPrivateKey), encPrivateKeyPass: spData?.encPrivateKeyPass, nameIDFormat: config.identifierFormat ? [config.identifierFormat] @@ -100,10 +100,10 @@ export function createIdP(config: SAMLConfig) { if (idpData?.metadata) { return saml.IdentityProvider({ metadata: idpData.metadata, - privateKey: idpData.privateKey, + privateKey: normalizePem(idpData.privateKey), privateKeyPass: idpData.privateKeyPass, isAssertionEncrypted: idpData.isAssertionEncrypted, - encPrivateKey: idpData.encPrivateKey, + encPrivateKey: normalizePem(idpData.encPrivateKey), encPrivateKeyPass: idpData.encPrivateKeyPass, }); } @@ -119,7 +119,7 @@ export function createIdP(config: SAMLConfig) { signingCert: idpData?.cert || config.cert, wantAuthnRequestsSigned: config.authnRequestsSigned || false, isAssertionEncrypted: idpData?.isAssertionEncrypted || false, - encPrivateKey: idpData?.encPrivateKey, + encPrivateKey: normalizePem(idpData?.encPrivateKey), encPrivateKeyPass: idpData?.encPrivateKeyPass, }); } diff --git a/packages/sso/src/routes/saml-pipeline.ts b/packages/sso/src/routes/saml-pipeline.ts index 83d312043a..521b517cd3 100644 --- a/packages/sso/src/routes/saml-pipeline.ts +++ b/packages/sso/src/routes/saml-pipeline.ts @@ -376,6 +376,13 @@ export async function processSAMLResponse( const attributes = extract.attributes || {}; const mapping = parsedSamlConfig.mapping ?? {}; + // samlify >= 2.13 types attribute values as `string | string[]` to support + // multi-valued attributes. The identity fields below are single-valued. + const attr = (key: string): string | undefined => { + const value = attributes[key]; + return Array.isArray(value) ? value[0] : value; + }; + const userInfo = { ...Object.fromEntries( Object.entries(mapping.extraFields || {}).map(([key, value]) => [ @@ -383,22 +390,24 @@ export async function processSAMLResponse( attributes[value as string], ]), ), - id: attributes[mapping.id || "nameID"] || extract.nameID, + id: attr(mapping.id || "nameID") || extract.nameID, email: ( - attributes[mapping.email || "email"] || extract.nameID + attr(mapping.email || "email") || + extract.nameID || + "" ).toLowerCase(), name: [ - attributes[mapping.firstName || "givenName"], - attributes[mapping.lastName || "surname"], + attr(mapping.firstName || "givenName"), + attr(mapping.lastName || "surname"), ] .filter(Boolean) .join(" ") || - attributes[mapping.name || "displayName"] || + attr(mapping.name || "displayName") || extract.nameID, emailVerified: options?.trustEmailVerified && mapping.emailVerified - ? ((attributes[mapping.emailVerified] || false) as boolean) + ? ((attr(mapping.emailVerified) || false) as boolean) : false, }; if (!userInfo.id || !userInfo.email) { diff --git a/packages/sso/src/routes/sso.ts b/packages/sso/src/routes/sso.ts index 39bbb6c3d1..259c5e4e29 100644 --- a/packages/sso/src/routes/sso.ts +++ b/packages/sso/src/routes/sso.ts @@ -20,6 +20,7 @@ import { handleOAuthUserInfo } from "better-auth/oauth2"; import { decodeJwt } from "jose"; import type { BindingContext } from "samlify/types/src/entity"; import type { IdentityProvider } from "samlify/types/src/entity-idp"; +import type { RequestInfo } from "samlify/types/src/types"; import * as z from "zod"; import * as constants from "../constants"; import { assignOrganizationFromProvider } from "../linking"; @@ -45,7 +46,12 @@ import type { SSOOptions, SSOProvider, } from "../types"; -import { domainMatches, safeJsonParse, validateEmailDomain } from "../utils"; +import { + domainMatches, + normalizePem, + safeJsonParse, + validateEmailDomain, +} from "../utils"; import { getVerificationIdentifier } from "./domain-verification"; import { createIdP, @@ -1291,9 +1297,10 @@ export const signInSSO = (options?: SSOOptions) => { const sp = saml.ServiceProvider({ metadata: metadata, allowCreate: true, - privateKey: + privateKey: normalizePem( parsedSamlConfig.spMetadata?.privateKey || - parsedSamlConfig.privateKey, + parsedSamlConfig.privateKey, + ), privateKeyPass: parsedSamlConfig.spMetadata?.privateKeyPass, relayState, }); @@ -1313,16 +1320,16 @@ export const signInSSO = (options?: SSOOptions) => { wantAuthnRequestsSigned: parsedSamlConfig.authnRequestsSigned || false, isAssertionEncrypted: idpData?.isAssertionEncrypted || false, - encPrivateKey: idpData?.encPrivateKey, + encPrivateKey: normalizePem(idpData?.encPrivateKey), encPrivateKeyPass: idpData?.encPrivateKeyPass, }); } else { idp = saml.IdentityProvider({ metadata: idpData.metadata, - privateKey: idpData.privateKey, + privateKey: normalizePem(idpData.privateKey), privateKeyPass: idpData.privateKeyPass, isAssertionEncrypted: idpData.isAssertionEncrypted, - encPrivateKey: idpData.encPrivateKey, + encPrivateKey: normalizePem(idpData.encPrivateKey), encPrivateKeyPass: idpData.encPrivateKeyPass, }); } @@ -2235,17 +2242,17 @@ async function handleLogoutRequest( deleteSessionCookie(ctx); - const requestId = parsed.extract.request?.id || ""; + // Pass the parsed request so samlify links `InResponseTo` and fills the + // response template (ID, Issuer, IssueInstant, Destination, StatusCode). const res = sp.createLogoutResponse( idp, - null, + parsed as unknown as RequestInfo, binding, relayState || "", - (template: string) => - template - .replace("{InResponseTo}", requestId) - .replace("{StatusCode}", constants.SAML_STATUS_SUCCESS), - ) as { context: string; entityEndpoint?: string }; + ) as { + context: string; + entityEndpoint?: string; + }; if (binding === "post" && res.entityEndpoint) { return createSAMLPostForm( diff --git a/packages/sso/src/saml.test.ts b/packages/sso/src/saml.test.ts index 32bf836efd..10aa1fbda2 100644 --- a/packages/sso/src/saml.test.ts +++ b/packages/sso/src/saml.test.ts @@ -20,6 +20,7 @@ import type { Response as ExpressResponse, } from "express"; import express from "express"; +import type { RequestInfo } from "samlify/types/src/types"; import { afterAll, afterEach, @@ -34,6 +35,7 @@ import { sso, validateSAMLTimestamp } from "."; import { ssoClient } from "./client"; import { DEFAULT_CLOCK_SKEW_MS } from "./constants"; import { saml } from "./samlify"; +import { normalizePem } from "./utils"; const spMetadata = ` @@ -109,7 +111,18 @@ const idpMetadata = ` `; -const idPk = ` +/** + * Strips the indentation that keeps these PEM fixtures readable in the source. + * samlify >= 2.11 parses keys with Node's native crypto (OpenSSL 3), which + * rejects leading whitespace in PEM blocks; node-forge previously tolerated it. + */ +const dedentPem = (strings: TemplateStringsArray) => + `${(strings[0] ?? "") + .split("\n") + .map((line) => line.trim()) + .join("\n") + .trim()}\n`; +const idPk = dedentPem` -----BEGIN RSA PRIVATE KEY----- MIIJKgIBAAKCAgEA+YIi6C8hA+NSB7dEcQf5OseCtL8wCohnnD8nnUTUdaMor9zm JLhuY4ExNsHD2XO5A9RIOXerYdAVh8v6L2jUuZpIJzleoypDGlluLAjEpy52lstL @@ -163,7 +176,7 @@ const idPk = ` -----END RSA PRIVATE KEY----- `; -const spPrivateKey = ` +const spPrivateKey = dedentPem` -----BEGIN RSA PRIVATE KEY----- Proc-Type: 4,ENCRYPTED DEK-Info: DES-EDE3-CBC,9C86371F0420A091 @@ -195,7 +208,7 @@ const spPrivateKey = ` jnnjCjsK5YzTa4hmbHhPZIW262KoFV9TqxYKkhP5ab7AXRSakrdrY2cwACWN4AMT -----END RSA PRIVATE KEY----- `; -const idpPrivateKey = ` +const idpPrivateKey = dedentPem` -----BEGIN RSA PRIVATE KEY----- Proc-Type: 4,ENCRYPTED DEK-Info: DES-EDE3-CBC,116B0EBB2F2F0A9D @@ -250,7 +263,7 @@ const certificate = ` yyoWAJDUHiAmvFA= -----END CERTIFICATE----- `; -const idpEncryptionKey = ` +const idpEncryptionKey = dedentPem` -----BEGIN RSA PRIVATE KEY----- Proc-Type: 4,ENCRYPTED DEK-Info: DES-EDE3-CBC,860FDB9F3BE14699 @@ -282,7 +295,7 @@ const idpEncryptionKey = ` ISbutnQPUN5fsaIsgKDIV3T7n6519t6brobcW5bdigmf5ebFeZJ16/lYy6V77UM5 -----END RSA PRIVATE KEY----- `; -const spEncryptionKey = ` +const spEncryptionKey = dedentPem` -----BEGIN RSA PRIVATE KEY----- Proc-Type: 4,ENCRYPTED DEK-Info: DES-EDE3-CBC,860FDB9F3BE14699 @@ -493,13 +506,13 @@ const createMockSAMLIdP = (port: number, options: MockIdPOptions = {}) => { emailAddress: emailValue, famName: "hello world", }; - const { context, entityEndpoint } = await idp.createLoginResponse( + const { context, entityEndpoint } = (await idp.createLoginResponse( sp, {} as any, saml.Constants.wording.binding.post, user, createTemplateCallback(idp, sp, user.emailAddress), - ); + )) as { context: string; entityEndpoint?: string }; res.status(200).json({ samlResponse: context, entityEndpoint }); } catch (error) { res.status(500).json({ @@ -589,6 +602,21 @@ afterAll(async () => { await sharedMockIdP.stop(); }); +describe("private key normalization", () => { + // samlify >= 2.11 parses keys with native crypto (OpenSSL 3), which rejects + // indented PEM that node-forge tolerated. normalizePem keeps such keys working. + it("loads PEM keys pasted with leading indentation", () => { + const indented = idPk + .split("\n") + .map((line) => ` ${line}`) + .join("\n"); + expect(() => createPrivateKey({ key: indented })).toThrow(); + expect(() => + createPrivateKey({ key: normalizePem(indented) }), + ).not.toThrow(); + }); +}); + describe("SAML SSO with defaultSSO array", async () => { const data = { user: [], @@ -5443,7 +5471,7 @@ describe("SAML Single Logout (SLO)", () => { const logoutResponse = idp.createLogoutResponse( sp, - null, + null as unknown as RequestInfo, saml.Constants.wording.binding.redirect, callbackUrl, ) as { context: string }; @@ -5527,7 +5555,7 @@ describe("SAML Single Logout (SLO)", () => { const logoutResponse = idp.createLogoutResponse( sp, - null, + null as unknown as RequestInfo, saml.Constants.wording.binding.redirect, "", ) as { context: string }; diff --git a/packages/sso/src/utils.ts b/packages/sso/src/utils.ts index a85ca1e841..0c7f1c0ee7 100644 --- a/packages/sso/src/utils.ts +++ b/packages/sso/src/utils.ts @@ -74,6 +74,23 @@ export function parseCertificate(certPem: string) { }; } +/** + * samlify >= 2.11 parses private keys with Node's native crypto (OpenSSL 3), + * which rejects PEM blocks carrying leading indentation that the previous + * node-forge implementation tolerated. Trim each line so keys pasted with + * surrounding whitespace (e.g. indented YAML/JSON config) still load. + */ +export function normalizePem(key: string): string; +export function normalizePem(key: string | undefined): string | undefined; +export function normalizePem(key: string | undefined): string | undefined { + if (!key) return key; + return `${key + .split("\n") + .map((line) => line.trim()) + .join("\n") + .trim()}\n`; +} + export function getHostnameFromDomain(domain: string): string | null { return getHostname(domain) || null; } diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 10ca147acb..17423492f6 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -1466,8 +1466,8 @@ importers: specifier: ^6.1.3 version: 6.1.3 samlify: - specifier: ~2.10.2 - version: 2.10.2 + specifier: ^2.13.1 + version: 2.13.1 tldts: specifier: ^6.1.0 version: 6.1.86 @@ -11656,9 +11656,6 @@ packages: pako@0.2.9: resolution: {integrity: sha512-NUcwaKxUxWrZLpDG+z/xZaCgQITkA/Dv4V/T6bw7VON6l1Xz/VnrBqrYjZQ12TamKHzITTfOEIYUj48y2KXImA==} - pako@1.0.11: - resolution: {integrity: sha512-4hLB8Py4zZce5s4yd9XzopqwVv/yGNhV1Bl8NTmCq1763HeK2+EwVTv+leGeL13Dnh2wfbqowVPXCIO0z4taYw==} - parent-module@1.0.1: resolution: {integrity: sha512-GQ2EWRpQV8/o+Aw8YqtfZZPfNRWZYkbidE9k5rpl/hC3vtHHBfGm2Ifi6qWV+coDGkrUKZAxE3Lot5kcsRlh+g==} engines: {node: '>=6'} @@ -12655,8 +12652,8 @@ packages: safer-buffer@2.1.2: resolution: {integrity: sha512-YZo3K82SD7Riyi0E1EQPojLz7kpepnSQI9IyPbHHg1XXXevb5dJI7tpyN2ADxGcQbHG7vcyRHk0cbwqcQriUtg==} - samlify@2.10.2: - resolution: {integrity: sha512-y5s1cHwclqwP8h7K2Wj9SfP1q+1S9+jrs5OAegYTLAiuFi7nDvuKqbiXLmUTvYPMpzHcX94wTY2+D604jgTKvA==} + samlify@2.13.1: + resolution: {integrity: sha512-vdYr/zohDGBbfWNU4miEzc1jmWOtkLySPViapC6nfGkv9KxzLq4UlGkKyryzwLw4jVlZk88Rw93HaCRVpe+t+g==} satori@0.21.0: resolution: {integrity: sha512-PV5II6Z+gsFFLq4jp6mslt0ObNRzcTi5Xq+8JtJ3ReqBS9k4nF4ajqITFXygPZgRA6l5+22NYIS1WRTqTfDp5g==} @@ -14347,6 +14344,10 @@ packages: resolution: {integrity: sha512-NNXnzrkDrAzalLhIUc01jO2mOzXGXh1JwPgkihcLLzw98c0WgYDmmjSh1Kl3wzaxSVWMuA+fe0WTWOBDWCBmNA==} engines: {node: '>=0.6.0'} + xpath@0.0.34: + resolution: {integrity: sha512-FxF6+rkr1rNSQrhUNYrAFJpRXNzlDoMxeXN5qI84939ylEv3qqPFKa85Oxr6tDaJKqwW6KKyo2v26TSv3k6LeA==} + engines: {node: '>=0.6.0'} + xtend@4.0.2: resolution: {integrity: sha512-LKYU1iAXJXUgAXn9URjiu+MWhyUXHsvfp7mcuYm9dSUKK0/CjtrUwFAxD82/mCWbtLsGjFIad0wIsod4zrTAEQ==} engines: {node: '>=0.4'} @@ -26009,8 +26010,6 @@ snapshots: pako@0.2.9: {} - pako@1.0.11: {} - parent-module@1.0.1: dependencies: callsites: 3.1.0 @@ -27414,19 +27413,15 @@ snapshots: safer-buffer@2.1.2: {} - samlify@2.10.2: + samlify@2.13.1: dependencies: '@authenio/xml-encryption': 2.0.2 '@xmldom/xmldom': 0.8.13 - camelcase: 6.3.0 - node-forge: 1.4.0 node-rsa: 1.1.1 - pako: 1.0.11 - uuid: 11.1.1 xml: 1.0.1 xml-crypto: 6.1.2 xml-escape: 1.1.0 - xpath: 0.0.32 + xpath: 0.0.34 satori@0.21.0: dependencies: @@ -29289,6 +29284,8 @@ snapshots: xpath@0.0.33: {} + xpath@0.0.34: {} + xtend@4.0.2: {} y18n@4.0.3: {}