Fix possible crashes due to RangeErrors in rlpx auth decoding (#558)

- Fix two possible RangeErrors, due to negative seq allocation
in decodeAuthMessageEIP8 and in decodeAckMessageEIP8
- Set the minimum auth message size to AuthMessageEIP8Length,
in case there are clients that no longer add padding
- Add tests for invalid length cases

eth/p2p/auth.nim

@@ -1,6 +1,6 @@
 #
 #                 Ethereum P2P
-#              (c) Copyright 2018
+#              (c) Copyright 2018-2022
 #       Status Research & Development GmbH
 #
 #            Licensed under either of
@@ -8,7 +8,7 @@
 #            MIT license (LICENSE-MIT)
 #
 
-## This module implements Ethereum authentication
+## This module implements Ethereum RLPx authentication
 
 {.push raises: [Defect].}
 
@@ -22,18 +22,36 @@ export results
 
 const
   SupportedRlpxVersion* = 4'u8
+
+  # Auth message sizes
+
+  # Pre EIP8
   PlainAuthMessageV4Length* = 194
   AuthMessageV4Length* = 307
+
+  # EIP8
+  # signature + pubkey + nounce + version + rlp encoding overhead
+  # 65 + 64 + 32 + 1 + 7 = 169
   PlainAuthMessageEIP8Length = 169
-  PlainAuthMessageMaxEIP8* = PlainAuthMessageEIP8Length + 255
+  PlainAuthMessageMaxEIP8* = PlainAuthMessageEIP8Length + 255 # with padding
-  AuthMessageEIP8Length* = 282 + 2
+  # Min. encrypted message + size prefix = 284
-  AuthMessageMaxEIP8* = AuthMessageEIP8Length + 255
+  AuthMessageEIP8Length* = PlainAuthMessageEIP8Length + eciesOverheadLength + 2
+  AuthMessageMaxEIP8* = AuthMessageEIP8Length + 255 # with padding
+
+  # Ack message sizes
+
+  # Pre EIP8
   PlainAckMessageV4Length* = 97
   AckMessageV4Length* = 210
+
+  # EIP 8
+  # pubkey + nounce + version + rlp encoding overhead
+  # 64 + 32 + 1 + 5 = 102
   PlainAckMessageEIP8Length* = 102
-  PlainAckMessageMaxEIP8* = PlainAckMessageEIP8Length + 255
+  PlainAckMessageMaxEIP8* = PlainAckMessageEIP8Length + 255 # with padding
-  AckMessageEIP8Length* = 215 + 2
+  # Min. encrypted message + size prefix = 217
-  AckMessageMaxEIP8* = AckMessageEIP8Length + 255
+  AckMessageEIP8Length* = PlainAckMessageEIP8Length + eciesOverheadLength + 2
+  AckMessageMaxEIP8* = AckMessageEIP8Length + 255 # with padding
 
 type
   Nonce* = array[KeyLength, byte]
@@ -361,12 +379,18 @@ proc decodeAuthMessageV4(h: var Handshake, m: openArray[byte]): AuthResult[void]
 proc decodeAuthMessageEIP8(h: var Handshake, m: openArray[byte]): AuthResult[void] =
   ## Decodes EIP-8 AuthMessage.
   let size = uint16.fromBytesBE(m)
-  h.expectedLength = int(size) + 2
+  h.expectedLength = 2 + int(size)
+
+  # Check if the prefixed size is => than the minimum
+  if h.expectedLength < AuthMessageEIP8Length:
+    return err(AuthError.IncompleteError)
+
   if h.expectedLength > len(m):
     return err(AuthError.IncompleteError)
+
   var buffer = newSeq[byte](eciesDecryptedLength(int(size)))
-  if eciesDecrypt(toa(m, 2, int(size)), buffer, h.host.seckey,
+  if eciesDecrypt(
-                  toa(m, 0, 2)).isErr:
+      toa(m, 2, int(size)), buffer, h.host.seckey, toa(m, 0, 2)).isErr:
     return err(AuthError.EciesError)
   try:
     var reader = rlpFromBytes(buffer)
@@ -408,13 +432,18 @@ proc decodeAuthMessageEIP8(h: var Handshake, m: openArray[byte]): AuthResult[voi
 proc decodeAckMessageEIP8*(h: var Handshake, m: openArray[byte]): AuthResult[void] =
   ## Decodes EIP-8 AckMessage.
   let size = uint16.fromBytesBE(m)
-
   h.expectedLength = 2 + int(size)
+
+  # Check if the prefixed size is => than the minimum
+  if h.expectedLength < AckMessageEIP8Length:
+    return err(AuthError.IncompleteError)
+
   if h.expectedLength > len(m):
     return err(AuthError.IncompleteError)
+
   var buffer = newSeq[byte](eciesDecryptedLength(int(size)))
-  if eciesDecrypt(toa(m, 2, int(size)), buffer, h.host.seckey,
+  if eciesDecrypt(
-                  toa(m, 0, 2)).isErr:
+      toa(m, 2, int(size)), buffer, h.host.seckey, toa(m, 0, 2)).isErr:
     return err(AuthError.EciesError)
   try:
     var reader = rlpFromBytes(buffer)
@@ -456,7 +485,8 @@ proc decodeAckMessageV4(h: var Handshake, m: openArray[byte]): AuthResult[void]
 
 proc decodeAuthMessage*(h: var Handshake, input: openArray[byte]): AuthResult[void] =
   ## Decodes AuthMessage from `input`.
-  if len(input) < AuthMessageV4Length:
+  # Using the smallest min. message length of the two types
+  if len(input) < AuthMessageEIP8Length:
     return err(AuthError.IncompleteError)
 
   if len(input) == AuthMessageV4Length:
@@ -470,8 +500,10 @@ proc decodeAuthMessage*(h: var Handshake, input: openArray[byte]): AuthResult[vo
 
 proc decodeAckMessage*(h: var Handshake, input: openArray[byte]): AuthResult[void] =
   ## Decodes AckMessage from `input`.
+  # Using the smallest min. message length of the two types
   if len(input) < AckMessageV4Length:
     return err(AuthError.IncompleteError)
+
   if len(input) == AckMessageV4Length:
     let res = h.decodeAckMessageV4(input)
     if res.isOk(): return res

eth/p2p/ecies.nim

@@ -1,6 +1,6 @@
 #
 #                 Ethereum P2P
-#              (c) Copyright 2018
+#              (c) Copyright 2018-2022
 #       Status Research & Development GmbH
 #
 #            Licensed under either of
@@ -22,6 +22,11 @@ export results
 const
   emptyMac* = array[0, byte]([])
 
+  eciesOverheadLength* =
+    # Data overhead size for ECIES encrypted message
+    # pubkey + IV + MAC = 65 + 16 + 32 = 113
+    1 + sizeof(PublicKey) + aes128.sizeBlock + sha256.sizeDigest
+
 type
   EciesError* = enum
     BufferOverrun   = "ecies: output buffer size is too small"
@@ -42,17 +47,13 @@ type
 proc mapErrTo[T](r: SkResult[T], v: static EciesError): EciesResult[T] =
   r.mapErr(proc (e: cstring): EciesError = v)
 
-template eciesOverheadLength*(): int =
-  ## Return data overhead size for ECIES encrypted message
-  1 + sizeof(PublicKey) + aes128.sizeBlock + sha256.sizeDigest
-
 template eciesEncryptedLength*(size: int): int =
   ## Return size of encrypted message for message with size `size`.
-  size + eciesOverheadLength()
+  size + eciesOverheadLength
 
 template eciesDecryptedLength*(size: int): int =
   ## Return size of decrypted message for encrypted message with size `size`.
-  size - eciesOverheadLength()
+  size - eciesOverheadLength
 
 template eciesMacLength(size: int): int =
   ## Return size of authenticated data
@@ -175,9 +176,9 @@ proc eciesDecrypt*(input: openArray[byte],
   var header = cast[ptr EciesHeader](unsafeAddr input[0])
   if header.version != 0x04:
     return err(WrongHeader)
-  if len(input) <= eciesOverheadLength():
+  if len(input) <= eciesOverheadLength:
     return err(IncompleteError)
-  if len(input) - eciesOverheadLength() > len(output):
+  if len(input) - eciesOverheadLength > len(output):
     return err(BufferOverrun)
 
   var
@@ -192,7 +193,7 @@ proc eciesDecrypt*(input: openArray[byte],
     sha256.digest(material.toOpenArray(KeyLength div 2, material.high))
   burnMem(material)
 
-  let macsize = eciesMacLength(len(input) - eciesOverheadLength())
+  let macsize = eciesMacLength(len(input) - eciesOverheadLength)
   ctx.init(macKey.data)
   burnMem(macKey)
   ctx.update(toOpenArray(input, eciesIvPos(), eciesIvPos() + macsize - 1))

tests/p2p/test_auth.nim

@@ -466,3 +466,81 @@ suite "Ethereum P2P handshake test suite":
         check:
           csecInitiator.aesKey == csecResponder.aesKey
           csecInitiator.macKey == csecResponder.macKey
+
+    test "Invalid AuthMessage - Minimum input size":
+      var responder = newTestHandshake({Responder})
+
+      # 1 byte short on minimum AuthMessage size
+      var m = newSeq[byte](AuthMessageEIP8Length - 1)
+
+      let res = responder.decodeAuthMessage(m)
+      check:
+        res.isErr()
+        res.error == AuthError.IncompleteError
+
+    test "Invalid AuthMessage - Minimum size prefix":
+      var responder = newTestHandshake({Responder})
+
+      # Minimum size for EIP8 AuthMessage
+      var m = newSeq[byte](AuthMessageEIP8Length)
+      # size prefix size of 281, 1 byte short
+      m[0] = 1
+      m[1] = 25
+
+      let res = responder.decodeAuthMessage(m)
+      check:
+        res.isErr()
+        res.error == AuthError.IncompleteError
+
+    test "Invalid AuthMessage - Size prefix bigger than input":
+      var responder = newTestHandshake({Responder})
+
+      # Minimum size for EIP8 AuthMessage
+      var m = newSeq[byte](AuthMessageEIP8Length)
+      # size prefix size of 283, 1 byte too many
+      m[0] = 1
+      m[1] = 27
+
+      let res = responder.decodeAuthMessage(m)
+      check:
+        res.isErr()
+        res.error == AuthError.IncompleteError
+
+    test "Invalid AckMessage - Minimum input size":
+      var initiator = newTestHandshake({Initiator, EIP8})
+
+      # 1 byte short on minimum size
+      let m = newSeq[byte](AckMessageV4Length - 1)
+
+      let res = initiator.decodeAckMessage(m)
+      check:
+        res.isErr()
+        res.error == AuthError.IncompleteError
+
+    test "Invalid AckMessage - Minimum size prefix":
+      var initiator = newTestHandshake({Initiator, EIP8})
+
+      # Minimum size for EIP8 AckMessage
+      var m = newSeq[byte](AckMessageEIP8Length)
+      # size prefix size of 214, 1 byte short
+      m[0] = 0
+      m[1] = 214
+
+      let res = initiator.decodeAckMessage(m)
+      check:
+        res.isErr()
+        res.error == AuthError.IncompleteError
+
+    test "Invalid AckMessage - Size prefix bigger than input":
+      var initiator = newTestHandshake({Initiator, EIP8})
+
+      # Minimum size for EIP8 AckMessage
+      var m = newSeq[byte](AckMessageEIP8Length)
+      # size prefix size of 216, 1 byte too many
+      m[0] = 0
+      m[1] = 216
+
+      let res = initiator.decodeAckMessage(m)
+      check:
+        res.isErr()
+        res.error == AuthError.IncompleteError