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

@@ -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

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

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

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"

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()