WPB-18695 Return a specific error for leaf signature verification failure (#4665)

Author: battermann

changelog.d/5-internal/WPB-18695

@@ -0,0 +1 @@
+Dedicated error label for MLS leaf node signature validation failure

integration/test/Test/MLS.hs

@@ -4,6 +4,8 @@ module Test.MLS where
 
 import API.Brig (claimKeyPackages, deleteClient)
 import API.Galley
+import Data.Bits
+import qualified Data.ByteString as B
 import qualified Data.ByteString.Base64 as Base64
 import qualified Data.ByteString.Char8 as B8
 import qualified Data.Map as Map
@@ -941,3 +943,30 @@ testRemoteAddLegacy domain = do
     void $ uploadNewKeyPackage suite bob1
     convId <- createNewGroup suite alice1
     void $ createAddCommit alice1 convId [alice, bob] >>= sendAndConsumeCommitBundle
+
+testInvalidLeafNodeSignature :: (HasCallStack) => App ()
+testInvalidLeafNodeSignature = do
+  alice <- randomUser OwnDomain def
+  [creator, other] <- traverse (createMLSClient def) (replicate 2 alice)
+  (_, conv) <- createSelfGroup def creator
+  convId <- objConvId conv
+  void $ createAddCommit creator convId [alice] >>= sendAndConsumeCommitBundle
+
+  void $ uploadNewKeyPackage def other
+
+  mp <- createExternalCommit convId other Nothing
+  bindResponse (postMLSCommitBundle other (mkBundle mp {message = makeSignatureCorrupt mp.message})) $ \resp -> do
+    resp.status `shouldMatchInt` 400
+    resp.json %. "label" `shouldMatch` "mls-invalid-leaf-node-signature"
+  where
+    -- This is a hack to make the signature invalid.
+    -- It works as long as the format of the MLS message does not change
+    -- in any way that changes the offset of the signature.
+    -- If this test ever starts flaking, we should consider
+    -- factoring the MLS code out of wire-api into a separate shared package
+    -- and use it in this test to invalidate the signature.
+    makeSignatureCorrupt :: ByteString -> ByteString
+    makeSignatureCorrupt bs = case B.splitAt 0xb0 bs of
+      (left, right) -> case B.uncons right of
+        Just (h, t) -> left <> B.singleton (h `xor` 0x01) <> t
+        Nothing -> bs

libs/wire-api/src/Wire/API/Error/Brig.hs

@@ -119,6 +119,7 @@ data BrigError
   | UserGroupNotATeamAdmin
   | UserGroupMemberIsNotInTheSameTeam
   | DuplicateEntry
+  | MLSInvalidLeafNodeSignature
 
 instance (Typeable (MapError e), KnownError (MapError e)) => IsSwaggerError (e :: BrigError) where
   addToOpenApi = addStaticErrorToSwagger @(MapError e)
@@ -355,3 +356,5 @@ type instance MapError 'UserGroupNotATeamAdmin = 'StaticError 403 "user-group-wr
 type instance MapError 'UserGroupMemberIsNotInTheSameTeam = 'StaticError 400 "user-group-invalid" "Only team members of the same team can be added to a user group."
 
 type instance MapError 'DuplicateEntry = 'StaticError 409 "duplicate-entry" "Entry already exists"
+
+type instance MapError 'MLSInvalidLeafNodeSignature = 'StaticError 400 "mls-invalid-leaf-node-signature" "Invalid leaf node signature in key package"

libs/wire-api/src/Wire/API/Error/Galley.hs

@@ -151,6 +151,7 @@ data GalleyError
   | ChannelsNotEnabled
   | NotAnMlsConversation
   | MLSReadReceiptsNotAllowed
+  | MLSInvalidLeafNodeSignature
   deriving (Show, Eq, Generic)
   deriving (FromJSON, ToJSON) via (CustomEncoded GalleyError)
 
@@ -347,6 +348,8 @@ type instance MapError 'NotAnMlsConversation = 'StaticError 403 "not-mls-convers
 
 type instance MapError 'MLSReadReceiptsNotAllowed = 'StaticError 403 "mls-receipts-not-allowed" "Read receipts on MLS conversations are not allowed"
 
+type instance MapError 'MLSInvalidLeafNodeSignature = 'StaticError 400 "mls-invalid-leaf-node-signature" "Invalid leaf node signature"
+
 --------------------------------------------------------------------------------
 -- Team Member errors
 

libs/wire-api/src/Wire/API/MLS/Validation.hs

@@ -19,16 +19,13 @@ module Wire.API.MLS.Validation
   ( -- * Main key package validation function
     validateKeyPackage,
     validateLeafNode,
+    ValidationError (..),
   )
 where
 
 import Control.Applicative
 import Control.Error.Util
 import Data.ByteArray qualified as BA
-import Data.Text qualified as T
-import Data.Text.Lazy qualified as LT
-import Data.Text.Lazy.Builder qualified as LT
-import Data.Text.Lazy.Builder.Int qualified as LT
 import Data.X509 qualified as X509
 import Imports
 import Wire.API.MLS.Capabilities
@@ -39,20 +36,17 @@ import Wire.API.MLS.LeafNode
 import Wire.API.MLS.Lifetime
 import Wire.API.MLS.ProtocolVersion
 import Wire.API.MLS.Serialisation
+import Wire.API.MLS.Validation.Error
 
 validateKeyPackage ::
   Maybe ClientIdentity ->
   KeyPackage ->
-  Either Text (CipherSuiteTag, Lifetime)
+  Either ValidationError (CipherSuiteTag, Lifetime)
 validateKeyPackage mIdentity kp = do
   -- get ciphersuite
   cs <-
     maybe
-      ( Left
-          ( "Unsupported ciphersuite 0x"
-              <> LT.toStrict (LT.toLazyText (LT.hexadecimal kp.cipherSuite.cipherSuiteNumber))
-          )
-      )
+      (Left (UnsupportedCipherSuite kp.cipherSuite.cipherSuiteNumber))
       pure
       $ cipherSuiteTag kp.cipherSuite
 
@@ -65,11 +59,11 @@ validateKeyPackage mIdentity kp = do
         kp.tbs
         kp.signature_
     )
-    $ Left "Invalid KeyPackage signature"
+    $ Left InvalidKeyPackageSignature
 
   -- validate protocol version
   maybe
-    (Left "Unsupported protocol version")
+    (Left UnsupportedProtocolVersion)
     pure
     (pvTag (kp.protocolVersion) >>= guard . (== ProtocolMLS10))
 
@@ -79,7 +73,7 @@ validateKeyPackage mIdentity kp = do
   lt <- case kp.leafNode.source of
     LeafNodeSourceKeyPackage lt -> pure lt
     -- unreachable
-    _ -> Left "Unexpected leaf node source"
+    _ -> Left UnexpectedLeafNodeSource
 
   pure (cs, lt)
 
@@ -88,7 +82,7 @@ validateLeafNode ::
   Maybe ClientIdentity ->
   LeafNodeTBSExtra ->
   LeafNode ->
-  Either Text ()
+  Either ValidationError ()
 validateLeafNode cs mIdentity extra leafNode = do
   let tbs = LeafNodeTBS leafNode.core extra
   unless
@@ -99,27 +93,25 @@ validateLeafNode cs mIdentity extra leafNode = do
         (mkRawMLS tbs)
         leafNode.signature_
     )
-    $ Left "Invalid LeafNode signature"
+    $ Left InvalidLeafNodeSignature
 
   validateCredential cs leafNode.signatureKey mIdentity leafNode.credential
   validateSource extra.tag leafNode.source
   validateCapabilities (credentialTag leafNode.credential) leafNode.capabilities
 
-validateCredential :: CipherSuiteTag -> ByteString -> Maybe ClientIdentity -> Credential -> Either Text ()
+validateCredential :: CipherSuiteTag -> ByteString -> Maybe ClientIdentity -> Credential -> Either ValidationError ()
 validateCredential cs pkey mIdentity cred = do
   -- FUTUREWORK: check signature in the case of an x509 credential
   (identity, mkey) <-
     either credentialError pure $
       credentialIdentityAndKey cred
   traverse_ (validateCredentialKey (csSignatureScheme cs) pkey) mkey
   unless (maybe True (identity ==) mIdentity) $
-    Left "client identity does not match credential identity"
+    Left IdentityMismatch
   where
-    credentialError e =
-      Left $
-        "Failed to parse identity: " <> e
+    credentialError e = Left $ FailedToParseIdentity e
 
-validateCredentialKey :: SignatureSchemeTag -> ByteString -> X509.PubKey -> Either Text ()
+validateCredentialKey :: SignatureSchemeTag -> ByteString -> X509.PubKey -> Either ValidationError ()
 validateCredentialKey Ed25519 pk1 (X509.PubKeyEd25519 pk2) = validateCredentialKeyBS pk1 (BA.convert pk2)
 validateCredentialKey Ecdsa_secp256r1_sha256 pk1 (X509.PubKeyEC pk2) =
   case pk2.pubkeyEC_pub of
@@ -131,28 +123,28 @@ validateCredentialKey Ecdsa_secp521r1_sha512 pk1 (X509.PubKeyEC pk2) =
   case pk2.pubkeyEC_pub of
     X509.SerializedPoint bs -> validateCredentialKeyBS pk1 bs
 validateCredentialKey ss _ _ =
-  Left $
-    "Certificate signature scheme " <> T.pack (show ss) <> " does not match client's public key"
+  Left $ SchemeMismatch ss
 
-validateCredentialKeyBS :: ByteString -> ByteString -> Either Text ()
+validateCredentialKeyBS :: ByteString -> ByteString -> Either ValidationError ()
 validateCredentialKeyBS pk1 pk2 =
-  note "Certificate public key does not match client's" $
+  note PublicKeyMismatch $
     guard (pk1 == pk2)
 
-validateSource :: LeafNodeSourceTag -> LeafNodeSource -> Either Text ()
+validateSource :: LeafNodeSourceTag -> LeafNodeSource -> Either ValidationError ()
 validateSource t s = do
   let t' = leafNodeSourceTag s
   if t == t'
     then pure ()
     else
       Left $
-        "Expected '"
-          <> t.name
-          <> "' source, got '"
-          <> t'.name
-          <> "'"
+        LeafNodeSourceTagMisMatch $
+          "Expected '"
+            <> t.name
+            <> "' source, got '"
+            <> t'.name
+            <> "'"
 
-validateCapabilities :: CredentialTag -> Capabilities -> Either Text ()
+validateCapabilities :: CredentialTag -> Capabilities -> Either ValidationError ()
 validateCapabilities ctag caps =
   unless (fromMLSEnum ctag `elem` caps.credentials) $
-    Left "missing BasicCredential capability"
+    Left BasicCredentialCapabilityMissing

libs/wire-api/src/Wire/API/MLS/Validation/Error.hs

@@ -0,0 +1,52 @@
+-- This file is part of the Wire Server implementation.
+--
+-- Copyright (C) 2025 Wire Swiss GmbH <[email protected]>
+--
+-- This program is free software: you can redistribute it and/or modify it under
+-- the terms of the GNU Affero General Public License as published by the Free
+-- Software Foundation, either version 3 of the License, or (at your option) any
+-- later version.
+--
+-- This program is distributed in the hope that it will be useful, but WITHOUT
+-- ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
+-- FOR A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
+-- details.
+--
+-- You should have received a copy of the GNU Affero General Public License along
+-- with this program. If not, see <https://www.gnu.org/licenses/>.
+
+module Wire.API.MLS.Validation.Error where
+
+import Data.Text qualified as T
+import Data.Text.Lazy qualified as LT
+import Data.Text.Lazy.Builder qualified as LT
+import Data.Text.Lazy.Builder.Int qualified as LT
+import Imports
+import Wire.API.MLS.CipherSuite (SignatureSchemeTag)
+
+data ValidationError
+  = InvalidKeyPackageSignature
+  | UnsupportedCipherSuite Word16
+  | InvalidLeafNodeSignature
+  | FailedToParseIdentity Text
+  | SchemeMismatch SignatureSchemeTag
+  | LeafNodeSourceTagMisMatch Text
+  | UnsupportedProtocolVersion
+  | UnexpectedLeafNodeSource
+  | IdentityMismatch
+  | PublicKeyMismatch
+  | BasicCredentialCapabilityMissing
+  deriving (Show, Eq, Generic)
+
+toText :: ValidationError -> Text
+toText InvalidKeyPackageSignature = "Invalid KeyPackage signature"
+toText (UnsupportedCipherSuite cs) = "Unsupported ciphersuite 0x" <> LT.toStrict (LT.toLazyText (LT.hexadecimal cs))
+toText InvalidLeafNodeSignature = "Invalid LeafNode signature"
+toText (FailedToParseIdentity err) = "Failed to parse identity: " <> err
+toText (SchemeMismatch ss) = "Certificate signature scheme " <> T.pack (show ss) <> " does not match client's public key"
+toText (LeafNodeSourceTagMisMatch err) = err
+toText UnsupportedProtocolVersion = "Unsupported protocol version"
+toText UnexpectedLeafNodeSource = "Unexpected leaf node source"
+toText IdentityMismatch = "Client identity does not match credential identity"
+toText PublicKeyMismatch = "Certificate public key does not match client's"
+toText BasicCredentialCapabilityMissing = "Missing BasicCredential capability"

libs/wire-api/src/Wire/API/Routes/Public/Galley/MLS.hs

@@ -80,6 +80,7 @@ type MLSMessagingAPI =
         :> CanThrow MLSProposalFailure
         :> CanThrow NonFederatingBackends
         :> CanThrow UnreachableBackends
+        :> CanThrow 'MLSInvalidLeafNodeSignature
         :> "messages"
         :> ZLocalUser
         :> ZClient
@@ -116,6 +117,7 @@ type MLSMessagingAPI =
                :> CanThrow NonFederatingBackends
                :> CanThrow UnreachableBackends
                :> CanThrow GroupIdVersionNotSupported
+               :> CanThrow MLSInvalidLeafNodeSignature
                :> "commit-bundles"
                :> ZLocalUser
                :> ZClient

libs/wire-api/wire-api.cabal

@@ -137,6 +137,7 @@ library
     Wire.API.MLS.Servant
     Wire.API.MLS.SubConversation
     Wire.API.MLS.Validation
+    Wire.API.MLS.Validation.Error
     Wire.API.MLS.Welcome
     Wire.API.Notification
     Wire.API.OAuth

services/brig/src/Brig/API/MLS/KeyPackages/Validation.hs

@@ -36,19 +36,21 @@ import Data.Time.Clock.POSIX
 import Imports
 import Wire.API.Error
 import Wire.API.Error.Brig
+import Wire.API.Error.Brig qualified as E
 import Wire.API.MLS.CipherSuite
 import Wire.API.MLS.Credential
 import Wire.API.MLS.KeyPackage
 import Wire.API.MLS.Lifetime
 import Wire.API.MLS.Serialisation
 import Wire.API.MLS.Validation
+import Wire.API.MLS.Validation.Error (toText)
 
 validateUploadedKeyPackage ::
   ClientIdentity ->
   RawMLS KeyPackage ->
   Handler r (KeyPackageRef, CipherSuiteTag, KeyPackageData)
 validateUploadedKeyPackage identity kp = do
-  (cs, lt) <- either mlsProtocolError pure $ validateKeyPackage (Just identity) kp.value
+  (cs, lt) <- either mlsProtocolErrorFromValidationError pure $ validateKeyPackage (Just identity) kp.value
 
   validateLifetime lt
 
@@ -95,6 +97,10 @@ validateLifetime' now mMaxLifetime lt = do
     when (tsPOSIX (ltNotAfter lt) > now + maxLifetime) $
       Left "Key package expiration time is too far in the future"
 
+mlsProtocolErrorFromValidationError :: ValidationError -> Handler r a
+mlsProtocolErrorFromValidationError InvalidLeafNodeSignature = throwStd (errorToWai @E.MLSInvalidLeafNodeSignature)
+mlsProtocolErrorFromValidationError err = mlsProtocolError (toText err)
+
 mlsProtocolError :: Text -> Handler r a
 mlsProtocolError msg =
   throwStd . dynErrorToWai $

services/galley/src/Galley/API/MLS/Commit/Core.hs

@@ -67,6 +67,7 @@ import Wire.API.MLS.LeafNode
 import Wire.API.MLS.Serialisation
 import Wire.API.MLS.SubConversation
 import Wire.API.MLS.Validation
+import Wire.API.MLS.Validation.Error (toText)
 import Wire.API.User.Client
 import Wire.NotificationSubsystem
 
@@ -102,7 +103,8 @@ type HasProposalActionEffects r =
 
 getCommitData ::
   ( HasProposalEffects r,
-    Member (ErrorS 'MLSProposalNotFound) r
+    Member (ErrorS 'MLSProposalNotFound) r,
+    Member (ErrorS MLSInvalidLeafNodeSignature) r
   ) =>
   SenderIdentity ->
   Local ConvOrSubConv ->
@@ -246,7 +248,8 @@ checkUpdatePath ::
     Member (Error MLSProtocolError) r,
     Member (Error FederationError) r,
     Member BrigAccess r,
-    Member FederatorAccess r
+    Member FederatorAccess r,
+    Member (ErrorS MLSInvalidLeafNodeSignature) r
   ) =>
   Local ConvOrSubConv ->
   SenderIdentity ->
@@ -257,9 +260,10 @@ checkUpdatePath lConvOrSub senderIdentity ciphersuite path = for_ senderIdentity
   let groupId = cnvmlsGroupId (tUnqualified lConvOrSub).mlsMeta
   let extra = LeafNodeTBSExtraCommit groupId index
   case validateLeafNode ciphersuite (Just senderIdentity.client) extra path.leaf.value of
+    Left InvalidLeafNodeSignature -> throwS @'MLSInvalidLeafNodeSignature
     Left errMsg ->
       throw $
-        mlsProtocolError ("Tried to add invalid LeafNode: " <> errMsg)
+        mlsProtocolError ("Tried to add invalid LeafNode: " <> toText errMsg)
     Right _ -> pure ()
   clientInfo <-
     getSingleClientInfo