Merge pull request #194 from status-im/decrypt-failure

Discv5: Specifically handle decryption errors

eth/p2p/discoveryv5/discovery_db.nim

@@ -50,3 +50,10 @@ method loadKeys*(db: DiscoveryDB, id: NodeId, address: Address, r, w: var AesKey
   except CatchableError:
     return false
 
+method deleteKeys*(db: DiscoveryDB, id: NodeId, address: Address):
+    bool {.raises: [Defect].} =
+  try:
+    db.backend.del(makeKey(id, address))
+    return true
+  except CatchableError:
+    return false

eth/p2p/discoveryv5/encoding.nim

@@ -1,5 +1,5 @@
 import
-  std/tables, nimcrypto, stint, chronicles,
+  std/[tables, options], nimcrypto, stint, chronicles,
   types, node, enr, hkdf, ../enode, eth/[rlp, keys]
 
 const
@@ -7,7 +7,7 @@ const
   keyAgreementPrefix = "discovery v5 key agreement"
   authSchemeName* = "gcm"
   gcmNonceSize* = 12
-  gcmTagSize = 16
+  gcmTagSize* = 16
   tagSize* = 32 ## size of the tag where each message (except whoareyou) starts
   ## with
 
@@ -43,7 +43,8 @@ type
   DecodeStatus* = enum
     Success,
     HandshakeError,
-    PacketError
+    PacketError,
+    DecryptError
 
 proc randomBytes*(v: var openarray[byte]) =
   if nimcrypto.randomBytes(v) != v.len:
@@ -159,17 +160,25 @@ proc encodeEncrypted*(c: Codec,
   headBuf.add(encryptGCM(writeKey, nonce, body, tag))
   return (headBuf, nonce)
 
-proc decryptGCM(key: AesKey, nonce, ct, authData: openarray[byte]): seq[byte] =
+proc decryptGCM*(key: AesKey, nonce, ct, authData: openarray[byte]):
+    Option[seq[byte]] =
+  if ct.len <= gcmTagSize:
+    debug "cipher is missing tag", len = ct.len
+    return
+
   var dctx: GCM[aes128]
   dctx.init(key, nonce, authData)
-  result = newSeq[byte](ct.len - gcmTagSize)
+  var res = newSeq[byte](ct.len - gcmTagSize)
   var tag: array[gcmTagSize, byte]
-  dctx.decrypt(ct.toOpenArray(0, ct.high - gcmTagSize), result)
+  dctx.decrypt(ct.toOpenArray(0, ct.high - gcmTagSize), res)
   dctx.getTag(tag)
-  if tag != ct.toOpenArray(ct.len - gcmTagSize, ct.high):
-    result = @[]
   dctx.clear()
 
+  if tag != ct.toOpenArray(ct.len - gcmTagSize, ct.high):
+    return
+
+  return some(res)
+
 type
   DecodePacketResult = enum
     decodingSuccessful
@@ -222,7 +231,15 @@ proc decodeAuthResp(c: Codec, fromId: NodeId, head: AuthHeader,
 
   var zeroNonce: array[gcmNonceSize, byte]
   let respData = decryptGCM(secrets.authRespKey, zeroNonce, head.response, [])
-  let authResp = rlp.decode(respData, AuthResponse)
+  if respData.isNone():
+    return false
+
+  let authResp = rlp.decode(respData.get(), AuthResponse)
+  # TODO:
+  # 1. Should allow for not having an ENR included, solved for now by sending
+  # whoareyou with always recordSeq of 0
+  # 2. Should verify ENR and check for correct id in case an ENR is included
+  # 3. Should verify id nonce signature
 
   newNode = newNode(authResp.record)
   return true
@@ -275,7 +292,7 @@ proc decodeEncrypted*(c: var Codec,
     var writeKey: AesKey
     if not c.db.loadKeys(fromId, fromAddr, readKey, writeKey):
       trace "Decoding failed (no keys)"
-      return PacketError
+      return DecryptError
       # doAssert(false, "TODO: HANDLE ME!")
 
   let headSize = tagSize + r.position
@@ -283,8 +300,14 @@ proc decodeEncrypted*(c: var Codec,
 
   let body = decryptGCM(readKey, auth.auth, bodyEnc.toOpenArray,
     input[0 .. tagSize - 1].toOpenArray)
-  if body.len > 1:
-    let status = decodePacketBody(body[0], body.toOpenArray(1, body.high), packet)
+  if body.isNone():
+    discard c.db.deleteKeys(fromId, fromAddr)
+    return DecryptError
+
+  let packetData = body.get()
+  if packetData.len > 1:
+    let status = decodePacketBody(packetData[0],
+      packetData.toOpenArray(1, packetData.high), packet)
     if status == decodingSuccessful:
       return Success
     else:

eth/p2p/discoveryv5/protocol.nim

@@ -93,7 +93,7 @@ proc decodeWhoAreYou(d: Protocol, msg: Bytes): Whoareyou =
 
 proc sendWhoareyou(d: Protocol, address: Address, toNode: NodeId, authTag: AuthTag) =
   trace "sending who are you", to = $toNode, toAddress = $address
-  let challenge = Whoareyou(authTag: authTag, recordSeq: 1)
+  let challenge = Whoareyou(authTag: authTag, recordSeq: 0)
   encoding.randomBytes(challenge.idNonce)
   # If there is already a handshake going on for this nodeid then we drop this
   # new one. Handshake will get cleaned up after `handshakeTimeout`.
@@ -211,11 +211,17 @@ proc receive*(d: Protocol, a: Address, msg: Bytes) {.gcsafe,
           waiter.complete(packet.some)
         else:
           debug "TODO: handle packet: ", packet = packet.kind, origin = $node
-    elif decoded == DecodeStatus.PacketError:
-      debug "Could not decode packet, respond with whoareyou",
+    elif decoded == DecodeStatus.DecryptError:
+      debug "Could not decrypt packet, respond with whoareyou",
         localNode = $d.localNode, address = a
+      # only sendingWhoareyou in case it is a decryption failure
       d.sendWhoareyou(a, sender, authTag)
-    # No Whoareyou in case it is a Handshake Failure
+    elif decoded == DecodeStatus.PacketError:
+      # Still adding the node in case there is a packet error (could be
+      # unsupported packet)
+      if not node.isNil:
+        debug "Adding new node to routing table", node = $node, localNode = $d.localNode
+        discard d.routingTable.addNode(node)
 
 proc waitPacket(d: Protocol, fromNode: Node, reqId: RequestId): Future[Option[Packet]] =
   result = newFuture[Option[Packet]]("waitPacket")
@@ -356,6 +362,7 @@ proc revalidateNode(p: Protocol, n: Node)
       discard
 
     p.routingTable.setJustSeen(n)
+    trace "Revalidated node", node = $n
   else:
     if false: # TODO: if not bootnode:
       p.routingTable.removeNode(n)
@@ -386,7 +393,8 @@ proc lookupLoop(d: Protocol) {.async.} =
     trace "lookupLoop canceled"
 
 proc open*(d: Protocol) =
-  debug "Starting discovery node", node = $d.localNode
+  debug "Starting discovery node", node = $d.localNode,
+    uri = toURI(d.localNode.record)
   # TODO allow binding to specific IP / IPv6 / etc
   let ta = initTAddress(IPv4_any(), d.localNode.node.address.udpPort)
   d.transp = newDatagramTransport(processClient, udata = d, local = ta)

eth/p2p/discoveryv5/types.nim

@@ -87,6 +87,9 @@ method storeKeys*(db: Database, id: NodeId, address: Address, r, w: AesKey):
 method loadKeys*(db: Database, id: NodeId, address: Address, r, w: var AesKey):
     bool {.base, raises: [Defect].} = discard
 
+method deleteKeys*(db: Database, id: NodeId, address: Address):
+    bool {.raises: [Defect].} = discard
+
 proc toBytes*(id: NodeId): array[32, byte] {.inline.} =
   id.toByteArrayBE()
 

tests/p2p/test_discv5_encoding.nim

@@ -1,5 +1,5 @@
 import
-  unittest, stew/byteutils, stint,
+  unittest, options, sequtils, stew/byteutils, stint,
   eth/[rlp, keys] , eth/p2p/discoveryv5/[types, encoding, enr]
 
 # According to test vectors:
@@ -141,16 +141,16 @@ suite "Discovery v5 Cryptographic Primitives":
       eph.data == hexToSeqByte(sharedSecret)
 
   test "Key Derivation":
-    const
-      # input
-      secretKey = "0x02a77e3aa0c144ae7c0a3af73692b7d6e5b7a2fdc0eda16e8d5e6cb0d08e88dd04"
-      nodeIdA = "0xa448f24c6d18e575453db13171562b71999873db5b286df957af199ec94617f7"
-      nodeIdB = "0x885bba8dfeddd49855459df852ad5b63d13a3fae593f3f9fa7e317fd43651409"
-      idNonce = "0x0101010101010101010101010101010101010101010101010101010101010101"
-      # expected output
-      initiatorKey = "0x238d8b50e4363cf603a48c6cc3542967"
-      recipientKey = "0xbebc0183484f7e7ca2ac32e3d72c8891"
-      authRespKey = "0xe987ad9e414d5b4f9bfe4ff1e52f2fae"
+    # const
+    #   # input
+    #   secretKey = "0x02a77e3aa0c144ae7c0a3af73692b7d6e5b7a2fdc0eda16e8d5e6cb0d08e88dd04"
+    #   nodeIdA = "0xa448f24c6d18e575453db13171562b71999873db5b286df957af199ec94617f7"
+    #   nodeIdB = "0x885bba8dfeddd49855459df852ad5b63d13a3fae593f3f9fa7e317fd43651409"
+    #   idNonce = "0x0101010101010101010101010101010101010101010101010101010101010101"
+    #   # expected output
+    #   initiatorKey = "0x238d8b50e4363cf603a48c6cc3542967"
+    #   recipientKey = "0xbebc0183484f7e7ca2ac32e3d72c8891"
+    #   authRespKey = "0xe987ad9e414d5b4f9bfe4ff1e52f2fae"
 
     # Code doesn't allow to start from shared `secretKey`, but only from the
     # public and private key. Would require pulling `ecdhAgree` out of
@@ -194,3 +194,38 @@ suite "Discovery v5 Cryptographic Primitives":
     # The encryption of the auth-resp-pt uses one of these keys, as does the
     # encryption of the message itself. So the whole test depends on this.
     skip()
+
+suite "Discovery v5 Additional":
+  test "Encryption/Decryption":
+    let
+      encryptionKey = hexToByteArray[aesKeySize]("0x9f2d77db7004bf8a1a85107ac686990b")
+      nonce = hexToByteArray[authTagSize]("0x27b5af763c446acd2749fe8e")
+      ad = hexToByteArray[tagSize]("0x93a7400fa0d6a694ebc24d5cf570f65d04215b6ac00757875e3f3a5f42107903")
+      pt = hexToSeqByte("0xa1")
+
+    let ct = encryptGCM(encryptionKey, nonce, pt, ad)
+    let decrypted = decryptGCM(encryptionKey, nonce, ct, ad)
+
+    check decrypted.get() == pt
+
+  test "Decryption":
+    let
+      encryptionKey = hexToByteArray[aesKeySize]("0x9f2d77db7004bf8a1a85107ac686990b")
+      nonce = hexToByteArray[authTagSize]("0x27b5af763c446acd2749fe8e")
+      ad = hexToByteArray[tagSize]("0x93a7400fa0d6a694ebc24d5cf570f65d04215b6ac00757875e3f3a5f42107903")
+      pt = hexToSeqByte("0x01c20101")
+      ct = hexToSeqByte("0xa5d12a2d94b8ccb3ba55558229867dc13bfa3648")
+
+    # valid case
+    check decryptGCM(encryptionKey, nonce, ct, ad).get() == pt
+
+    # invalid tag/data sizes
+    var invalidCipher: seq[byte] = @[]
+    check decryptGCM(encryptionKey, nonce, invalidCipher, ad).isNone()
+
+    invalidCipher = repeat(byte(4), gcmTagSize)
+    check decryptGCM(encryptionKey, nonce, invalidCipher, ad).isNone()
+
+    # invalid tag/data itself
+    invalidCipher = repeat(byte(4), gcmTagSize + 1)
+    check decryptGCM(encryptionKey, nonce, invalidCipher, ad).isNone()