Tutorial usage of CheckTx for message validation is insecure

[UnitylChaos]

Tendermint only uses CheckTx as a local filter to decide whether or not a transaction should be included in the mempool for a future proposal. But when another validator proposes a block, we never run CheckTx on those transactions, and only run DeliverTx when the block is committed. This could be abused to sneak in transactions which would fail CheckTx, but which DeliverTx can still handle.

The tutorial as currently written uses the ValidateMessage class to define validity constraints on messages, and these are then run through defaultCheckTx. Since these checks are not run at the start of DeliverTx, it would be possible for a malicious proposer to create a transaction signed by address A, which deletes a name held by address B.

[UnitylChaos]

Umm also, and this might need to be a separate issue, but it looks to me like the Bank module is not doing any sort of author checking.

kepler/hs-abci-sdk/src/Tendermint/SDK/Modules/Bank/Messages.hs

Lines 51 to 52 in fe68362

	instance ValidateMessage TransferMsg where
	validateMessage _ = Success ()

kepler/hs-abci-sdk/src/Tendermint/SDK/Modules/Bank/Router.hs

Lines 26 to 33 in fe68362

	transferH
	:: Members BankEffs r
	=> RoutingTx TransferMsg
	-> Sem r ()
	transferH (RoutingTx Tx{txMsg=Msg{msgData}}) =
	let TransferMsg{..} = msgData
	coin = Coin transferCoinId transferAmount
	in transfer transferFrom coin transferTo

kepler/hs-abci-sdk/src/Tendermint/SDK/Modules/Bank/Keeper.hs

Lines 63 to 82 in fe68362

	transferF addr1 (Auth.Coin cid amount) addr2 = do
	-- check if addr1 has amt
	(Auth.Coin _ addr1Bal) <- getCoinBalance addr1 cid
	if addr1Bal >= amount
	then do
	(Auth.Coin _ addr2Bal) <- getCoinBalance addr2 cid
	let newCoinBalance1 = Auth.Coin cid (addr1Bal - amount)
	newCoinBalance2 = Auth.Coin cid (addr2Bal + amount)
	-- update both balances
	putCoinBalance addr1 newCoinBalance1
	putCoinBalance addr2 newCoinBalance2
	let event = TransferEvent
	{ transferEventAmount = amount
	, transferEventCoinId = cid
	, transferEventTo = addr2
	, transferEventFrom = addr1
	}
	BaseApp.emit event
	BaseApp.logEvent event
	else throw @BankError (InsufficientFunds "Insufficient funds for transfer.")

[martyall]

RE Bank module, good find!

I see what you're saying about checkTx -- a node can run transactions during block processing that it's never seen before, that were only seen by the node that's proposing the block. In that case we need to run those checks as part of the deliver phase.

Nothing comes to mind immediately, but I'm sure this will be easy enough to fix. Do you have a proposal for how it will work?

[UnitylChaos]

I would say the simplest thing to do would be to call validateMessage before passing the data to the keeper handler. This could be done manually on every router function, such as:

kepler/hs-abci-docs/nameservice/src/Nameservice/Modules/Nameservice/Router.hs

Lines 34 to 41 in fe68362

	buyNameH
	:: Members NameserviceEffs r
	=> Members BaseEffs r
	=> RoutingTx BuyNameMsg
	-> Sem r ()
	buyNameH (RoutingTx Tx{txMsg=Msg{msgData}}) = do
	incCount "buy_total"
	withTimer "buy_duration_seconds" $ buyName msgData

Or there could be a requirement on all transaction messages that they have a ValidateMessage instance, and call it earlier for all tx.
Then it could be called in the transaction router, somewhere around here:

kepler/hs-abci-sdk/src/Tendermint/SDK/BaseApp/Transaction/Router.hs

Lines 66 to 80 in aba9914

	instance ( HasMessageType msg, HasCodec msg, HasCodec (OnCheckReturn c oc a), Member (Embed IO) r) => HasTxRouter (TypedMessage msg :~> Return' oc a) r c where
	type RouteTx (TypedMessage msg :~> Return' oc a) r c = RoutingTx msg -> Sem (TxEffs :& r) (OnCheckReturn c oc a)
	routeTx _ _ _ subserver =
	let f (RoutingTx tx@Tx{txMsg}) =
	if msgType txMsg == mt
	then case decode $ msgData txMsg of
	Left e -> R.delayedFail $
	R.InvalidRequest ("Failed to parse message of type " <> mt <> ": " <> e <> ".")
	Right a -> pure . RoutingTx $ tx {txMsg = txMsg {msgData = a}}
	else R.delayedFail R.PathNotFound
	in methodRouter $
	R.addBody subserver $ R.withRequest f
	where mt = messageType (Proxy :: Proxy msg)

Or even earlier in parseTx, at the same time that msgAuthor is extracted from the signature. (this would have the side effect of making defaultCheckTx redundant, since ValidateMessage checks would be automatically performed on both paths)

kepler/hs-abci-sdk/src/Tendermint/SDK/Types/Transaction.hs

Lines 104 to 123 in fe68362

	parseTx p bs = do
	rawTx@RawTransaction{..} <- decode bs
	recSig <- note "Unable to parse transaction signature as a recovery signature." $
	makeRecoverableSignature p rawTransactionSignature
	let txForSigning = rawTx {rawTransactionSignature = ""}
	signBytes = makeDigest txForSigning
	signerPubKey <- note "Signature recovery failed." $ recover p recSig signBytes
	return $ Tx
	{ txMsg = Msg
	{ msgData = typedMsgData rawTransactionData
	, msgAuthor = addressFromPubKey p signerPubKey
	, msgType = typedMsgType rawTransactionData
	}
	, txRoute = cs rawTransactionRoute
	, txGas = rawTransactionGas
	, txSignature = recSig
	, txSignBytes = signBytes
	, txSigner = signerPubKey
	, txNonce = rawTransactionNonce
	}

This would work for any static checks, but dynamic checks such as balance checks would still have to be performed in the keeper handler when actually running the transaction.

This creates a small issue for my use case though, which has messages (Utxo style transactions) where the expected message author(s) can't be determined statically, and involves a store lookup. For that case I can't really see a clean way to use this, other than extending validateMessage to be in a Sem. But I can just use a simple workaround where I don't use ValidateMessage for author checking, and just pass msgAuthor into the keeper handler and do those checks there.

[UnitylChaos]

This actually brings up another issue though, which is that parseTx requires recoverable signatures, and the upstream library has removed support for them in versions past 0.2.5 Relevant Commit

Signature verification via recovering the pubkey from the signature and then checking equality against something in the message feels pretty non-standard to me (and made it harder to see that signature verification was actually happening), so it might be worth changing this to the more usual pattern. Ie extract expected author key from message, and verify directly with signature and message bytes.

[martyall]

To keep from talking about a bunch of things at once, maybe we can first focus on what the merits of something like a static validation check are because it's not even clear to me and I wrote it 🙃

Assumptions:

1. The app developer is defining their transaction message types in protobuf files. This means that all messages are defined in terms of primitives like bytestring, int32, etc.
2. Applications benefit if transaction message types are built from higher level like Address or Balance.
3. Things like checking that a bytestring parses to an Address in every handler is too annoying to consider as an option.

There is a philosophy in the haskell world of "Parse, don't validate" (blog post. This makes me thing that actually the proper static validation class is something like

class ParseMessage a b where
  parseMessage :: Msg a -> V.Validation [MessageSemanticError] (Msg b)

And the point of this is to check things like "a bytestring parses as an Address", "a transfer amount is strictly greater than 0", "a string has a certain form", etc. This means that when we write keeper handlers (1) we don't have to remember to check this and (2) even if we do remember, we don't fill it with this kind of noise. They are not meant for checking stuff against a database to make sure that something is possible, like "the user has enough funds". That kind of stuff is definitely supposed to happen in the keeper handler in my opinion.

So before moving on to talk about other things, if you agree to this I feel like I should make an issue to change the typeclass to the above.

[UnitylChaos]

Having read the blog post (thanks for that), I agree that parsing into types which constrain the data possibilities is definitely preferable to just "validation".

As for the assumptions, I agree that at a base level the message types are proto or json encoded bytestrings, and that the app developer is expected to define them as Haskell data types. I'm a little confused on the third point though, as I thought that the usage of HasCodec (and the Either return type for decode) would already take care of checking for things like making sure that a value parses as an Address. And that you could take advantage of this to make a value >= 0 by using a Natural instead of an Int (for example).
So given that, I was thinking this kind of validation via parsing into a constrained type could be done by just specifying more constrained types in the Message type (ie Natural instead of Int, NonEmpty a instead of List a, etc).
I'm not clear on what exactly the ParseMessage class would be doing then, or where it would be doing it.