diff --git a/CHANGELOG.md b/CHANGELOG.md index cc16d3f96b4..e11ba1a5dd9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,8 @@ +Changes in [34.11.0](https://github.com/matrix-org/matrix-js-sdk/releases/tag/v34.11.0) (2024-11-12) +==================================================================================================== +# Security +- Fixes for [CVE-2024-50336](https://nvd.nist.gov/vuln/detail/CVE-2024-50336) / [GHSA-xvg8-m4x3-w6xr](https://github.com/matrix-org/matrix-js-sdk/security/advisories/GHSA-xvg8-m4x3-w6xr). + Changes in [34.10.0](https://github.com/matrix-org/matrix-js-sdk/releases/tag/v34.10.0) (2024-11-05) ==================================================================================================== ## 🦖 Deprecations diff --git a/spec/unit/content-repo.spec.ts b/spec/unit/content-repo.spec.ts index 23f504c1e43..8d24107fe26 100644 --- a/spec/unit/content-repo.spec.ts +++ b/spec/unit/content-repo.spec.ts @@ -63,20 +63,6 @@ describe("ContentRepo", function () { ); }); - it("should put fragments from mxc:// URIs after any query parameters", function () { - const mxcUri = "mxc://server.name/resourceid#automade"; - expect(getHttpUriForMxc(baseUrl, mxcUri, 32)).toEqual( - baseUrl + "/_matrix/media/v3/thumbnail/server.name/resourceid" + "?width=32#automade", - ); - }); - - it("should put fragments from mxc:// URIs at the end of the HTTP URI", function () { - const mxcUri = "mxc://server.name/resourceid#automade"; - expect(getHttpUriForMxc(baseUrl, mxcUri)).toEqual( - baseUrl + "/_matrix/media/v3/download/server.name/resourceid#automade", - ); - }); - it("should return an authenticated URL when requested", function () { const mxcUri = "mxc://server.name/resourceid"; expect(getHttpUriForMxc(baseUrl, mxcUri, undefined, undefined, undefined, undefined, true, true)).toEqual( @@ -98,5 +84,30 @@ describe("ContentRepo", function () { "/_matrix/client/v1/media/thumbnail/server.name/resourceid?width=64&height=64&method=scale&allow_redirect=true", ); }); + + it("should drop mxc urls with invalid server_name", () => { + const mxcUri = "mxc://server.name:test/foobar"; + expect(getHttpUriForMxc(baseUrl, mxcUri)).toEqual(""); + }); + + it("should drop mxc urls with invalid media_id", () => { + const mxcUri = "mxc://server.name/foobar:test"; + expect(getHttpUriForMxc(baseUrl, mxcUri)).toEqual(""); + }); + + it("should drop mxc urls attempting a path traversal attack", () => { + const mxcUri = "mxc://../../../../foo"; + expect(getHttpUriForMxc(baseUrl, mxcUri)).toEqual(""); + }); + + it("should drop mxc urls attempting to pass query parameters", () => { + const mxcUri = "mxc://server.name/foobar?bar=baz"; + expect(getHttpUriForMxc(baseUrl, mxcUri)).toEqual(""); + }); + + it("should drop mxc urls with too many parts", () => { + const mxcUri = "mxc://server.name/foo//bar"; + expect(getHttpUriForMxc(baseUrl, mxcUri)).toEqual(""); + }); }); }); diff --git a/spec/unit/matrixrtc/MatrixRTCSession.spec.ts b/spec/unit/matrixrtc/MatrixRTCSession.spec.ts index 96b1db9b031..fb62a669c7b 100644 --- a/spec/unit/matrixrtc/MatrixRTCSession.spec.ts +++ b/spec/unit/matrixrtc/MatrixRTCSession.spec.ts @@ -467,6 +467,36 @@ describe("MatrixRTCSession", () => { jest.useRealTimers(); }); + it("uses membershipExpiryTimeout from join config", async () => { + const realSetTimeout = setTimeout; + jest.useFakeTimers(); + sess!.joinRoomSession([mockFocus], mockFocus, { membershipExpiryTimeout: 60000 }); + await Promise.race([sentStateEvent, new Promise((resolve) => realSetTimeout(resolve, 500))]); + expect(client.sendStateEvent).toHaveBeenCalledWith( + mockRoom!.roomId, + EventType.GroupCallMemberPrefix, + { + memberships: [ + { + application: "m.call", + scope: "m.room", + call_id: "", + device_id: "AAAAAAA", + expires: 60000, + expires_ts: Date.now() + 60000, + foci_active: [mockFocus], + + membershipID: expect.stringMatching(".*"), + }, + ], + }, + "@alice:example.org", + ); + await Promise.race([sentDelayedState, new Promise((resolve) => realSetTimeout(resolve, 500))]); + expect(client._unstable_sendDelayedStateEvent).toHaveBeenCalledTimes(0); + jest.useRealTimers(); + }); + describe("non-legacy calls", () => { const activeFocusConfig = { type: "livekit", livekit_service_url: "https://active.url" }; const activeFocus = { type: "livekit", focus_selection: "oldest_membership" }; @@ -478,6 +508,19 @@ describe("MatrixRTCSession", () => { jest.useFakeTimers(); + // preparing the delayed disconnect should handle the delay being too long + const sendDelayedStateExceedAttempt = new Promise((resolve) => { + const error = new MatrixError({ + "errcode": "M_UNKNOWN", + "org.matrix.msc4140.errcode": "M_MAX_DELAY_EXCEEDED", + "org.matrix.msc4140.max_delay": 7500, + }); + sendDelayedStateMock.mockImplementationOnce(() => { + resolve(); + return Promise.reject(error); + }); + }); + // preparing the delayed disconnect should handle ratelimiting const sendDelayedStateAttempt = new Promise((resolve) => { const error = new MatrixError({ errcode: "M_LIMIT_EXCEEDED" }); @@ -511,7 +554,14 @@ describe("MatrixRTCSession", () => { }); }); - sess!.joinRoomSession([activeFocusConfig], activeFocus, { useLegacyMemberEvents: false }); + sess!.joinRoomSession([activeFocusConfig], activeFocus, { + useLegacyMemberEvents: false, + membershipServerSideExpiryTimeout: 9000, + }); + + expect(sess).toHaveProperty("membershipServerSideExpiryTimeout", 9000); + await sendDelayedStateExceedAttempt.then(); // needed to resolve after the send attempt catches + expect(sess).toHaveProperty("membershipServerSideExpiryTimeout", 7500); await sendDelayedStateAttempt; jest.advanceTimersByTime(5000); diff --git a/src/content-repo.ts b/src/content-repo.ts index b6174b6d8d9..b0b02a05691 100644 --- a/src/content-repo.ts +++ b/src/content-repo.ts @@ -1,5 +1,5 @@ /* -Copyright 2015 - 2021 The Matrix.org Foundation C.I.C. +Copyright 2015 - 2024 The Matrix.org Foundation C.I.C. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -14,7 +14,22 @@ See the License for the specific language governing permissions and limitations under the License. */ -import { encodeParams } from "./utils.ts"; +// Validation based on https://spec.matrix.org/v1.12/appendices/#server-name +// We do not use the validation described in https://spec.matrix.org/v1.12/client-server-api/#security-considerations-5 +// as it'd wrongly make all MXCs invalid due to not allowing `[].:` in server names. +const serverNameRegex = + /^(?:(?:\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3})|(?:\[[\dA-Fa-f:.]{2,45}])|(?:[A-Za-z\d\-.]{1,255}))(?::\d{1,5})?$/; +function validateServerName(serverName: string): boolean { + const matches = serverNameRegex.exec(serverName); + return matches?.[0] === serverName; +} + +// Validation based on https://spec.matrix.org/v1.12/client-server-api/#security-considerations-5 +const mediaIdRegex = /^[\w-]+$/; +function validateMediaId(mediaId: string): boolean { + const matches = mediaIdRegex.exec(mediaId); + return matches?.[0] === mediaId; +} /** * Get the HTTP URL for an MXC URI. @@ -36,7 +51,7 @@ import { encodeParams } from "./utils.ts"; * for authenticated media will *not* be checked - it is the caller's responsibility * to do so before calling this function. Note also that `useAuthentication` * implies `allowRedirects`. Defaults to false (unauthenticated endpoints). - * @returns The complete URL to the content. + * @returns The complete URL to the content, may be an empty string if the provided mxc is not valid. */ export function getHttpUriForMxc( baseUrl: string, @@ -51,7 +66,7 @@ export function getHttpUriForMxc( if (typeof mxc !== "string" || !mxc) { return ""; } - if (mxc.indexOf("mxc://") !== 0) { + if (!mxc.startsWith("mxc://")) { if (allowDirectLinks) { return mxc; } else { @@ -59,6 +74,11 @@ export function getHttpUriForMxc( } } + const [serverName, mediaId, ...rest] = mxc.slice(6).split("/"); + if (rest.length > 0 || !validateServerName(serverName) || !validateMediaId(mediaId)) { + return ""; + } + if (useAuthentication) { allowRedirects = true; // per docs (MSC3916 always expects redirects) @@ -67,46 +87,31 @@ export function getHttpUriForMxc( // callers, hopefully. } - let serverAndMediaId = mxc.slice(6); // strips mxc:// let prefix: string; + const isThumbnailRequest = !!width || !!height || !!resizeMethod; + const verb = isThumbnailRequest ? "thumbnail" : "download"; if (useAuthentication) { - prefix = "/_matrix/client/v1/media/download/"; + prefix = `/_matrix/client/v1/media/${verb}`; } else { - prefix = "/_matrix/media/v3/download/"; + prefix = `/_matrix/media/v3/${verb}`; } - const params: Record = {}; + + const url = new URL(`${prefix}/${serverName}/${mediaId}`, baseUrl); if (width) { - params["width"] = Math.round(width).toString(); + url.searchParams.set("width", Math.round(width).toString()); } if (height) { - params["height"] = Math.round(height).toString(); + url.searchParams.set("height", Math.round(height).toString()); } if (resizeMethod) { - params["method"] = resizeMethod; - } - if (Object.keys(params).length > 0) { - // these are thumbnailing params so they probably want the - // thumbnailing API... - if (useAuthentication) { - prefix = "/_matrix/client/v1/media/thumbnail/"; - } else { - prefix = "/_matrix/media/v3/thumbnail/"; - } + url.searchParams.set("method", resizeMethod); } if (typeof allowRedirects === "boolean") { // We add this after, so we don't convert everything to a thumbnail request. - params["allow_redirect"] = JSON.stringify(allowRedirects); - } - - const fragmentOffset = serverAndMediaId.indexOf("#"); - let fragment = ""; - if (fragmentOffset >= 0) { - fragment = serverAndMediaId.slice(fragmentOffset); - serverAndMediaId = serverAndMediaId.slice(0, fragmentOffset); + url.searchParams.set("allow_redirect", JSON.stringify(allowRedirects)); } - const urlParams = Object.keys(params).length === 0 ? "" : "?" + encodeParams(params); - return baseUrl + prefix + serverAndMediaId + urlParams + fragment; + return url.href; } diff --git a/src/embedded.ts b/src/embedded.ts index be03037f974..bc32398e3b5 100644 --- a/src/embedded.ts +++ b/src/embedded.ts @@ -162,7 +162,7 @@ export class RoomWidgetClient extends MatrixClient { data: T, ): Promise => { try { - return await transportSend(action, data); + return await transportSend(action, data); } catch (error) { processAndThrow(error); } @@ -174,7 +174,7 @@ export class RoomWidgetClient extends MatrixClient { data: T, ): Promise => { try { - return await transportSendComplete(action, data); + return await transportSendComplete(action, data); } catch (error) { processAndThrow(error); } diff --git a/src/matrixrtc/MatrixRTCSession.ts b/src/matrixrtc/MatrixRTCSession.ts index a317495a95d..046591ede17 100644 --- a/src/matrixrtc/MatrixRTCSession.ts +++ b/src/matrixrtc/MatrixRTCSession.ts @@ -42,20 +42,6 @@ import { sleep } from "../utils.ts"; const logger = rootLogger.getChild("MatrixRTCSession"); -const MEMBERSHIP_EXPIRY_TIME = 60 * 60 * 1000; -const MEMBER_EVENT_CHECK_PERIOD = 2 * 60 * 1000; // How often we check to see if we need to re-send our member event -const CALL_MEMBER_EVENT_RETRY_DELAY_MIN = 3000; -const UPDATE_ENCRYPTION_KEY_THROTTLE = 3000; - -// A delay after a member leaves before we create and publish a new key, because people -// tend to leave calls at the same time -const MAKE_KEY_DELAY = 3000; -// The delay between creating and sending a new key and starting to encrypt with it. This gives others -// a chance to receive the new key to minimise the chance they don't get media they can't decrypt. -// The total time between a member leaving and the call switching to new keys is therefore -// MAKE_KEY_DELAY + SEND_KEY_DELAY -const USE_KEY_DELAY = 5000; - const getParticipantId = (userId: string, deviceId: string): string => `${userId}:${deviceId}`; const getParticipantIdFromMembership = (m: CallMembership): string => getParticipantId(m.sender!, m.deviceId); @@ -87,12 +73,15 @@ export type MatrixRTCSessionEventHandlerMap = { participantId: string, ) => void; }; + export interface JoinSessionConfig { - /** If true, generate and share a media key for this participant, + /** + * If true, generate and share a media key for this participant, * and emit MatrixRTCSessionEvent.EncryptionKeyChanged when * media keys for other participants become available. */ manageMediaKeys?: boolean; + /** Lets you configure how the events for the session are formatted. * - legacy: use one event with a membership array. * - MSC4143: use one event per membership (with only one membership per event) @@ -100,7 +89,64 @@ export interface JoinSessionConfig { * `CallMembershipDataLegacy` and `SessionMembershipData` */ useLegacyMemberEvents?: boolean; + + /** + * The timeout (in milliseconds) after we joined the call, that our membership should expire + * unless we have explicitly updated it. + */ + membershipExpiryTimeout?: number; + + /** + * The period (in milliseconds) with which we check that our membership event still exists on the + * server. If it is not found we create it again. + */ + memberEventCheckPeriod?: number; + + /** + * The minimum delay (in milliseconds) after which we will retry sending the membership event if it + * failed to send. + */ + callMemberEventRetryDelayMinimum?: number; + + /** + * The jitter (in milliseconds) which is added to callMemberEventRetryDelayMinimum before retrying + * sending the membership event. e.g. if this is set to 1000, then a random delay of between 0 and 1000 + * milliseconds will be added. + */ + callMemberEventRetryJitter?: number; + + /** + * The minimum time (in milliseconds) between each attempt to send encryption key(s). + * e.g. if this is set to 1000, then we will send at most one key event every second. + */ + updateEncryptionKeyThrottle?: number; + + /** + * The delay (in milliseconds) after a member leaves before we create and publish a new key, because people + * tend to leave calls at the same time. + */ + makeKeyDelay?: number; + + /** + * The delay (in milliseconds) between creating and sending a new key and starting to encrypt with it. This + * gives other a chance to receive the new key to minimise the chance they don't get media they can't decrypt. + * The total time between a member leaving and the call switching to new keys is therefore: + * makeKeyDelay + useKeyDelay + */ + useKeyDelay?: number; + + /** + * The timeout (in milliseconds) after which the server will consider the membership to have expired if it + * has not received a keep-alive from the client. + */ + membershipServerSideExpiryTimeout?: number; + + /** + * The period (in milliseconds) that the client will send membership keep-alives to the server. + */ + membershipKeepAlivePeriod?: number; } + /** * A MatrixRTCSession manages the membership & properties of a MatrixRTC session. * This class doesn't deal with media at all, just membership & properties of a session. @@ -109,10 +155,57 @@ export class MatrixRTCSession extends TypedEventEmitter Date.now() + this.lastEncryptionKeyUpdateRequest + this.updateEncryptionKeyThrottle > Date.now() ) { logger.info("Last encryption key event sent too recently: postponing"); if (this.keysEventUpdateTimeout === undefined) { - this.keysEventUpdateTimeout = setTimeout(this.sendEncryptionKeysEvent, UPDATE_ENCRYPTION_KEY_THROTTLE); + this.keysEventUpdateTimeout = setTimeout( + this.sendEncryptionKeysEvent, + this.updateEncryptionKeyThrottle, + ); } return; } @@ -772,6 +870,12 @@ export class MatrixRTCSession extends TypedEventEmitter => { + // TODO: Should this await on a shared promise? if (this.updateCallMembershipRunning) { this.needCallMembershipUpdate = true; return; @@ -1003,7 +1108,10 @@ export class MatrixRTCSession extends TypedEventEmitter => { try { - // TODO: If delayed event times out, re-join! const res = await resendIfRateLimited(() => this.client._unstable_sendDelayedStateEvent( this.room.roomId, { - delay: 8000, + delay: this.membershipServerSideExpiryTimeout, }, EventType.GroupCallMemberPrefix, {}, // leave event @@ -1044,6 +1151,20 @@ export class MatrixRTCSession extends TypedEventEmitter maxDelayAllowed + ) { + this.membershipServerSideExpiryTimeoutOverride = maxDelayAllowed; + return prepareDelayedDisconnection(); + } + } logger.error("Failed to prepare delayed disconnection event:", e); } }; @@ -1102,7 +1223,7 @@ export class MatrixRTCSession extends TypedEventEmitter => { diff --git a/src/rust-crypto/KeyClaimManager.ts b/src/rust-crypto/KeyClaimManager.ts index 3023de8c2f8..8cc1fd4e65c 100644 --- a/src/rust-crypto/KeyClaimManager.ts +++ b/src/rust-crypto/KeyClaimManager.ts @@ -50,7 +50,7 @@ export class KeyClaimManager { * Given a list of users, attempt to ensure that we have Olm Sessions active with each of their devices * * If we don't have an active olm session, we will claim a one-time key and start one. - * + * @param logger - logger to use * @param userList - list of userIDs to claim */ public ensureSessionsForUsers(logger: LogSpan, userList: Array): Promise { diff --git a/src/rust-crypto/rust-crypto.ts b/src/rust-crypto/rust-crypto.ts index 2d65e7f5c58..c2c9b8304f0 100644 --- a/src/rust-crypto/rust-crypto.ts +++ b/src/rust-crypto/rust-crypto.ts @@ -757,7 +757,7 @@ export class RustCrypto extends TypedEventEmitter { await this.crossSigningIdentity.bootstrapCrossSigning(opts); @@ -953,7 +953,7 @@ export class RustCrypto extends TypedEventEmitter { return this.eventDecryptor.getEncryptionInfoForEvent(event); @@ -1214,7 +1214,7 @@ export class RustCrypto extends TypedEventEmitter { return await this.backupManager.isKeyBackupTrusted(info); @@ -1223,7 +1223,7 @@ export class RustCrypto extends TypedEventEmitter { return await this.backupManager.checkKeyBackupAndEnable(true);