From aa92ad4f42d772c53ce4a2d9c76cf760b4450031 Mon Sep 17 00:00:00 2001 From: Kim De Mey <7857583+kdeme@users.noreply.github.com> Date: Mon, 2 Dec 2024 15:38:00 +0700 Subject: [PATCH 1/2] discv4: Fix assert on invalid RLP list in Neighbours message (#763) The assert would occur when the rlp size of the a node in the nodes rlp list is incorrectly set too high and then the next `listElem` call for the next node will start from the incorrect data. When that data is not a list the assert in `listElem` will be triggered. Fixed by adding a `listLen` call which checks if it is a list. Added also more strictness by: - Checking if that list is of len 4, which it must be - raising immediatly on invalid IP length - raising immediatly on invalid public key / node id + test cases --- eth/p2p/discovery.nim | 14 +++++++------- tests/p2p/test_discovery.nim | 20 ++++++++++++++++++++ 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/eth/p2p/discovery.nim b/eth/p2p/discovery.nim index aa0e722..7725f32 100644 --- a/eth/p2p/discovery.nim +++ b/eth/p2p/discovery.nim @@ -220,6 +220,9 @@ proc recvNeighbours( var neighbours = newSeqOfCap[Node](16) for i in 0 ..< sz: let n = neighboursList.listElem(i) + if n.listLen() != 4: + raise newException(RlpError, "Invalid nodes list") + let ipBlob = n.listElem(0).toBytes var ip: IpAddress case ipBlob.len @@ -228,17 +231,14 @@ proc recvNeighbours( of 16: ip = IpAddress(family: IpAddressFamily.IPv6, address_v6: toArray(16, ipBlob)) else: - error "Wrong ip address length!" - continue + raise newException(RlpError, "Invalid RLP byte string length for IP address") let udpPort = n.listElem(1).toInt(uint16).Port let tcpPort = n.listElem(2).toInt(uint16).Port - let pk = PublicKey.fromRaw(n.listElem(3).toBytes) - if pk.isErr: - warn "Could not parse public key" - continue + let pk = PublicKey.fromRaw(n.listElem(3).toBytes).valueOr: + raise newException(RlpError, "Invalid RLP byte string for node id") - neighbours.add(newNode(pk[], Address(ip: ip, udpPort: udpPort, tcpPort: tcpPort))) + neighbours.add(newNode(pk, Address(ip: ip, udpPort: udpPort, tcpPort: tcpPort))) d.kademlia.recvNeighbours(node, neighbours) proc recvFindNode( diff --git a/tests/p2p/test_discovery.nim b/tests/p2p/test_discovery.nim index c9c5460..14b89ce 100644 --- a/tests/p2p/test_discovery.nim +++ b/tests/p2p/test_discovery.nim @@ -13,6 +13,7 @@ import std/sequtils, chronos, stew/byteutils, nimcrypto/keccak, testutils/unittests, ../../eth/common/keys, ../../eth/p2p/[discovery, kademlia, enode], + ../../eth/rlp, ../stubloglevel proc localAddress(port: int): Address = @@ -108,6 +109,21 @@ procSuite "Discovery Tests": # We currently do not raise on this, so can't really test it # "0x03f847b841AA0000000000000000000000000000000000000000000000000000000000000000a99a96bd988e1839272f93257bd9dfb2e558390e1f9bff28cdc8f04c9b5d06b1846fd3aed7", ] + + let invalidRlpData = [ + # valid version: "0x04f8a5f89ef84d847f000001827661827669b840ebefe173cab8832f6d6623f2a4d415959c1b80071e6af7d5986417d4bd07df19318b30d3d4fde75b025314ce22a523a714bef6c61838094e5d3640dccc6b9e34f84d847f000001827662827662b840ebefe173cab8832f6d6623f2a4d415959c1b80071e6af7d5986417d4bd07df19318b30d3d4fde75b025314ce22a523a714bef6c61838094e5d3640dccc6b9e348479e7f252", + # Invalid rlp item length (for the individual node list), causing a next listElem to read wrong data. + "0x04f8a5f89ef856847f000001827661827669b840ebefe173cab8832f6d6623f2a4d415959c1b80071e6af7d5986417d4bd07df19318b30d3d4fde75b025314ce22a523a714bef6c61838094e5d3640dccc6b9e34f84d847f000001827662827662b8403114ae2fe09b7a71d6693451c0d043df8315fb811e43c7b97ef2ff87409a2601b4425f92ffff80008417d66489cb1f8414d9dd4393ba16ebb455aa47345a222b8479e7f252", + # Invalid IP length in nodes + "0x04f8a4f89df84c837f0001827661827669b840ebefe173cab8832f6d6623f2a4d415959c1b80071e6af7d5986417d4bd07df19318b30d3d4fde75b025314ce22a523a714bef6c61838094e5d3640dccc6b9e34f84d847f000001827662827662b840ebefe173cab8832f6d6623f2a4d415959c1b80071e6af7d5986417d4bd07df19318b30d3d4fde75b025314ce22a523a714bef6c61838094e5d3640dccc6b9e348479e7f252", + # Invalid Public key in nodes + "0x04f8a5f89ef84d847f000001827661827669b840fbefe173cab8832f6d6623f2a4d415959c1b80071e6af7d5986417d4bd07df19318b30d3d4fde75b025314ce22a523a714bef6c61838094e5d3640dccc6b9e34f84d847f000001827662827662b840ebefe173cab8832f6d6623f2a4d415959c1b80071e6af7d5986417d4bd07df19318b30d3d4fde75b025314ce22a523a714bef6c61838094e5d3640dccc6b9e348479e7f252", + # Invalid Public key in nodes - too short + "0x04f8a4f89df84c847f000001827661827669b83fefe173cab8832f6d6623f2a4d415959c1b80071e6af7d5986417d4bd07df19318b30d3d4fde75b025314ce22a523a714bef6c61838094e5d3640dccc6b9e34f84d847f000001827662827662b840ebefe173cab8832f6d6623f2a4d415959c1b80071e6af7d5986417d4bd07df19318b30d3d4fde75b025314ce22a523a714bef6c61838094e5d3640dccc6b9e348479e7f252", + # nodes list len of 3 (removed port) + "0x04f8a2f89bf84a847f000001827661b840ebefe173cab8832f6d6623f2a4d415959c1b80071e6af7d5986417d4bd07df19318b30d3d4fde75b025314ce22a523a714bef6c61838094e5d3640dccc6b9e34f84d847f000001827662827662b840ebefe173cab8832f6d6623f2a4d415959c1b80071e6af7d5986417d4bd07df19318b30d3d4fde75b025314ce22a523a714bef6c61838094e5d3640dccc6b9e348479e7f252" + ] + let address = localAddress(20302) nodeKey = PrivateKey.fromHex( @@ -117,6 +133,10 @@ procSuite "Discovery Tests": expect DiscProtocolError: bootNode.receive(address, packData(hexToSeqByte(data), nodeKey)) + for data in invalidRlpData: + expect RlpError: + bootNode.receive(address, packData(hexToSeqByte(data), nodeKey)) + # empty msg id and payload, doesn't raise, just fails and prints wrong # msg mac bootNode.receive(address, packData(@[], nodeKey)) From dcfbc4291d39b59563828c3e32be4d51a2f25931 Mon Sep 17 00:00:00 2001 From: Jacek Sieka Date: Wed, 4 Dec 2024 12:40:44 +0100 Subject: [PATCH 2/2] update version to 0.5.0 (#764) 1.0.0 was never an intentional release and the nim-eth repo is not yet stable in terms of API, thus we start at 0.5.0 and go from there for the first tagged release. Most of the eth code in this repo now is aligned with various specs meaning that most API can be considered "mostly" stable, but there are still aspects being worked on as well as a potential future reorganisation of the code turning nim-eth into a more "pure" spec-driven core ethereum infrastructure repo, removing in the process application-level stuff that happens to be common between nimbus-eth1 and eth2 and ended up in here for convenience. --- eth.nimble | 4 ++-- eth/common/base_rlp.nim | 4 ++-- eth/p2p/discovery.nim | 4 ++-- eth/p2p/discoveryv5/encoding.nim | 14 +++++++------- eth/p2p/discoveryv5/node.nim | 2 +- eth/p2p/discoveryv5/sessions.nim | 4 ++-- eth/p2p/kademlia.nim | 2 +- tests/fuzzing/discovery/generate.nim | 2 +- 8 files changed, 18 insertions(+), 18 deletions(-) diff --git a/eth.nimble b/eth.nimble index 449bfdf..cc44b87 100644 --- a/eth.nimble +++ b/eth.nimble @@ -1,6 +1,6 @@ mode = ScriptMode.Verbose -version = "1.0.0" +version = "0.5.0" author = "Status Research & Development GmbH" description = "Ethereum Common library" license = "MIT" @@ -8,7 +8,7 @@ skipDirs = @["tests"] requires "nim >= 1.6.0", "nimcrypto", - "stint", + "stint >= 0.8.0", "secp256k1", "chronos", "chronicles", diff --git a/eth/common/base_rlp.nim b/eth/common/base_rlp.nim index 4765cfa..58ae0c2 100644 --- a/eth/common/base_rlp.nim +++ b/eth/common/base_rlp.nim @@ -8,7 +8,7 @@ {.push raises: [].} import - std/typetraits, ./base, ../rlp, + std/typetraits, ./base, ../rlp, ../rlp/results as rlp_results export base, rlp, rlp_results @@ -49,7 +49,7 @@ func significantBytesBE(val: openArray[byte]): int = proc append*(w: var RlpWriter, value: StUint) = if value > 128: - let bytes = value.toByteArrayBE + let bytes = value.toBytesBE let nonZeroBytes = significantBytesBE(bytes) w.append bytes.toOpenArray(bytes.len - nonZeroBytes, bytes.len - 1) else: diff --git a/eth/p2p/discovery.nim b/eth/p2p/discovery.nim index 7725f32..638a552 100644 --- a/eth/p2p/discovery.nim +++ b/eth/p2p/discovery.nim @@ -143,7 +143,7 @@ proc sendPong*(d: DiscoveryProtocol, n: Node, token: MDigest[256]) = proc sendFindNode*(d: DiscoveryProtocol, n: Node, targetNodeId: NodeId) = var data: array[64, byte] - data[32 .. ^1] = targetNodeId.toByteArrayBE() + data[32 .. ^1] = targetNodeId.toBytesBE() let payload = rlp.encode((data, expiration())) let msg = pack(cmdFindNode, payload, d.privKey) trace ">>> find_node to ", n #, ": ", msg.toHex() @@ -249,7 +249,7 @@ proc recvFindNode( let rng = rlp.listElem(0).toBytes # Check for pubkey len if rng.len == 64: - let nodeId = readUintBE[256](rng[32 .. ^1]) + let nodeId = UInt256.fromBytesBE(rng.toOpenArray(32, 63)) d.kademlia.recvFindNode(node, nodeId) else: trace "Invalid target public key received" diff --git a/eth/p2p/discoveryv5/encoding.nim b/eth/p2p/discoveryv5/encoding.nim index f187633..ef6f2a3 100644 --- a/eth/p2p/discoveryv5/encoding.nim +++ b/eth/p2p/discoveryv5/encoding.nim @@ -141,7 +141,7 @@ proc idHash(challengeData, ephkey: openArray[byte], nodeId: NodeId): ctx.update(idSignatureText) ctx.update(challengeData) ctx.update(ephkey) - ctx.update(nodeId.toByteArrayBE()) + ctx.update(nodeId.toBytesBE()) result = ctx.finish() ctx.clear() @@ -160,8 +160,8 @@ proc deriveKeys*(n1, n2: NodeId, priv: PrivateKey, pub: PublicKey, var info = newSeqOfCap[byte](keyAgreementPrefix.len + 32 * 2) for i, c in keyAgreementPrefix: info.add(byte(c)) - info.add(n1.toByteArrayBE()) - info.add(n2.toByteArrayBE()) + info.add(n1.toBytesBE()) + info.add(n2.toBytesBE()) var secrets: HandshakeSecrets static: assert(sizeof(secrets) == aesKeySize * 2) @@ -200,7 +200,7 @@ proc decryptGCM*(key: AesKey, nonce, ct, authData: openArray[byte]): proc encryptHeader*(id: NodeId, iv, header: openArray[byte]): seq[byte] = var ectx: CTR[aes128] - ectx.init(id.toByteArrayBE().toOpenArray(0, 15), iv) + ectx.init(id.toBytesBE().toOpenArray(0, 15), iv) result = newSeq[byte](header.len) ectx.encrypt(header, result) ectx.clear() @@ -226,7 +226,7 @@ proc encodeMessagePacket*(rng: var HmacDrbgContext, c: var Codec, # static-header let - authdata = c.localNode.id.toByteArrayBE() + authdata = c.localNode.id.toBytesBE() staticHeader = encodeStaticHeader(Flag.OrdinaryMessage, nonce, authdata.len()) @@ -313,7 +313,7 @@ proc encodeHandshakePacket*(rng: var HmacDrbgContext, c: var Codec, var authdata: seq[byte] var authdataHead: seq[byte] - authdataHead.add(c.localNode.id.toByteArrayBE()) + authdataHead.add(c.localNode.id.toBytesBE()) authdataHead.add(64'u8) # sig-size: 64 authdataHead.add(33'u8) # eph-key-size: 33 authdata.add(authdataHead) @@ -359,7 +359,7 @@ proc decodeHeader*(id: NodeId, iv, maskedHeader: openArray[byte]): # No need to check staticHeader size as that is included in minimum packet # size check in decodePacket var ectx: CTR[aes128] - ectx.init(id.toByteArrayBE().toOpenArray(0, aesKeySize - 1), iv) + ectx.init(id.toBytesBE().toOpenArray(0, aesKeySize - 1), iv) # Decrypt static-header part of the header var staticHeader = newSeq[byte](staticHeaderSize) ectx.decrypt(maskedHeader.toOpenArray(0, staticHeaderSize - 1), staticHeader) diff --git a/eth/p2p/discoveryv5/node.nim b/eth/p2p/discoveryv5/node.nim index c1667ce..6d6bbbf 100644 --- a/eth/p2p/discoveryv5/node.nim +++ b/eth/p2p/discoveryv5/node.nim @@ -80,7 +80,7 @@ func `==`*(a, b: Node): bool = (not a.isNil and not b.isNil and a.pubkey == b.pubkey) func hash*(id: NodeId): Hash = - hash(id.toByteArrayBE) + hash(id.toBytesBE) proc random*(T: type NodeId, rng: var HmacDrbgContext): T = rng.generate(T) diff --git a/eth/p2p/discoveryv5/sessions.nim b/eth/p2p/discoveryv5/sessions.nim index ae4385c..3eeaad0 100644 --- a/eth/p2p/discoveryv5/sessions.nim +++ b/eth/p2p/discoveryv5/sessions.nim @@ -32,7 +32,7 @@ type func makeKey(id: NodeId, address: Address): SessionKey = var pos = 0 - result[pos ..< pos+sizeof(id)] = toBytes(id) + result[pos ..< pos+sizeof(id)] = toBytesBE(id) pos.inc(sizeof(id)) case address.ip.family of IpAddressFamily.IpV4: @@ -40,7 +40,7 @@ func makeKey(id: NodeId, address: Address): SessionKey = of IpAddressFamily.IpV6: result[pos ..< pos+sizeof(address.ip.address_v6)] = address.ip.address_v6 pos.inc(sizeof(address.ip.address_v6)) - result[pos ..< pos+sizeof(address.port)] = toBytes(address.port.uint16) + result[pos ..< pos+sizeof(address.port)] = toBytesBE(address.port.uint16) func store*(s: var Sessions, id: NodeId, address: Address, r, w: AesKey) = var value: array[sizeof(r) + sizeof(w), byte] diff --git a/eth/p2p/kademlia.nim b/eth/p2p/kademlia.nim index e3d49d7..3cd60cf 100644 --- a/eth/p2p/kademlia.nim +++ b/eth/p2p/kademlia.nim @@ -102,7 +102,7 @@ proc `==`*(a, b: Node): bool = (a.isNil and b.isNil) or (not a.isNil and not b.isNil and a.node.pubkey == b.node.pubkey) proc timeKey(id: NodeId, ip: IpAddress, cmd: CommandId): TimeKey = - result[0..31] = id.toByteArrayBE()[0..31] + result[0..31] = id.toBytesBE()[0..31] case ip.family of IpAddressFamily.IPv6: result[32..47] = ip.address_v6[0..15] diff --git a/tests/fuzzing/discovery/generate.nim b/tests/fuzzing/discovery/generate.nim index a77d6f5..0d3fd54 100644 --- a/tests/fuzzing/discovery/generate.nim +++ b/tests/fuzzing/discovery/generate.nim @@ -38,7 +38,7 @@ proc generate() = # valid data for a FindNode packet block: var data: array[64, byte] - data[32 .. ^1] = peerKey.toPublicKey().toNodeId().toByteArrayBE() + data[32 .. ^1] = peerKey.toPublicKey().toNodeId().toBytesBE() let payload = rlp.encode((data, expiration())) let encodedData = @[3.byte] & @payload debug "FindNode", data=byteutils.toHex(encodedData)