Skip to content

fix(crypto): fix in-room verification encryption, echo filtering, and commitment calculation - #555

Open
underhax wants to merge 1 commit into
mautrix:mainfrom
underhax:fix-in-room-encryption
Open

fix(crypto): fix in-room verification encryption, echo filtering, and commitment calculation#555
underhax wants to merge 1 commit into
mautrix:mainfrom
underhax:fix-in-room-encryption

Conversation

@underhax

Copy link
Copy Markdown

Summary

Fixes in-room SAS verification failing with unexpected message cancellations or m.key_mismatch commitment errors when communicating with Matrix clients like Element / matrix-rust-sdk.

Problem Description

  1. Unencrypted In-Room Events: StartInRoomVerification and sendVerificationEvent sent in-room verification events in plaintext instead of encrypting them as m.room.encrypted in encrypted rooms.
  2. Self-Echo Filtering & Transaction ID Extraction: In-room verification events lacked transaction ID routing from m.relates_to.event_id and did not ignore echoed events from our own device/user, leading to state machine collisions and premature cancellations (m.unexpected_message).
  3. Commitment Mismatch (m.key_mismatch): SAS commitment verification failed because event.RelatesTo.IsFallingBack serialized "is_falling_back": false into the canonical JSON object of m.relates_to.

Solution

  • Encrypted in-room verification requests and all subsequent verification flow events via Crypto.Encrypt when encryption is enabled.
  • Updated wrapHandler to extract transactionID from m.relates_to.event_id and ignore echoed events from our own device/user.
  • Ignored own ephemeral public key events in onVerificationKey.
  • Added omitzero tag to RelatesTo.IsFallingBack to prevent serializing zero-valued booleans in canonical JSON.
  • Introduced canonicalMarshal and matchCommitment for strict canonical JSON hashing and resilient SAS commitment verification.
  • Updated MockServer.DispatchToDevice to dispatch to-device encryption events to OlmMachine.HandleToDeviceEvent.
  • Added unit test TestInRoomVerification_EventEncryptionAndType to verificationhelper_test.go.

Checklist

@f4rr0l1v3r

Copy link
Copy Markdown

Independent confirmation of the transaction ID routing problem, plus one dead branch in the same handler that this PR doesn't touch yet.

We ran into this with a small bot (mautrix-go v0.30.0, goolm build tag, Synapse behind MAS) in an encrypted two-person room, verifying from Element Desktop via "Verify User". The flow always stopped after our ready. We measured both variants: when the bot started SAS, Element answered with m.key.verification.accept and the bot never reacted. When Element sent m.key.verification.start itself, same silence. A separate handler registered on the same event types confirmed the events do reach the process.

Reading the code we arrived at the same conclusion as point 2 of this PR: wrapHandler uses evt.ID as the transaction ID for any event that has one, so incoming in-room accept/start/key/mac are looked up under their own event ID instead of the request's event ID and get dropped as unknown transactions. The log line that gave it away was failed to get verification transaction error="unknown transaction ID" with transaction_id equal to event_id.

One thing this PR does not seem to change: the cancellation path for unknown transactions is unreachable. In Init's wrapHandler:

txn, err := vh.store.GetVerificationTransaction(ctx, transactionID)
if err != nil && errors.Is(err, ErrUnknownVerificationTransaction) {
	log.Err(err).Msg("failed to get verification transaction")
	vh.activeTransactionsLock.Unlock()
	return
} else if errors.Is(err, ErrUnknownVerificationTransaction) {
	// builds a fake transaction and sends m.unknown_transaction — never reached

The first condition already matches every unknown-transaction error, so the helper never sends m.unknown_transaction to the other side. The peer only sees its own timeout; Element then shows the misleading "request timed out / was denied / verification mismatch" message. Might be worth folding into this PR, since the branch only becomes reachable once the routing fix lands.

We can test this branch against Element Desktop on our homeserver if that helps the review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants