From 522db295f2b85b9bc676a08eb0731246e3c56cb6 Mon Sep 17 00:00:00 2001 From: Kim De Mey Date: Mon, 14 Nov 2022 15:49:37 +0100 Subject: [PATCH] Fix RLP deserialzation for Enum with holes (#554) RLP Enum deserialization would currently not check if "hole values" were attempted to be converted to the enum type. Now use checkedEnumAssign and fail with RlpTypeMismatch on invalid values. There is at least one occurance of an enum with holes in rlpx p2p: DisconnectionReason. For this enum the issue could occur. Also: - Added enum RLP tests and rlpx p2p disconnect message tests to test the DisconnectionReason with enum hole value. - Fixed worse custom DisconnectionReason decoding occurance in rlpx in waitSingleMsg proc where this issue could occur. --- eth/p2p/private/p2p_types.nim | 28 ++++++++++++++---------- eth/p2p/rlpx.nim | 13 +++++------ eth/rlp.nim | 11 +++++++--- tests/p2p/test_rlpx_thunk.json | 22 ++++++++++++++----- tests/rlp/test_api_usage.nim | 40 ++++++++++++++++++++++++++++++---- 5 files changed, 81 insertions(+), 33 deletions(-) diff --git a/eth/p2p/private/p2p_types.nim b/eth/p2p/private/p2p_types.nim index 488fdfe..8dde77c 100644 --- a/eth/p2p/private/p2p_types.nim +++ b/eth/p2p/private/p2p_types.nim @@ -167,19 +167,23 @@ type Disconnecting, Disconnected + # Disconnect message reasons as specified: + # https://github.com/ethereum/devp2p/blob/master/rlpx.md#disconnect-0x01 + # Receiving values that are too large or that are in the enum hole will + # trigger `RlpTypeMismatch` error on deserialization. DisconnectionReason* = enum - DisconnectRequested, - TcpError, - BreachOfProtocol, - UselessPeer, - TooManyPeers, - AlreadyConnected, - IncompatibleProtocolVersion, - NullNodeIdentityReceived, - ClientQuitting, - UnexpectedIdentity, - SelfConnection, - MessageTimeout, + DisconnectRequested = 0x00, + TcpError = 0x01, + BreachOfProtocol = 0x02, + UselessPeer = 0x03, + TooManyPeers = 0x04, + AlreadyConnected = 0x05, + IncompatibleProtocolVersion = 0x06, + NullNodeIdentityReceived = 0x07, + ClientQuitting = 0x08, + UnexpectedIdentity = 0x09, + SelfConnection = 0x0A, + MessageTimeout = 0x0B, SubprotocolReason = 0x10 proc `$`*(peer: Peer): string = $peer.remote diff --git a/eth/p2p/rlpx.nim b/eth/p2p/rlpx.nim index 2b2e05f..c1c0cea 100644 --- a/eth/p2p/rlpx.nim +++ b/eth/p2p/rlpx.nim @@ -708,14 +708,11 @@ proc waitSingleMsg(peer: Peer, MsgType: type): Future[MsgType] {.async.} = "Invalid RLPx message body") elif nextMsgId == 1: # p2p.disconnect - if nextMsgData.isList(): - let reason = DisconnectionReason nextMsgData.listElem(0).toInt(uint32) - await peer.disconnect(reason) - trace "disconnect message received in waitSingleMsg", reason, peer - raisePeerDisconnected("Unexpected disconnect", reason) - else: - raise newException(RlpTypeMismatch, - "List expected, but the source RLP is not a list.") + let reasonList = rlp.read(nextMsgData, DisconnectionReasonList) + let reason = reasonList.value + await peer.disconnect(reason) + trace "disconnect message received in waitSingleMsg", reason, peer + raisePeerDisconnected("Unexpected disconnect", reason) else: warn "Dropped RLPX message", msg = peer.dispatcher.messages[nextMsgId].name diff --git a/eth/rlp.nim b/eth/rlp.nim index 4c9abfc..e43d5b0 100644 --- a/eth/rlp.nim +++ b/eth/rlp.nim @@ -8,6 +8,8 @@ import ./rlp/[writer, object_serialization], ./rlp/priv/defs +from stew/objects import checkedEnumAssign + export writer, object_serialization @@ -293,12 +295,15 @@ proc readImpl(rlp: var Rlp, T: type Integer): Integer = proc readImpl(rlp: var Rlp, T: type[enum]): T = let value = rlp.toInt(int) - if value < ord(T.low) or value > ord(T.high): + + var res: T + if not checkedEnumAssign(res, value): raise newException(RlpTypeMismatch, - "Enum expected, but the source RLP is not in valid range.") - result = type(result)(value) + "Enum value expected, but the source RLP is not in valid range.") rlp.skipElem + res + proc readImpl(rlp: var Rlp, T: type bool): T = result = rlp.toInt(int) != 0 rlp.skipElem diff --git a/tests/p2p/test_rlpx_thunk.json b/tests/p2p/test_rlpx_thunk.json index 5c90ad6..0bad2ae 100644 --- a/tests/p2p/test_rlpx_thunk.json +++ b/tests/p2p/test_rlpx_thunk.json @@ -59,15 +59,25 @@ "error": "RlpTypeMismatch", "description": "listElem to assert on not a single entry list" }, - "Listing single element list with entry off enum range": { - "payload": "01c116", + "DisconnectReason: single element list with entry out off enum range": { + "payload": "01c111", "error": "RlpTypeMismatch", - "description": "Disconnect reason code out of bounds 0..16 (got: 22)" + "description": "Disconnect reason code out of bounds 0..16 (got: 17)" }, - "Listing single element off enum range": { - "payload": "0116", + "DisconnectReason: single element out off enum range": { + "payload": "0111", "error": "RlpTypeMismatch", - "description": "Disconnect reason code out of bounds 0..16 (got: 22)" + "description": "Disconnect reason code out of bounds 0..16 (got: 17)" + }, + "DisconnectReason: single element list with enum hole value": { + "payload": "01c10C", + "error": "RlpTypeMismatch", + "description": "Error on Disconnect reason with enum hole value" + }, + "DisconnectReason: single element with enum hole value": { + "payload": "010C", + "error": "RlpTypeMismatch", + "description": "Error on Disconnect reason with enum hole value" }, "devp2p hello packet version 22 + additional list elements for EIP-8": { "payload": "00f87137916b6e6574682f76302e39312f706c616e39cdc5836574683dc6846d6f726b1682270fb840fda1cff674c90c9a197539fe3dfb53086ace64f83ed7c6eabec741f7f381cc803e52ab2cd55d5569bce4347107a310dfd5f88a010cd2ffd1005ca406f1842877c883666f6f836261720304" diff --git a/tests/rlp/test_api_usage.nim b/tests/rlp/test_api_usage.nim index 57132fe..39cf063 100644 --- a/tests/rlp/test_api_usage.nim +++ b/tests/rlp/test_api_usage.nim @@ -203,16 +203,48 @@ suite "test api usage": test "invalid enum": type MyEnum = enum - foo, - bar + foo = 0x00, + bar = 0x01 var writer = initRlpWriter() - writer.append(2) - writer.append(-1) + writer.append(1) # valid + writer.append(2) # invalid + writer.append(-1) # invalid let bytes = writer.finish() var rlp = rlpFromBytes(bytes) + + check rlp.read(MyEnum) == bar + expect RlpTypeMismatch: discard rlp.read(MyEnum) rlp.skipElem() + expect RlpTypeMismatch: discard rlp.read(MyEnum) + + test "invalid enum - enum with hole": + type + MyEnum = enum + foo = 0x00, + bar = 0x01, + baz = 0x0100 + + var writer = initRlpWriter() + writer.append(1) # valid + writer.append(2) # invalid - enum hole value + writer.append(256) # valid + writer.append(257) # invalid - too large + let bytes = writer.finish() + var rlp = rlpFromBytes(bytes) + + check rlp.read(MyEnum) == bar + + expect RlpTypeMismatch: + discard rlp.read(MyEnum) + rlp.skipElem() + + check rlp.read(MyEnum) == baz + + expect RlpTypeMismatch: + discard rlp.read(MyEnum) + rlp.skipElem()