verificationhelper/sas: fix error when both sides send StartSAS

Signed-off-by: Sumner Evans <sumner.evans@automattic.com>

crypto/verificationhelper/sas.go

@@ -224,28 +224,29 @@ func (vh *VerificationHelper) onVerificationStartSAS(ctx context.Context, txn Ve
 	}
 	txn.MACMethod = macMethod
 	txn.EphemeralKey = &ECDHPrivateKey{ephemeralKey}
-	txn.StartEventContent = startEvt
 
-	commitment, err := calculateCommitment(ephemeralKey.PublicKey(), startEvt)
-	if err != nil {
-		return fmt.Errorf("failed to calculate commitment: %w", err)
-	}
+	if !txn.StartedByUs {
+		commitment, err := calculateCommitment(ephemeralKey.PublicKey(), txn)
+		if err != nil {
+			return fmt.Errorf("failed to calculate commitment: %w", err)
+		}
 
-	err = vh.sendVerificationEvent(ctx, txn, event.InRoomVerificationAccept, &event.VerificationAcceptEventContent{
-		Commitment:                commitment,
-		Hash:                      hashAlgorithm,
-		KeyAgreementProtocol:      keyAggreementProtocol,
-		MessageAuthenticationCode: macMethod,
-		ShortAuthenticationString: sasMethods,
-	})
-	if err != nil {
-		return fmt.Errorf("failed to send accept event: %w", err)
+		err = vh.sendVerificationEvent(ctx, txn, event.InRoomVerificationAccept, &event.VerificationAcceptEventContent{
+			Commitment:                commitment,
+			Hash:                      hashAlgorithm,
+			KeyAgreementProtocol:      keyAggreementProtocol,
+			MessageAuthenticationCode: macMethod,
+			ShortAuthenticationString: sasMethods,
+		})
+		if err != nil {
+			return fmt.Errorf("failed to send accept event: %w", err)
+		}
+		txn.VerificationState = VerificationStateSASAccepted
 	}
-	txn.VerificationState = VerificationStateSASAccepted
 	return vh.store.SaveVerificationTransaction(ctx, txn)
 }
 
-func calculateCommitment(ephemeralPubKey *ecdh.PublicKey, startEvt *event.VerificationStartEventContent) ([]byte, error) {
+func calculateCommitment(ephemeralPubKey *ecdh.PublicKey, txn VerificationTransaction) ([]byte, error) {
 	// The commitmentHashInput is the hash (encoded as unpadded base64) of the
 	// concatenation of the device's ephemeral public key (encoded as
 	// unpadded base64) and the canonical JSON representation of the
@@ -255,7 +256,7 @@ func calculateCommitment(ephemeralPubKey *ecdh.PublicKey, startEvt *event.Verifi
 	// hashing it, but we are just stuck on that.
 	commitmentHashInput := sha256.New()
 	commitmentHashInput.Write([]byte(base64.RawStdEncoding.EncodeToString(ephemeralPubKey.Bytes())))
-	encodedStartEvt, err := json.Marshal(startEvt)
+	encodedStartEvt, err := json.Marshal(txn.StartEventContent)
 	if err != nil {
 		return nil, err
 	}
@@ -339,7 +340,7 @@ func (vh *VerificationHelper) onVerificationKey(ctx context.Context, txn Verific
 
 	if txn.EphemeralPublicKeyShared {
 		// Verify that the commitment hash is correct
-		commitment, err := calculateCommitment(publicKey, txn.StartEventContent)
+		commitment, err := calculateCommitment(publicKey, txn)
 		if err != nil {
 			log.Err(err).Msg("Failed to calculate commitment")
 			return

crypto/verificationhelper/verificationhelper.go

@@ -818,6 +818,8 @@ func (vh *VerificationHelper) onVerificationStart(ctx context.Context, txn Verif
 	} else if txn.VerificationState != VerificationStateReady {
 		vh.cancelVerificationTxn(ctx, txn, event.VerificationCancelCodeUnexpectedMessage, "got start event for transaction that is not in ready state")
 		return
+	} else {
+		txn.StartEventContent = startEvt
 	}
 
 	switch startEvt.Method {
@@ -829,7 +831,7 @@ func (vh *VerificationHelper) onVerificationStart(ctx context.Context, txn Verif
 		}
 	case event.VerificationMethodReciprocate:
 		log.Info().Msg("Received reciprocate start event")
-		if !bytes.Equal(txn.QRCodeSharedSecret, startEvt.Secret) {
+		if !bytes.Equal(txn.QRCodeSharedSecret, txn.StartEventContent.Secret) {
 			vh.cancelVerificationTxn(ctx, txn, event.VerificationCancelCodeKeyMismatch, "reciprocated shared secret does not match")
 			return
 		}
@@ -842,8 +844,8 @@ func (vh *VerificationHelper) onVerificationStart(ctx context.Context, txn Verif
 		// Note that we should never get m.qr_code.show.v1 or m.qr_code.scan.v1
 		// here, since the start command for scanning and showing QR codes
 		// should be of type m.reciprocate.v1.
-		log.Error().Str("method", string(startEvt.Method)).Msg("Unsupported verification method in start event")
-		vh.cancelVerificationTxn(ctx, txn, event.VerificationCancelCodeUnknownMethod, fmt.Sprintf("unknown method %s", startEvt.Method))
+		log.Error().Str("method", string(txn.StartEventContent.Method)).Msg("Unsupported verification method in start event")
+		vh.cancelVerificationTxn(ctx, txn, event.VerificationCancelCodeUnknownMethod, fmt.Sprintf("unknown method %s", txn.StartEventContent.Method))
 	}
 }
 

crypto/verificationhelper/verificationhelper_sas_test.go

@@ -283,3 +283,80 @@ func TestVerification_SAS(t *testing.T) {
 		})
 	}
 }
+
+func TestVerification_SAS_BothCallStart(t *testing.T) {
+	ctx := log.Logger.WithContext(context.TODO())
+
+	ts, sendingClient, receivingClient, _, _, sendingMachine, receivingMachine := initServerAndLoginTwoAlice(t, ctx)
+	defer ts.Close()
+	sendingCallbacks, receivingCallbacks, sendingHelper, receivingHelper := initDefaultCallbacks(t, ctx, sendingClient, receivingClient, sendingMachine, receivingMachine)
+	var err error
+
+	var sendingRecoveryKey string
+	var sendingCrossSigningKeysCache *crypto.CrossSigningKeysCache
+
+	sendingRecoveryKey, sendingCrossSigningKeysCache, err = sendingMachine.GenerateAndUploadCrossSigningKeys(ctx, nil, "")
+	require.NoError(t, err)
+	assert.NotEmpty(t, sendingRecoveryKey)
+	assert.NotNil(t, sendingCrossSigningKeysCache)
+
+	// Send the verification request from the sender device and accept
+	// it on the receiving device and receive the verification ready
+	// event on the sending device.
+	txnID, err := sendingHelper.StartVerification(ctx, aliceUserID)
+	require.NoError(t, err)
+	ts.dispatchToDevice(t, ctx, receivingClient)
+	err = receivingHelper.AcceptVerification(ctx, txnID)
+	require.NoError(t, err)
+	ts.dispatchToDevice(t, ctx, sendingClient)
+
+	err = sendingHelper.StartSAS(ctx, txnID)
+	require.NoError(t, err)
+
+	err = receivingHelper.StartSAS(ctx, txnID)
+	require.NoError(t, err)
+
+	// Ensure that both devices have received the verification start event.
+	receivingInbox := ts.DeviceInbox[aliceUserID][receivingDeviceID]
+	assert.Len(t, receivingInbox, 1)
+	assert.Equal(t, txnID, receivingInbox[0].Content.AsVerificationStart().TransactionID)
+	sendingInbox := ts.DeviceInbox[aliceUserID][sendingDeviceID]
+	assert.Len(t, sendingInbox, 1)
+	assert.Equal(t, txnID, sendingInbox[0].Content.AsVerificationStart().TransactionID)
+
+	// Process the start event from the receiving client to the sending client.
+	ts.dispatchToDevice(t, ctx, sendingClient)
+	receivingInbox = ts.DeviceInbox[aliceUserID][receivingDeviceID]
+	assert.Len(t, receivingInbox, 2)
+	assert.Equal(t, txnID, receivingInbox[0].Content.AsVerificationStart().TransactionID)
+	assert.Equal(t, txnID, receivingInbox[1].Content.AsVerificationAccept().TransactionID)
+
+	// Process the rest of the events until we need to confirm the SAS.
+	for len(ts.DeviceInbox[aliceUserID][sendingDeviceID]) > 0 || len(ts.DeviceInbox[aliceUserID][receivingDeviceID]) > 0 {
+		ts.dispatchToDevice(t, ctx, receivingClient)
+		ts.dispatchToDevice(t, ctx, sendingClient)
+	}
+
+	// Confirm the SAS only the receiving device.
+	receivingHelper.ConfirmSAS(ctx, txnID)
+	ts.dispatchToDevice(t, ctx, sendingClient)
+
+	// Verification is not done until both devices confirm the SAS.
+	assert.False(t, sendingCallbacks.IsVerificationDone(txnID))
+	assert.False(t, receivingCallbacks.IsVerificationDone(txnID))
+
+	// Now, confirm it on the sending device.
+	sendingHelper.ConfirmSAS(ctx, txnID)
+
+	// Dispatching the events to the receiving device should get us to the done
+	// state on the receiving device.
+	ts.dispatchToDevice(t, ctx, receivingClient)
+	assert.False(t, sendingCallbacks.IsVerificationDone(txnID))
+	assert.True(t, receivingCallbacks.IsVerificationDone(txnID))
+
+	// Dispatching the events to the sending client should get us to the done
+	// state on the sending device.
+	ts.dispatchToDevice(t, ctx, sendingClient)
+	assert.True(t, sendingCallbacks.IsVerificationDone(txnID))
+	assert.True(t, receivingCallbacks.IsVerificationDone(txnID))
+}