Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create a CanonicalAddr serialization and key type #2364

Closed
9 tasks
Tracked by #2311
teor2345 opened this issue Jun 22, 2021 · 1 comment
Closed
9 tasks
Tracked by #2311

Create a CanonicalAddr serialization and key type #2364

teor2345 opened this issue Jun 22, 2021 · 1 comment
Labels
A-network Area: Network protocol updates or fixes A-rust Area: Updates to Rust code C-security Category: Security issues I-invalid-data Zebra relies on invalid or untrusted data, or sends invalid data I-remote-node-overload Zebra can overload other nodes on the network

Comments

@teor2345
Copy link
Contributor

teor2345 commented Jun 22, 2021

Motivation

In PR #2276, we make sure that all SocketAddrs are canonical in the MetaAddr and MetaAddrChange constructors.

Otherwise, some SocketAddrs could compare unequal, but actually connect to the same peer. This breaks Zebra's network security overall connection limits (PeerSet / AddressBook) and new connection rate-limits (CandidateSet / AddressBook).

But non-canonical addresses could accidentally be added in new code. So we should enforce canonical addresses using a CanonicalAddr type.

IPv4 in IPv6 Specifications

Bitcoin

Each encapsulated network IP address currently uses the following structure:
IP address: IPv6 address in big endian byte order. IPv4 addresses can be provided as IPv4-mapped IPv6 addresses

https://developer.bitcoin.org/reference/p2p_networking.html#addr

Rust API

But the Rust IPv6Addr::to_ipv4 method supports IPv4-compatible and IPv4-mapped IPv6 addresses:

Converts this address to an IPv4 address. Returns None if this address is neither IPv4-compatible or IPv4-mapped.

https://doc.rust-lang.org/std/net/struct.Ipv6Addr.html#method.to_ipv4

IPv4-mapped IPv6 Addresses

             +----------------------+---------------------+
             | Attribute            | Value               |
             +----------------------+---------------------+
             | Address Block        | ::ffff:0:0/96       |
             | Name                 | IPv4-mapped Address |
             | RFC                  | [RFC4291]           |

https://datatracker.ietf.org/doc/html/rfc6890#page-15

IPv4-compatible IPv6 Addresses

The "IPv4-Compatible IPv6 address" was defined to assist in the IPv6
transition.  The format of the "IPv4-Compatible IPv6 address" is as
follows:

|                80 bits               | 16 |      32 bits        |
+--------------------------------------+--------------------------+
|0000..............................0000|0000|    IPv4 address     |
+--------------------------------------+----+---------------------+

https://datatracker.ietf.org/doc/html/rfc4291#section-2.5.5.1

Rust IPv6 APIs

IPv6 socket addresses consist of an IPv6 address, a 16-bit port number, as well as fields containing the traffic class, the flow label, and a scope identifier (see IETF RFC 2553, Section 3.3 for more details).

https://doc.rust-lang.org/std/net/struct.SocketAddrV6.html

PartialEq for SocketAddrV6 also checks the flowinfo and scope_id fields:

impl PartialEq for SocketAddrV6 {
    fn eq(&self, other: &SocketAddrV6) -> bool {
        self.inner.sin6_port == other.inner.sin6_port
            && self.inner.sin6_addr.s6_addr == other.inner.sin6_addr.s6_addr
            && self.inner.sin6_flowinfo == other.inner.sin6_flowinfo
            && self.inner.sin6_scope_id == other.inner.sin6_scope_id
    }
}

https://doc.rust-lang.org/src/std/net/addr.rs.html#700-707

https://doc.rust-lang.org/std/net/struct.SocketAddrV6.html#method.scope_id
https://doc.rust-lang.org/std/net/struct.SocketAddrV6.html#method.flowinfo

Solution

  • Add a CanonicalAddr type to zebra_chain::serialization::socket_addr
    • Give it ip: IpAddr and port: u16 fields
    • Add From<SocketAddr> and FromStr methods
    • Use the same accessor method names as SocketAddr
  • Replace SocketAddr with CanonicalAddr in zebra-network:
    • Message
    • Request / Response
    • MetaAddr / AddressBook
  • impl Arbitrary for CanonicalAddr, then remove the canonical_socket_addr strategy attributes

Alternatives

Currently Zebra converts SocketAddrs into their canonical form in a few different places. It would be easy to forget to add this conversion, or forget to test it.

We could just store an IPv6 address internally, and map IPv4-mapped IPv6 addresses to an IPv4 address in the ip method. (And in the Debug impl.)

We could just work in IPv6 addresses, but this makes debugging harder, and might impact compatibility with older C libraries with poor IPv6 support.

@teor2345 teor2345 added A-rust Area: Updates to Rust code S-needs-triage Status: A bug report needs triage P-Medium C-security Category: Security issues I-remote-node-overload Zebra can overload other nodes on the network I-invalid-data Zebra relies on invalid or untrusted data, or sends invalid data A-network Area: Network protocol updates or fixes labels Jun 22, 2021
@mpguerra mpguerra removed the S-needs-triage Status: A bug report needs triage label Jun 25, 2021
@mpguerra mpguerra mentioned this issue Jan 27, 2022
40 tasks
@teor2345
Copy link
Contributor Author

teor2345 commented Mar 1, 2022

This would be a useful validation improvement, but it's not needed any time soon.

@teor2345 teor2345 closed this as completed Mar 1, 2022
@mpguerra mpguerra closed this as not planned Won't fix, can't repro, duplicate, stale May 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-network Area: Network protocol updates or fixes A-rust Area: Updates to Rust code C-security Category: Security issues I-invalid-data Zebra relies on invalid or untrusted data, or sends invalid data I-remote-node-overload Zebra can overload other nodes on the network
Projects
None yet
Development

No branches or pull requests

2 participants