diff --git a/.changeset/gentle-cats-change.md b/.changeset/gentle-cats-change.md new file mode 100644 index 0000000000000..da4f32ad30a64 --- /dev/null +++ b/.changeset/gentle-cats-change.md @@ -0,0 +1,8 @@ +--- +'@rocket.chat/meteor': patch +'@rocket.chat/core-typings': patch +'@rocket.chat/model-typings': patch +'@rocket.chat/models': patch +--- + +Security Hotfix (https://docs.rocket.chat/docs/security-fixes-and-updates) diff --git a/apps/meteor/app/meteor-accounts-saml/server/definition/ISAMLAssertion.ts b/apps/meteor/app/meteor-accounts-saml/server/definition/ISAMLAssertion.ts index 1744afe7b3800..7480d34cb96ea 100644 --- a/apps/meteor/app/meteor-accounts-saml/server/definition/ISAMLAssertion.ts +++ b/apps/meteor/app/meteor-accounts-saml/server/definition/ISAMLAssertion.ts @@ -1,4 +1,4 @@ export interface ISAMLAssertion { - assertion: Element | Document; + assertion: Element; xml: string; } diff --git a/apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts b/apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts index 61e4bd7421b88..015c379c408c6 100644 --- a/apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts +++ b/apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts @@ -1,7 +1,7 @@ import type { ServerResponse } from 'node:http'; import type { IUser, IIncomingMessage, IPersonalAccessToken, IRole } from '@rocket.chat/core-typings'; -import { CredentialTokens, Rooms, Users, Roles } from '@rocket.chat/models'; +import { CredentialTokens, Rooms, Users, Roles, SamlUsedAssertions } from '@rocket.chat/models'; import { Random } from '@rocket.chat/random'; import { escapeRegExp, escapeHTML } from '@rocket.chat/string-helpers'; import { Accounts } from 'meteor/accounts-base'; @@ -505,6 +505,20 @@ export class SAML { throw new Error('No user data collected from IdP response.'); } + const baseExpireAt = profile.expireAt instanceof Date ? profile.expireAt : new Date(Date.now() + 300000); + + const safeExpireAt = new Date(baseExpireAt.getTime() + (service.allowedClockDrift || 0)); + + if (!profile.assertionId || !profile.issuer) { + SAMLUtils.error({ msg: 'Invalid SAML response: missing Assertion ID or Issuer', profile }); + throw new Error('Invalid SAML response: missing Assertion ID or Issuer.'); + } + + if (!(await SamlUsedAssertions.markUsed(profile.assertionId, profile.issuer, safeExpireAt))) { + SAMLUtils.warn({ msg: 'SAML assertion replay detected', issuer: profile.issuer, assertionId: profile.assertionId }); + throw new Error('An assertion with the same ID cannot be used more than once.'); + } + // create a random token to store the login result // to test an IdP initiated login on localhost, use the following URL (assuming SimpleSAMLPHP on localhost:8080): // http://localhost:8080/simplesaml/saml2/idp/SSOService.php?spentityid=http://localhost:3000/_saml/metadata/test-sp diff --git a/apps/meteor/app/meteor-accounts-saml/server/lib/Utils.ts b/apps/meteor/app/meteor-accounts-saml/server/lib/Utils.ts index c7bd6d032fb65..6a540e56e266d 100644 --- a/apps/meteor/app/meteor-accounts-saml/server/lib/Utils.ts +++ b/apps/meteor/app/meteor-accounts-saml/server/lib/Utils.ts @@ -149,6 +149,12 @@ export class SAMLUtils { } } + public static warn(obj: object | string): void { + if (logger) { + logger.warn(obj); + } + } + public static async inflateXml(deflatedXml: Buffer): Promise> { return new Promise((resolve, reject) => { zlib.inflateRaw(deflatedXml, (err, inflatedXml) => { diff --git a/apps/meteor/app/meteor-accounts-saml/server/lib/parsers/Response.ts b/apps/meteor/app/meteor-accounts-saml/server/lib/parsers/Response.ts index 8fa5e28eb2841..0887cc8652bb2 100644 --- a/apps/meteor/app/meteor-accounts-saml/server/lib/parsers/Response.ts +++ b/apps/meteor/app/meteor-accounts-saml/server/lib/parsers/Response.ts @@ -89,7 +89,7 @@ export class ResponseParser { } SAMLUtils.log('Status ok'); - let assertion: XmlParent; + let assertion: Element; let assertionData: ISAMLAssertion; let issuer; @@ -118,6 +118,18 @@ export class ResponseParser { profile.issuer = issuer.textContent; } + if (assertion.hasAttribute('ID')) { + profile.assertionId = assertion.getAttribute('ID'); + } + + const conditions = assertion.getElementsByTagNameNS('urn:oasis:names:tc:SAML:2.0:assertion', 'Conditions')[0]; + if (conditions?.hasAttribute('NotOnOrAfter')) { + const notOnOrAfter = conditions.getAttribute('NotOnOrAfter'); + if (notOnOrAfter) { + profile.expireAt = new Date(notOnOrAfter); + } + } + const subject = this.getSubject(assertion); if (subject) { const nameID = subject.getElementsByTagNameNS('urn:oasis:names:tc:SAML:2.0:assertion', 'NameID')[0]; @@ -205,7 +217,7 @@ export class ResponseParser { throw new Error('Too many SAML assertions'); } - let assertion: XmlParent = allAssertions[0]; + let assertion: Element = allAssertions[0]; const encAssertion = allEncrypedAssertions[0]; let newXml = null; @@ -295,11 +307,11 @@ export class ResponseParser { return this.validateSignatureChildren(xml, cert, response); } - private validateAssertionSignature(xml: string, cert: string, assertion: XmlParent): boolean { + private validateAssertionSignature(xml: string, cert: string, assertion: Element): boolean { return this.validateSignatureChildren(xml, cert, assertion); } - private validateSignatureChildren(xml: string, cert: string, parent: XmlParent): boolean { + private validateSignatureChildren(xml: string, cert: string, parent: Element): boolean { const xpathSigQuery = ".//*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']"; const signatures = xmlCrypto.xpath(parent, xpathSigQuery) as Array; let signature = null; @@ -344,7 +356,7 @@ export class ResponseParser { return result; } - private getIssuer(assertion: XmlParent): any { + private getIssuer(assertion: Element): any { const issuers = assertion.getElementsByTagNameNS('urn:oasis:names:tc:SAML:2.0:assertion', 'Issuer'); if (issuers.length > 1) { throw new Error('Too many Issuers'); @@ -353,7 +365,7 @@ export class ResponseParser { return issuers[0]; } - private getSubject(assertion: XmlParent): XmlParent { + private getSubject(assertion: Element): XmlParent { let subject: XmlParent = assertion.getElementsByTagNameNS('urn:oasis:names:tc:SAML:2.0:assertion', 'Subject')[0]; const encSubject = assertion.getElementsByTagNameNS('urn:oasis:names:tc:SAML:2.0:assertion', 'EncryptedID')[0]; @@ -418,7 +430,7 @@ export class ResponseParser { return true; } - private validateAssertionConditions(assertion: XmlParent): void { + private validateAssertionConditions(assertion: Element): void { const conditions = assertion.getElementsByTagNameNS('urn:oasis:names:tc:SAML:2.0:assertion', 'Conditions')[0]; if (conditions && !this.validateNotBeforeNotOnOrAfterAssertions(conditions)) { throw new Error('NotBefore / NotOnOrAfter assertion failed'); diff --git a/apps/meteor/server/models.ts b/apps/meteor/server/models.ts index 49b5b98b19f08..98eb364cecb38 100644 --- a/apps/meteor/server/models.ts +++ b/apps/meteor/server/models.ts @@ -75,6 +75,7 @@ import { WebdavAccountsRaw, WorkspaceCredentialsRaw, AbacAttributesRaw, + SamlUsedAssertionsRaw, } from '@rocket.chat/models'; import type { Collection } from 'mongodb'; @@ -161,3 +162,4 @@ registerModel('IVideoConferenceModel', new VideoConferenceRaw(db)); registerModel('IWebdavAccountsModel', new WebdavAccountsRaw(db)); registerModel('IWorkspaceCredentialsModel', new WorkspaceCredentialsRaw(db)); registerModel('IAbacAttributesModel', new AbacAttributesRaw(db)); +registerModel('ISamlUsedAssertionsModel', new SamlUsedAssertionsRaw(db)); diff --git a/apps/meteor/tests/unit/app/meteor-accounts-saml/data.ts b/apps/meteor/tests/unit/app/meteor-accounts-saml/data.ts index 7a0c41c7667cc..6df868351c812 100644 --- a/apps/meteor/tests/unit/app/meteor-accounts-saml/data.ts +++ b/apps/meteor/tests/unit/app/meteor-accounts-saml/data.ts @@ -128,6 +128,32 @@ export const simpleSamlResponse = `${samlResponseHeader} ${samlResponseAssertion} ${samlResponseFooter}`; +export const samlResponseAssertionId = '_assertionwithconditions00000000000000001'; + +const samlResponseAssertionWithConditions = ` + [ISSUER] + + [NAMEID] + + + + + urn:oasis:names:tc:SAML:2.0:ac:classes:Password + + + + + 1 + + + `; + +export const samlResponseWithConditions = `${samlResponseHeader} + ${samlResponseIssuer} + ${samlResponseStatus} + ${samlResponseAssertionWithConditions} +${samlResponseFooter}`; + export const samlResponseMissingStatus = `${samlResponseHeader} ${samlResponseIssuer} ${samlResponseAssertion} diff --git a/apps/meteor/tests/unit/app/meteor-accounts-saml/server.tests.ts b/apps/meteor/tests/unit/app/meteor-accounts-saml/server.tests.ts index fb972157db7ce..fac3f1c039a88 100644 --- a/apps/meteor/tests/unit/app/meteor-accounts-saml/server.tests.ts +++ b/apps/meteor/tests/unit/app/meteor-accounts-saml/server.tests.ts @@ -1,6 +1,7 @@ import { isTruthy } from '@rocket.chat/tools'; import { expect } from 'chai'; import proxyquire from 'proxyquire'; +import sinon from 'sinon'; import { serviceProviderOptions, @@ -20,6 +21,8 @@ import { samlResponseMultipleAssertions, samlResponseMissingAssertion, samlResponseMultipleIssuers, + samlResponseWithConditions, + samlResponseAssertionId, samlResponseValidSignatures, samlResponseValidAssertionSignature, encryptedResponse, @@ -319,6 +322,29 @@ describe('SAML', () => { }); }); + it('should contain properties to avoid assertions replay in the profile object from the response', () => { + const notBefore = new Date(); + notBefore.setMinutes(notBefore.getMinutes() - 3); + + const notOnOrAfter = new Date(); + notOnOrAfter.setMinutes(notOnOrAfter.getMinutes() + 3); + + const response = samlResponseWithConditions + .replace('[NOTBEFORE]', notBefore.toISOString()) + .replace('[NOTONORAFTER]', notOnOrAfter.toISOString()); + + const parser = new ResponseParser(serviceProviderOptions); + parser.validate(makeLoginResponseEnvelope(response), (err, profile, loggedOut) => { + expect(err).to.be.null; + expect(profile).to.be.an('object'); + expect(profile).to.have.property('assertionId').equal(samlResponseAssertionId); + expect(profile).to.have.property('expireAt'); + // `expireAt` mirrors the assertion's NotOnOrAfter condition. + expect(new Date((profile as any).expireAt).toISOString()).to.equal(notOnOrAfter.toISOString()); + expect(loggedOut).to.be.false; + }); + }); + it('should respect NotOnOrAfter conditions', () => { const notBefore = new Date(); notBefore.setMinutes(notBefore.getMinutes() - 3); @@ -1018,4 +1044,105 @@ describe('SAML', () => { ); }); }); + + describe('[SAML.processRequest] validate action - assertion replay protection', () => { + const markUsed = sinon.stub(); + const credentialCreate = sinon.stub().resolves(); + + // `pending` captures the promise returned by the (async) validateResponse callback, + // because processValidateAction does not await it. Awaiting it in the test guarantees + // markUsed / storeCredential / redirect have already run before we assert. + let pending: Promise | undefined; + let lastRedirect: string | undefined; + + // A profile as produced by ResponseParser, carrying the replay-guard fields. + const replayProfile = { + assertionId: samlResponseAssertionId, + issuer: '[ISSUER]', + expireAt: new Date(Date.now() + 5 * 60 * 1000), + }; + + const res = { + writeHead: (_code: number, headers?: Record) => { + lastRedirect = headers?.Location; + }, + write: () => undefined, + end: () => undefined, + }; + + const loadSAML = () => + proxyquire.noCallThru().load('../../../../app/meteor-accounts-saml/server/lib/SAML', { + '@rocket.chat/models': { + SamlUsedAssertions: { markUsed }, + CredentialTokens: { create: credentialCreate }, + Users: {}, + Rooms: {}, + Roles: {}, + }, + '@rocket.chat/random': { Random: { id: () => '__credentialToken__' } }, + '@rocket.chat/string-helpers': { escapeRegExp: (s: string) => s, escapeHTML: (s: string) => s }, + 'meteor/accounts-base': { Accounts: { _generateStampedLoginToken: () => ({ token: 't' }) } }, + 'meteor/meteor': { Meteor: { absoluteUrl: (path = '') => `http://localhost:3000/${path}`, Error } }, + './ServiceProvider': { + SAMLServiceProvider: class { + validateResponse(_envelope: unknown, cb: (err: Error | null, profile: any) => Promise) { + pending = cb(null, replayProfile); + } + }, + }, + './Utils': { + SAMLUtils: { + relayState: null, + error: sinon.stub(), + warn: sinon.stub(), + log: sinon.stub(), + getValidationActionRedirectPath: (token: string) => `_saml/validate/${token}`, + }, + }, + './getSAMLEnvelope': { getSAMLEnvelope: async () => ({ relayState: null }) }, + '../../../../lib/utils/arrayUtils': { ensureArray: (v: any) => v }, + '../../../../server/lib/logger/system': { SystemLogger: { error: sinon.stub(), warn: sinon.stub() } }, + '../../../lib/server/functions/addUserToRoom': { addUserToRoom: sinon.stub() }, + '../../../lib/server/functions/createRoom': { createRoom: sinon.stub() }, + '../../../lib/server/functions/getUsernameSuggestion': { generateUsernameSuggestion: sinon.stub() }, + '../../../lib/server/functions/saveUserIdentity': { saveUserIdentity: sinon.stub() }, + '../../../settings/server': { settings: { get: sinon.stub() } }, + '../../../utils/lib/i18n': { i18n: { t: (s: string) => s, languages: [] } }, + }).SAML; + + const service = { ...serviceProviderOptions } as any; + const samlObject = { actionName: 'validate', serviceName: 'test-sp', credentialToken: '' } as any; + + beforeEach(() => { + markUsed.reset(); + credentialCreate.reset(); + credentialCreate.resolves(); + pending = undefined; + lastRedirect = undefined; + }); + + it('should mark the assertion as used and store the credential for a fresh assertion', async () => { + markUsed.resolves(true); + const SAML = loadSAML(); + + await SAML.processRequest({} as any, res as any, service, samlObject); + await pending; + + expect(markUsed.calledOnceWithExactly(replayProfile.assertionId, replayProfile.issuer, replayProfile.expireAt)).to.be.true; + expect(credentialCreate.calledOnce).to.be.true; + expect(lastRedirect).to.equal('http://localhost:3000/_saml/validate/__credentialToken__'); + }); + + it('should reject a replayed assertion: no credential stored and redirect to the base url', async () => { + markUsed.resolves(false); + const SAML = loadSAML(); + + await SAML.processRequest({} as any, res as any, service, samlObject); + await pending; + + expect(markUsed.calledOnce).to.be.true; + expect(credentialCreate.called).to.be.false; + expect(lastRedirect).to.equal('http://localhost:3000/'); + }); + }); }); diff --git a/packages/core-typings/src/ISamlUsedAssertions.ts b/packages/core-typings/src/ISamlUsedAssertions.ts new file mode 100644 index 0000000000000..6680985aa0442 --- /dev/null +++ b/packages/core-typings/src/ISamlUsedAssertions.ts @@ -0,0 +1,7 @@ +export interface ISamlUsedAssertions { + _id: string; + assertionId: string; + issuer: string; + expireAt: Date; + createdAt: Date; +} diff --git a/packages/core-typings/src/index.ts b/packages/core-typings/src/index.ts index 56ba73c7d77f9..14170eb7d971f 100644 --- a/packages/core-typings/src/index.ts +++ b/packages/core-typings/src/index.ts @@ -54,6 +54,7 @@ export type * from './IEmojiCustom'; export type * from './ICustomEmojiDescriptor'; export type * from './IAnalytics'; export type * from './ICredentialToken'; +export type * from './ISamlUsedAssertions'; export type * from './IAvatar'; export type * from './ICustomUserStatus'; export type * from './IEmailMessageHistory'; diff --git a/packages/model-typings/src/index.ts b/packages/model-typings/src/index.ts index 439447c49ea02..d6638a92c272c 100644 --- a/packages/model-typings/src/index.ts +++ b/packages/model-typings/src/index.ts @@ -81,3 +81,4 @@ export type * from './updater'; export type * from './models/IWorkspaceCredentialsModel'; export type * from './models/ICallHistoryModel'; export type * from './models/IAbacAttributesModel'; +export type * from './models/ISamlUsedAssertionsModel'; diff --git a/packages/model-typings/src/models/ISamlUsedAssertionsModel.ts b/packages/model-typings/src/models/ISamlUsedAssertionsModel.ts new file mode 100644 index 0000000000000..11cf934fa6514 --- /dev/null +++ b/packages/model-typings/src/models/ISamlUsedAssertionsModel.ts @@ -0,0 +1,7 @@ +import type { ISamlUsedAssertions } from '@rocket.chat/core-typings'; + +import type { IBaseModel } from './IBaseModel'; + +export interface ISamlUsedAssertionsModel extends IBaseModel { + markUsed(assertionId: string, issuer: string, expireAt: Date): Promise; +} diff --git a/packages/models/src/index.ts b/packages/models/src/index.ts index 099420cd2f7ae..a54acad092a2a 100644 --- a/packages/models/src/index.ts +++ b/packages/models/src/index.ts @@ -80,6 +80,7 @@ import type { IMediaCallNegotiationsModel, ICallHistoryModel, IAbacAttributesModel, + ISamlUsedAssertionsModel, } from '@rocket.chat/model-typings'; import type { Collection, Db } from 'mongodb'; @@ -106,6 +107,7 @@ import { UsersSessionsRaw, AbacAttributesRaw, ServerEventsRaw, + SamlUsedAssertionsRaw, } from './modelClasses'; import { proxify, registerModel } from './proxify'; @@ -207,6 +209,7 @@ export const Migrations = proxify('IMigrationsModel'); export const ModerationReports = proxify('IModerationReportsModel'); export const WorkspaceCredentials = proxify('IWorkspaceCredentialsModel'); export const AbacAttributes = proxify('IAbacAttributesModel'); +export const SamlUsedAssertions = proxify('ISamlUsedAssertionsModel'); export function registerServiceModels(db: Db, trash?: Collection>): void { registerModel('IUsersSessionsModel', () => new UsersSessionsRaw(db)); @@ -241,4 +244,5 @@ export function registerServiceModels(db: Db, trash?: Collection new LivechatVisitorsRaw(db)); registerModel('IAbacAttributesModel', () => new AbacAttributesRaw(db)); registerModel('IServerEventsModel', () => new ServerEventsRaw(db)); + registerModel('ISamlUsedAssertionsModel', () => new SamlUsedAssertionsRaw(db)); } diff --git a/packages/models/src/modelClasses.ts b/packages/models/src/modelClasses.ts index c554fc9698550..0807ea23a5950 100644 --- a/packages/models/src/modelClasses.ts +++ b/packages/models/src/modelClasses.ts @@ -48,6 +48,7 @@ export * from './models/PushToken'; export * from './models/Reports'; export * from './models/Roles'; export * from './models/Rooms'; +export * from './models/SamlUsedAssertions'; export * from './models/ServerEvents'; export * from './models/Sessions'; export * from './models/Settings'; diff --git a/packages/models/src/models/SamlUsedAssertions.ts b/packages/models/src/models/SamlUsedAssertions.ts new file mode 100644 index 0000000000000..8b1ccf7732a13 --- /dev/null +++ b/packages/models/src/models/SamlUsedAssertions.ts @@ -0,0 +1,37 @@ +import crypto from 'crypto'; + +import type { ISamlUsedAssertions, RocketChatRecordDeleted } from '@rocket.chat/core-typings'; +import type { ISamlUsedAssertionsModel } from '@rocket.chat/model-typings'; +import type { MongoServerError, Collection, Db, IndexDescription } from 'mongodb'; + +import { BaseRaw } from './BaseRaw'; + +const DUPLICATE_KEY_ERROR_CODE = 11000; + +export class SamlUsedAssertionsRaw extends BaseRaw implements ISamlUsedAssertionsModel { + constructor(db: Db, trash?: Collection>) { + super(db, 'saml_used_assertions', trash); + } + + protected override modelIndexes(): IndexDescription[] { + return [{ key: { expireAt: 1 }, sparse: true, expireAfterSeconds: 0 }]; + } + + async markUsed(assertionId: string, issuer: string, expireAt: Date): Promise { + try { + await this.insertOne({ + _id: crypto.createHash('sha256').update(JSON.stringify({ issuer, assertionId })).digest('hex'), + assertionId, + issuer, + expireAt, + createdAt: new Date(), + }); + return true; + } catch (error: unknown) { + if (typeof error === 'object' && error !== null && (error as MongoServerError).code === DUPLICATE_KEY_ERROR_CODE) { + return false; + } + throw error; + } + } +}