From 4c53836a13decb23915654c19100950e6ccd7b7e Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Mon, 11 Nov 2024 06:19:48 -0500 Subject: [PATCH 1/7] Remove redundant type arguments in function call (#4507) as the types can be deduced by the function arguments. --- src/embedded.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/embedded.ts b/src/embedded.ts index be03037f97..bc32398e3b 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); } From 5033d48013262278bd5d5867010bced9e4ee8193 Mon Sep 17 00:00:00 2001 From: Florian Duros Date: Mon, 11 Nov 2024 14:15:46 +0100 Subject: [PATCH 2/7] Fix tsdoc error in rust-crypto folder (#4504) --- src/rust-crypto/KeyClaimManager.ts | 2 +- src/rust-crypto/rust-crypto.ts | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/rust-crypto/KeyClaimManager.ts b/src/rust-crypto/KeyClaimManager.ts index 3023de8c2f..8cc1fd4e65 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 2d65e7f5c5..c2c9b8304f 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); From 6855ace6422082d173438cb23368d2fabc6a1086 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Mon, 11 Nov 2024 10:07:33 -0500 Subject: [PATCH 3/7] When state says you've left ongoing call, rejoin (#4342) * When state says you've left ongoing call, rejoin When receiving a state change that says you are no longer a member of a RTC session that you are actually still participating in, send another state event to put yourself back in the session membership. This can happen when an administrator overwrites your call membership event (which is allowed even with MSC3757's restrictions on state), or if your delayed disconnection event (via MSC4140) timed out before your client could send a heartbeat to delay it further. * Don't emit state changed on join recovery --- src/matrixrtc/MatrixRTCSession.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/matrixrtc/MatrixRTCSession.ts b/src/matrixrtc/MatrixRTCSession.ts index a317495a95..b8274fdbaa 100644 --- a/src/matrixrtc/MatrixRTCSession.ts +++ b/src/matrixrtc/MatrixRTCSession.ts @@ -772,6 +772,12 @@ export class MatrixRTCSession extends TypedEventEmitter => { + // TODO: Should this await on a shared promise? if (this.updateCallMembershipRunning) { this.needCallMembershipUpdate = true; return; @@ -1030,7 +1037,6 @@ 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, From 581b3209ab0f4cfdcc6fae1a0175b7b5868e2e45 Mon Sep 17 00:00:00 2001 From: Hugh Nimmo-Smith Date: Mon, 11 Nov 2024 15:35:05 +0000 Subject: [PATCH 4/7] Allow configuration of MatrixRTC timers when calling joinRoomSession() (#4510) --- spec/unit/matrixrtc/MatrixRTCSession.spec.ts | 30 ++++ src/matrixrtc/MatrixRTCSession.ts | 151 +++++++++++++++---- 2 files changed, 151 insertions(+), 30 deletions(-) diff --git a/spec/unit/matrixrtc/MatrixRTCSession.spec.ts b/spec/unit/matrixrtc/MatrixRTCSession.spec.ts index 96b1db9b03..6b45a93025 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" }; diff --git a/src/matrixrtc/MatrixRTCSession.ts b/src/matrixrtc/MatrixRTCSession.ts index b8274fdbaa..5d4d90ea58 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,47 @@ 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; } @@ -799,7 +887,7 @@ export class MatrixRTCSession extends TypedEventEmitter => { From 35d862ebd3e198e1ac2a02a5ed268f82890c9980 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Mon, 11 Nov 2024 15:48:53 -0500 Subject: [PATCH 5/7] Handle M_MAX_DELAY_EXCEEDED errors (#4511) * Handle M_MAX_DELAY_EXCEEDED errors Use a lower delay time if the server rejects a delay as too long. * Add test * Lint test * Update src/matrixrtc/MatrixRTCSession.ts Co-authored-by: Robin * Test computed expiry timeout value --------- Co-authored-by: Robin --- spec/unit/matrixrtc/MatrixRTCSession.spec.ts | 22 ++++++++++++++++- src/matrixrtc/MatrixRTCSession.ts | 26 +++++++++++++++++++- 2 files changed, 46 insertions(+), 2 deletions(-) diff --git a/spec/unit/matrixrtc/MatrixRTCSession.spec.ts b/spec/unit/matrixrtc/MatrixRTCSession.spec.ts index 6b45a93025..fb62a669c7 100644 --- a/spec/unit/matrixrtc/MatrixRTCSession.spec.ts +++ b/spec/unit/matrixrtc/MatrixRTCSession.spec.ts @@ -508,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" }); @@ -541,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/matrixrtc/MatrixRTCSession.ts b/src/matrixrtc/MatrixRTCSession.ts index 5d4d90ea58..046591ede1 100644 --- a/src/matrixrtc/MatrixRTCSession.ts +++ b/src/matrixrtc/MatrixRTCSession.ts @@ -184,8 +184,18 @@ export class MatrixRTCSession extends TypedEventEmitter maxDelayAllowed + ) { + this.membershipServerSideExpiryTimeoutOverride = maxDelayAllowed; + return prepareDelayedDisconnection(); + } + } logger.error("Failed to prepare delayed disconnection event:", e); } }; From 00aba742e442f59cdb9e21f74c7a6647d075cc40 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Tue, 12 Nov 2024 09:08:00 +0000 Subject: [PATCH 6/7] Merge commit from fork to avoid path traversal attacks and remove the legacy allowance for fragments in MXCs Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> --- spec/unit/content-repo.spec.ts | 39 ++++++++++++-------- src/content-repo.ts | 65 ++++++++++++++++++---------------- 2 files changed, 60 insertions(+), 44 deletions(-) diff --git a/spec/unit/content-repo.spec.ts b/spec/unit/content-repo.spec.ts index 23f504c1e4..8d24107fe2 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/src/content-repo.ts b/src/content-repo.ts index b6174b6d8d..b0b02a0569 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; } From 4d4ff4c3f2cb4d2c95d26ca17579cd28881f2966 Mon Sep 17 00:00:00 2001 From: RiotRobot Date: Tue, 12 Nov 2024 09:13:26 +0000 Subject: [PATCH 7/7] v34.11.0 --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index cc16d3f96b..e11ba1a5dd 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