From ed953d398cda08903ac0bade18e82ea42c92cde9 Mon Sep 17 00:00:00 2001 From: Scott Duensing Date: Wed, 24 Jun 2026 20:22:35 -0500 Subject: [PATCH] Security audit. --- lib/clients.js | 8 +++++--- lib/constants.js | 2 +- lib/oidc/discovery.js | 10 +++++++++- lib/oidc/provider.js | 5 +++++ lib/saml/idp.js | 5 +++++ lib/saml/routes.js | 18 ++++++++++++++---- package.json | 2 +- 7 files changed, 40 insertions(+), 10 deletions(-) diff --git a/lib/clients.js b/lib/clients.js index 6969437..1c3617d 100644 --- a/lib/clients.js +++ b/lib/clients.js @@ -72,9 +72,11 @@ const toOidcMetadata = (row) => { response_types: JSON.parse(row.response_types), token_endpoint_auth_method: row.token_auth_method }; - if (row.scope) { - meta.scope = row.scope; - } + // Cap every client at the scopes it registered; default to the minimal + // "openid" so a client that registered no scope cannot silently receive + // email/profile/groups (oidc-provider intersects requested scopes against + // this). Broader access must be granted explicitly at registration. + meta.scope = row.scope || "openid"; if (row.token_auth_method !== AUTH_NONE && row.secret_ciphertext) { meta.client_secret = idpCrypto.openText({ ciphertext: row.secret_ciphertext, diff --git a/lib/constants.js b/lib/constants.js index 351e36a..ad92f94 100644 --- a/lib/constants.js +++ b/lib/constants.js @@ -5,7 +5,7 @@ // crypto.js; protocol/policy values live here. const PLUGIN_NAME = "saltcorn-idp"; -const PLUGIN_VERSION = "0.0.5"; +const PLUGIN_VERSION = "0.0.7"; // Public OIDC/OAuth2 + machine endpoints live under this path and are // CSRF-exempt. Admin (browser, CSRF-protected) pages live under ADMIN_BASE_PATH. diff --git a/lib/oidc/discovery.js b/lib/oidc/discovery.js index 85d7eaf..16600ff 100644 --- a/lib/oidc/discovery.js +++ b/lib/oidc/discovery.js @@ -25,7 +25,15 @@ const issuerForReq = (req) => { // getState unavailable; fall back to request-derived host } if (!base) { - base = req.protocol + "://" + req.get("host"); + const host = req.get("host") || ""; + // Defensive: only a clean single host[:port] may seed the issuer in the + // fallback path. A forged/garbage Host (multiple values, schemes, control + // chars) must never be reflected into the advertised issuer/endpoints. + // (A valid-but-wrong Host still requires the operational fix: set base_url.) + if (!/^[A-Za-z0-9.-]+(:[0-9]{1,5})?$/.test(host)) { + throw new Error(`${constants.PLUGIN_NAME}: refusing to derive issuer from malformed Host "${host}"; set base_url`); + } + base = req.protocol + "://" + host; // eslint-disable-next-line no-console console.warn(`[${constants.PLUGIN_NAME}] base_url not set; deriving issuer from request Host (${base}). Set base_url to prevent Host-header issuer poisoning.`); } diff --git a/lib/oidc/provider.js b/lib/oidc/provider.js index bb4541b..00abdcf 100644 --- a/lib/oidc/provider.js +++ b/lib/oidc/provider.js @@ -67,6 +67,11 @@ const buildProvider = async (issuer) => { groups: ["groups"] }, scopes: ["openid", "email", "profile", "groups"], + // Require PKCE for ALL clients (oidc-provider only mandates it for public + // clients by default). For a security-first IdP this removes code + // interception/replay as a single-factor risk even if a confidential + // client's secret leaks. + pkce: { required: () => true }, features: { devInteractions: { enabled: false } }, diff --git a/lib/saml/idp.js b/lib/saml/idp.js index d6b2975..8fe4784 100644 --- a/lib/saml/idp.js +++ b/lib/saml/idp.js @@ -166,6 +166,11 @@ const buildSp = (entityID, acsUrl, opts) => { opts = opts || {}; const settings = { entityID: entityID, + // Sign BOTH the Response envelope AND the Assertion element. Signing the + // assertion (not just the message) gives per-assertion integrity and is + // the posture least exposed to XML-signature-wrapping at the consuming SP. + wantMessageSigned: true, + wantAssertionsSigned: true, assertionConsumerService: [ { Binding: saml.Constants.namespace.binding.post, Location: acsUrl } ] diff --git a/lib/saml/routes.js b/lib/saml/routes.js index d70a264..71ab7a2 100644 --- a/lib/saml/routes.js +++ b/lib/saml/routes.js @@ -138,7 +138,10 @@ const expandGroupValues = (template, groupsArr) => { values += "" + escapeXmlAttr(g) + ""; } } - return template.replace(re, values); + // Function replacer: the values string may contain a group name with $&/$`/$' + // which String.replace would otherwise treat as special replacement patterns + // and splice surrounding markup back into the (about-to-be-signed) assertion. + return template.replace(re, () => values); }; @@ -181,9 +184,13 @@ const makeLoginResponseRenderer = (opts) => { tvalue.InResponseTo = (opts.requestInfo && opts.requestInfo.extract && opts.requestInfo.extract.request && opts.requestInfo.extract.request.id) || ""; } for (const k of Object.keys(tvalue)) { - work = work.replace(new RegExp("\\{" + k + "\\}", "g"), escapeXmlAttr(tvalue[k])); + // Function replacer so a value containing $&/$`/$'/$n is inserted + // literally (escapeXmlAttr does not neutralize $) and cannot inject + // markup into the signed assertion. + const escaped = escapeXmlAttr(tvalue[k]); + work = work.replace(new RegExp("\\{" + k + "\\}", "g"), () => escaped); } - work = work.replace(/\{AuthnStatement\}/g, buildAuthnStatement(nowIso, sessionIndex)); + work = work.replace(/\{AuthnStatement\}/g, () => buildAuthnStatement(nowIso, sessionIndex)); return { id: respId, context: work }; }; }; @@ -204,7 +211,10 @@ const makeLogoutResponseRenderer = (opts) => { }; let work = template; for (const k of Object.keys(tvalue)) { - work = work.replace(new RegExp("\\{" + k + "\\}", "g"), escapeXmlAttr(tvalue[k])); + // Function replacer: see makeLoginResponseRenderer -- $-patterns in a + // value must not be interpreted as special replacement sequences. + const escaped = escapeXmlAttr(tvalue[k]); + work = work.replace(new RegExp("\\{" + k + "\\}", "g"), () => escaped); } return { id: respId, context: work }; }; diff --git a/package.json b/package.json index 69616ea..7a44e1f 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "saltcorn-idp", - "version": "0.0.5", + "version": "0.0.7", "description": "Saltcorn plugin: turns Saltcorn into an SSO Identity Provider (OIDC/OAuth2, LDAP with groups, and SAML 2.0). Per-tenant asymmetric signing keys sealed at rest; multi-tenant. See VENDORING.md for the dependency-ownership/security posture.", "main": "index.js", "scripts": {