From d2a3727c4c3fd4218a42104410a8c6a1e29a6819 Mon Sep 17 00:00:00 2001 From: Kim De Mey Date: Tue, 21 Feb 2023 09:34:26 +0100 Subject: [PATCH] Use chronos isGlobal to verify public IPs for net/nat and discv5 (#588) --- eth/net/nat.nim | 10 ++- eth/net/utils.nim | 16 ++-- eth/p2p/discoveryv5/nodes_verification.nim | 70 ++++++++++-------- tests/p2p/test_discoveryv5.nim | 85 +++++++++++++++++++--- 4 files changed, 125 insertions(+), 56 deletions(-) diff --git a/eth/net/nat.nim b/eth/net/nat.nim index ead32ee..0ffe4a4 100644 --- a/eth/net/nat.nim +++ b/eth/net/nat.nim @@ -115,20 +115,22 @@ proc getExternalIP*(natStrategy: NatStrategy, quiet = false): Option[IpAddress] # Further more, we check if the bind address (user provided, or a "0.0.0.0" # default) is a public IP. That's a long shot, because code paths involving a # user-provided bind address are not supposed to get here. -proc getRoutePrefSrc(bindIp: ValidIpAddress): (Option[ValidIpAddress], PrefSrcStatus) = +proc getRoutePrefSrc( + bindIp: ValidIpAddress): (Option[ValidIpAddress], PrefSrcStatus) = let bindAddress = initTAddress(bindIp, Port(0)) if bindAddress.isAnyLocal(): let ip = getRouteIpv4() if ip.isErr(): # No route was found, log error and continue without IP. - error "No routable IP address found, check your network connection", error = ip.error + error "No routable IP address found, check your network connection", + error = ip.error return (none(ValidIpAddress), NoRoutingInfo) - elif ip.get().isPublic(): + elif ip.get().isGlobalUnicast(): return (some(ip.get()), PrefSrcIsPublic) else: return (none(ValidIpAddress), PrefSrcIsPrivate) - elif bindAddress.isPublic(): + elif bindAddress.isGlobalUnicast(): return (some(ValidIpAddress.init(bindIp)), BindAddressIsPublic) else: return (none(ValidIpAddress), BindAddressIsPrivate) diff --git a/eth/net/utils.nim b/eth/net/utils.nim index 8769d01..6957e89 100644 --- a/eth/net/utils.nim +++ b/eth/net/utils.nim @@ -36,19 +36,15 @@ func dec*(ipLimits: var IpLimits, ip: ValidIpAddress) = elif val > 1: ipLimits.ips[ip] = val - 1 -func isPublic*(address: TransportAddress): bool = - # TODO: Some are still missing, for special reserved addresses see: - # https://www.iana.org/assignments/iana-ipv4-special-registry/iana-ipv4-special-registry.xhtml - # https://www.iana.org/assignments/iana-ipv6-special-registry/iana-ipv6-special-registry.xhtml - if address.isLoopback() or address.isSiteLocal() or - address.isMulticast() or address.isLinkLocal(): - false - else: +func isGlobalUnicast*(address: TransportAddress): bool = + if address.isGlobal() and address.isUnicast(): true + else: + false -func isPublic*(address: IpAddress): bool = +func isGlobalUnicast*(address: IpAddress): bool = let a = initTAddress(address, Port(0)) - a.isPublic + a.isGlobalUnicast() proc getRouteIpv4*(): Result[ValidIpAddress, cstring] = # Avoiding Exception with initTAddress and can't make it work with static. diff --git a/eth/p2p/discoveryv5/nodes_verification.nim b/eth/p2p/discoveryv5/nodes_verification.nim index 82984eb..6512cd1 100644 --- a/eth/p2p/discoveryv5/nodes_verification.nim +++ b/eth/p2p/discoveryv5/nodes_verification.nim @@ -1,35 +1,44 @@ +# Copyright (c) 2022-2023 Status Research & Development GmbH +# Licensed under either of +# * Apache License, version 2.0, ([LICENSE-APACHE](LICENSE-APACHE)) +# * MIT license ([LICENSE-MIT](LICENSE-MIT)) +# at your option. +# This file may not be copied, modified, or distributed except according to +# those terms. + {.push raises: [Defect].} import std/[sets, options], stew/results, stew/shims/net, chronicles, chronos, + ../../net/utils, "."/[node, enr, routing_table] logScope: topics = "nodes-verification" -proc validIp(sender, address: IpAddress): bool = - let - s = initTAddress(sender, Port(0)) - a = initTAddress(address, Port(0)) - if a.isAnyLocal(): - return false - if a.isMulticast(): - return false - if a.isLoopback() and not s.isLoopback(): - return false - if a.isSiteLocal() and not s.isSiteLocal(): - return false - # TODO: Also check for special reserved ip addresses: - # https://www.iana.org/assignments/iana-ipv4-special-registry/iana-ipv4-special-registry.xhtml - # https://www.iana.org/assignments/iana-ipv6-special-registry/iana-ipv6-special-registry.xhtml - return true +func validIp(sender, address: IpAddress): bool = + let a = initTAddress(address, Port(0)) + if a.isGlobalUnicast(): + true + else: + let s = initTAddress(sender, Port(0)) + if a.isLoopback() and s.isLoopback(): + true + elif a.isSiteLocal() and s.isSiteLocal(): + true + else: + false -proc verifyNodesRecords(enrs: openArray[Record], fromNode: Node, nodesLimit: int, +proc verifyNodesRecords( + enrs: openArray[Record], src: Node, nodesLimit: int, distances: Option[seq[uint16]]): seq[Node] = ## Verify and convert ENRs to a sequence of nodes. Only ENRs that pass ## verification will be added. ENRs are verified for duplicates, invalid ## addresses and invalid distances if those are specified. + logScope: + sender = src.record.toURI + var seen: HashSet[Node] var count = 0 for r in enrs: @@ -42,8 +51,7 @@ proc verifyNodesRecords(enrs: openArray[Record], fromNode: Node, nodesLimit: int # as in original Kademlia. Because of this it is chosen not to fail # immediately, but still process maximum `findNodeResultLimit`. if count >= nodesLimit: - debug "Too many ENRs", enrs = enrs.len(), - limit = nodesLimit, sender = fromNode.record.toURI + debug "Too many ENRs", enrs = enrs.len(), limit = nodesLimit break count.inc() @@ -54,23 +62,20 @@ proc verifyNodesRecords(enrs: openArray[Record], fromNode: Node, nodesLimit: int # Check for duplicates in the nodes reply. Duplicates are checked based # on node id. if n in seen: - trace "Duplicate node ids", - record = n.record.toURI, id = n.id, sender = fromNode.record.toURI + trace "Duplicate node ids", record = n.record.toURI, id = n.id continue # Check if the node has an address and if the address is public or from # the same local network or lo network as the sender. The latter allows # for local testing. if not n.address.isSome() or not - validIp(fromNode.address.get().ip, n.address.get().ip): - trace "Invalid ip-address", - record = n.record.toURI, node = n, sender = fromNode.record.toURI + validIp(src.address.get().ip, n.address.get().ip): + trace "Invalid ip-address", record = n.record.toURI, node = n continue # Check if returned node has one of the requested distances. if distances.isSome(): # TODO: This is incorrect for custom distances - if (not distances.get().contains(logDistance(n.id, fromNode.id))): - debug "Incorrect distance", - record = n.record.toURI, sender = fromNode.record.toURI + if (not distances.get().contains(logDistance(n.id, src.id))): + debug "Incorrect distance", record = n.record.toURI continue # No check on UDP port and thus any port is allowed, also the so called @@ -79,8 +84,11 @@ proc verifyNodesRecords(enrs: openArray[Record], fromNode: Node, nodesLimit: int seen.incl(n) result.add(n) -proc verifyNodesRecords*(enrs: openArray[Record], fromNode: Node, nodesLimit: int): seq[Node] = - verifyNodesRecords(enrs, fromNode, nodesLimit, none[seq[uint16]]()) +proc verifyNodesRecords*( + enrs: openArray[Record], src: Node, nodesLimit: int): seq[Node] = + verifyNodesRecords(enrs, src, nodesLimit, none[seq[uint16]]()) -proc verifyNodesRecords*(enrs: openArray[Record], fromNode: Node, nodesLimit: int, distances: seq[uint16]): seq[Node] = - verifyNodesRecords(enrs, fromNode, nodesLimit, some[seq[uint16]](distances)) +proc verifyNodesRecords*( + enrs: openArray[Record], src: Node, nodesLimit: int, + distances: seq[uint16]): seq[Node] = + verifyNodesRecords(enrs, src, nodesLimit, some[seq[uint16]](distances)) diff --git a/tests/p2p/test_discoveryv5.nim b/tests/p2p/test_discoveryv5.nim index 1057982..cce700a 100644 --- a/tests/p2p/test_discoveryv5.nim +++ b/tests/p2p/test_discoveryv5.nim @@ -507,15 +507,15 @@ suite "Discovery v5 Tests": await mainNode.closeWait() await testNode.closeWait() - test "Verify records of nodes message": + test "Verify records of Nodes message - global src": let port = Port(9000) - fromNoderecord = enr.Record.init(1, PrivateKey.random(rng[]), + srcRecord = enr.Record.init(1, PrivateKey.random(rng[]), some(ValidIpAddress.init("11.12.13.14")), some(port), some(port))[] - fromNode = newNode(fromNoderecord)[] + srcNode = newNode(srcRecord)[] pk = PrivateKey.random(rng[]) - targetDistance = @[logDistance(fromNode.id, pk.toPublicKey().toNodeId())] + targetDistance = @[logDistance(srcNode.id, pk.toPublicKey().toNodeId())] limit = 16 block: # Duplicates @@ -526,7 +526,7 @@ suite "Discovery v5 Tests": # Exact duplicates var records = @[record, record] - var nodes = verifyNodesRecords(records, fromNode, limit, targetDistance) + var nodes = verifyNodesRecords(records, srcNode, limit, targetDistance) check nodes.len == 1 # Node id duplicates @@ -534,7 +534,7 @@ suite "Discovery v5 Tests": 1, pk, some(ValidIpAddress.init("212.13.14.15")), some(port), some(port))[] records.add(recordSameId) - nodes = verifyNodesRecords(records, fromNode, limit, targetDistance) + nodes = verifyNodesRecords(records, srcNode, limit, targetDistance) check nodes.len == 1 block: # No address @@ -542,7 +542,16 @@ suite "Discovery v5 Tests": recordNoAddress = enr.Record.init( 1, pk, none(ValidIpAddress), some(port), some(port))[] records = [recordNoAddress] - test = verifyNodesRecords(records, fromNode, limit, targetDistance) + test = verifyNodesRecords(records, srcNode, limit, targetDistance) + check test.len == 0 + + block: # Invalid address - any local + let + recordInvalidAddress = enr.Record.init( + 1, pk, some(ValidIpAddress.init("0.0.0.0")), + some(port), some(port))[] + records = [recordInvalidAddress] + test = verifyNodesRecords(records, srcNode, limit, targetDistance) check test.len == 0 block: # Invalid address - site local @@ -551,7 +560,7 @@ suite "Discovery v5 Tests": 1, pk, some(ValidIpAddress.init("10.1.2.3")), some(port), some(port))[] records = [recordInvalidAddress] - test = verifyNodesRecords(records, fromNode, limit, targetDistance) + test = verifyNodesRecords(records, srcNode, limit, targetDistance) check test.len == 0 block: # Invalid address - loopback @@ -560,7 +569,7 @@ suite "Discovery v5 Tests": 1, pk, some(ValidIpAddress.init("127.0.0.1")), some(port), some(port))[] records = [recordInvalidAddress] - test = verifyNodesRecords(records, fromNode, limit, targetDistance) + test = verifyNodesRecords(records, srcNode, limit, targetDistance) check test.len == 0 block: # Invalid distance @@ -569,7 +578,7 @@ suite "Discovery v5 Tests": 1, pk, some(ValidIpAddress.init("12.13.14.15")), some(port), some(port))[] records = [recordInvalidDistance] - test = verifyNodesRecords(records, fromNode, limit, @[0'u16]) + test = verifyNodesRecords(records, srcNode, limit, @[0'u16]) check test.len == 0 block: # Invalid distance but distance validation is disabled @@ -578,7 +587,61 @@ suite "Discovery v5 Tests": 1, pk, some(ValidIpAddress.init("12.13.14.15")), some(port), some(port))[] records = [recordInvalidDistance] - test = verifyNodesRecords(records, fromNode, limit) + test = verifyNodesRecords(records, srcNode, limit) + check test.len == 1 + + test "Verify records of Nodes message - loopback src": + let + port = Port(9000) + srcRecord = enr.Record.init(1, PrivateKey.random(rng[]), + some(ValidIpAddress.init("127.0.0.0")), + some(port), some(port))[] + srcNode = newNode(srcRecord)[] + pk = PrivateKey.random(rng[]) + targetDistance = @[logDistance(srcNode.id, pk.toPublicKey().toNodeId())] + limit = 16 + + block: # valid address - lo with lo src + let + record = enr.Record.init( + 1, pk, some(ValidIpAddress.init("127.0.0.1")), + some(port), some(port))[] + test = verifyNodesRecords([record], srcNode, limit, targetDistance) + check test.len == 1 + + block: # valid address - global with lo src + let + record = enr.Record.init( + 1, pk, some(ValidIpAddress.init("1.2.3.4")), + some(port), some(port))[] + test = verifyNodesRecords([record], srcNode, limit, targetDistance) + check test.len == 1 + + test "Verify records of Nodes message - site local src": + let + port = Port(9000) + srcRecord = enr.Record.init(1, PrivateKey.random(rng[]), + some(ValidIpAddress.init("192.168.1.1")), + some(port), some(port))[] + srcNode = newNode(srcRecord)[] + pk = PrivateKey.random(rng[]) + targetDistance = @[logDistance(srcNode.id, pk.toPublicKey().toNodeId())] + limit = 16 + + block: # valid address - site local with site local src + let + record = enr.Record.init( + 1, pk, some(ValidIpAddress.init("192.168.1.2")), + some(port), some(port))[] + test = verifyNodesRecords([record], srcNode, limit, targetDistance) + check test.len == 1 + + block: # valid address - global with site local src + let + record = enr.Record.init( + 1, pk, some(ValidIpAddress.init("1.2.3.4")), + some(port), some(port))[] + test = verifyNodesRecords([record], srcNode, limit, targetDistance) check test.len == 1 test "Calculate lookup distances":