From 211e4fd438d16761b86c26a0b923379325e8ea6a Mon Sep 17 00:00:00 2001 From: Erik Olsson Date: Mon, 13 Jan 2025 14:40:38 +0100 Subject: [PATCH] harden valid group info check --- core/mls/mls-tools/crates/mls/src/lib.rs | 42 ++++++++++++-- packages/sdk/src/tests/multi_ne/mls.test.ts | 62 +++++++++++++++++++++ 2 files changed, 100 insertions(+), 4 deletions(-) diff --git a/core/mls/mls-tools/crates/mls/src/lib.rs b/core/mls/mls-tools/crates/mls/src/lib.rs index 85a9fbeba..311f410b1 100644 --- a/core/mls/mls-tools/crates/mls/src/lib.rs +++ b/core/mls/mls-tools/crates/mls/src/lib.rs @@ -72,6 +72,18 @@ fn validate_group_info_message(group_info_message_bytes: Vec, expected_epoch ValidationResult::Valid } +fn validate_external_group_can_process_group_info_message(mut external_group: ExternalGroup, group_info_message_bytes: Vec) -> Result<(), ValidationResult> { + let group_info_message = match MlsMessage::from_bytes(&group_info_message_bytes) { + Ok(group_info_message) => group_info_message, + Err(_) => return Err(ValidationResult::InvalidGroupInfo) + }; + + match external_group.process_incoming_message(group_info_message) { + Ok(_) => return Ok(()), + Err(_) => Err(ValidationResult::InvalidGroupInfo.into()) + } +} + pub fn validate_initial_group_info_request(request: InitialGroupInfoRequest) -> InitialGroupInfoResponse { let external_client = create_external_client(); let external_group_snapshot = match ExternalSnapshot::from_bytes(&request.external_group_snapshot) { @@ -81,14 +93,14 @@ pub fn validate_initial_group_info_request(request: InitialGroupInfoRequest) -> } }; - let external_group = match external_client.load_group(external_group_snapshot) { + let mut external_group = match external_client.load_group(external_group_snapshot) { Ok(group) => group, Err(_) => return InitialGroupInfoResponse { result: ValidationResult::InvalidExternalGroup.into(), } }; - match validate_group_info_message(request.group_info_message, + match validate_group_info_message(request.group_info_message.clone(), 0, external_group.group_context().group_id()) { ValidationResult::Valid => {}, @@ -124,6 +136,14 @@ pub fn validate_initial_group_info_request(request: InitialGroupInfoRequest) -> result: ValidationResult::InvalidPublicSignatureKey.into(), }; } + + match validate_external_group_can_process_group_info_message(external_group, request.group_info_message) { + Ok(_) => {}, + Err(result) => return InitialGroupInfoResponse { + result: result.into(), + } + } + return InitialGroupInfoResponse { result: ValidationResult::Valid.into(), }; @@ -162,7 +182,7 @@ pub fn validate_external_join_request(request: ExternalJoinRequest) -> ExternalJ }; } - match validate_group_info_message(request.proposed_external_join_info_message, + match validate_group_info_message(request.proposed_external_join_info_message.clone(), external_group.group_context().epoch() + 1, external_group.group_context().group_id()) { ValidationResult::Valid => {}, @@ -211,6 +231,13 @@ pub fn validate_external_join_request(request: ExternalJoinRequest) -> ExternalJ true => {} } + match validate_external_group_can_process_group_info_message(external_group, request.proposed_external_join_info_message) { + Ok(_) => {}, + Err(result) => return ExternalJoinResponse { + result: result.into(), + } + } + return ExternalJoinResponse { result: ValidationResult::Valid.into(), }; @@ -312,7 +339,7 @@ pub fn validate_welcome_message_request(request: WelcomeMessageRequest) -> Welco }; } - match validate_group_info_message(request.group_info_message, + match validate_group_info_message(request.group_info_message.clone(), external_group.group_context().epoch() + 1, external_group.group_context().group_id()) { ValidationResult::Valid => {}, @@ -376,6 +403,13 @@ pub fn validate_welcome_message_request(request: WelcomeMessageRequest) -> Welco }; } + match validate_external_group_can_process_group_info_message(external_group, request.group_info_message) { + Ok(_) => {}, + Err(result) => return WelcomeMessageResponse { + result: result.into(), + } + } + return WelcomeMessageResponse { result: ValidationResult::Valid.into(), }; diff --git a/packages/sdk/src/tests/multi_ne/mls.test.ts b/packages/sdk/src/tests/multi_ne/mls.test.ts index 54bee5716..f17e2c284 100644 --- a/packages/sdk/src/tests/multi_ne/mls.test.ts +++ b/packages/sdk/src/tests/multi_ne/mls.test.ts @@ -268,6 +268,22 @@ describe('mlsTests', () => { ) }) + test('invalid group info for initialize group is rejected', async () => { + const { groupInfoMessage, externalGroupSnapshot } = + await createGroupInfoAndExternalSnapshot(bobMlsGroup) + // tamper with the message a little bit + const invalidGroupInfoMessage = groupInfoMessage + invalidGroupInfoMessage[invalidGroupInfoMessage.length - 2] += 1 // make it invalid + const payload = makeMlsPayloadInitializeGroup( + bobMlsClient.signaturePublicKey(), + externalGroupSnapshot, + groupInfoMessage, + ) + await expect(bobClient._debugSendMls(streamId, payload)).rejects.toThrow( + 'INVALID_GROUP_INFO', + ) + }) + test('clients can create MLS Groups in channels', async () => { const { groupInfoMessage, externalGroupSnapshot } = await createGroupInfoAndExternalSnapshot(bobMlsGroup) @@ -329,6 +345,26 @@ describe('mlsTests', () => { ) }) + test('Invalid group info for external commits is rejected', async () => { + const { commit, groupInfoMessage } = await commitExternal( + aliceMlsClient, + latestGroupInfoMessage, + latestExternalGroupSnapshot, + ) + // tamper with the message a little bit + const invalidGroupInfoMessage = groupInfoMessage + invalidGroupInfoMessage[invalidGroupInfoMessage.length - 2] += 1 // make it invalid + + const aliceMlsPayload = makeMlsPayloadExternalJoin( + aliceMlsClient.signaturePublicKey(), + commit, + invalidGroupInfoMessage, + ) + await expect(aliceClient._debugSendMls(streamId, aliceMlsPayload)).rejects.toThrow( + 'INVALID_GROUP_INFO', + ) + }) + test('Valid external commits are accepted', async () => { const { commit: aliceCommit, groupInfoMessage: aliceGroupInfoMessage } = await commitExternal( @@ -544,6 +580,32 @@ describe('mlsTests', () => { ) }) + test('invalid group info for welcome messages is rejected', async () => { + const mls = bobClient.streams.get(streamId)!.view.membershipContent.mls + const keyPackage = Object.values(mls.pendingKeyPackages)[0] + const kp = MlsMessage.fromBytes(keyPackage.keyPackage) + const commitOutput = await bobMlsGroup.addMember(kp) + await bobMlsGroup.clearPendingCommit() + const groupInfoMessage = commitOutput.externalCommitGroupInfo!.toBytes() + + // tamper with the message a little bit + const invalidGroupInfoMessage = groupInfoMessage + invalidGroupInfoMessage[invalidGroupInfoMessage.length - 2] += 1 // make it invalid + + const commit = commitOutput.commitMessage.toBytes() + const welcomeMessages = commitOutput.welcomeMessages.map((wm) => wm.toBytes()) + + const payload = makeMlsPayloadWelcomeMessage( + commit, + [keyPackage.signaturePublicKey], + invalidGroupInfoMessage, + welcomeMessages, + ) + await expect(aliceClient._debugSendMls(streamId, payload)).rejects.toThrow( + 'INVALID_GROUP_INFO', + ) + }) + test('clients can add other members from key packages', async () => { const mls = bobClient.streams.get(streamId)!.view.membershipContent.mls const keyPackage = Object.values(mls.pendingKeyPackages)[0]