Documentation should be clearer about lack of support for IPv4-mapped IPv6 addresses in helper methods

[opara-tor]

Location (URL)

https://doc.rust-lang.org/std/net/enum.IpAddr.html
https://doc.rust-lang.org/std/net/struct.Ipv6Addr.html

Summary

There has been past discussion about whether Rust should support IPv4-mapped IPv6 addresses, and I don't want to raise that issue again. But the current behaviour has some footguns that I think should be documented better.

For example, I think it's reasonable to write a function such as:

/// Ensure that we don't make TCP connections to loopback addresses.
fn connect_if_not_loopback(s: SocketAddr) -> std::io::Result<Option<TcpStream>> {
    if s.ip().is_loopback() {
        return Ok(None);
    }

    Ok(Some(TcpStream::connect(s)?))
}

and it would be reasonable to expect that it would prevent connections to loopback addresses. And there isn't anything in IpAddr that would suggest otherwise.

But since Ipv6Addr::is_loopback() does not handle IPv4-mapped IPv6 addresses, this code is broken, and may lead to security issues in code where the author wasn't knowledgeable of IPv4-mapped IPv6 addresses.

For example:

let localhost = IpAddr::V6(Ipv4Addr::LOCALHOST.to_ipv6_mapped());
// We expect this to return `None`, but it connects to the loopback address and returns `Some`.
// This assertion panics.
assert!(connect_if_not_loopback(SocketAddr::new(localhost, 9000)).unwrap().is_none());

The correct code (AFAIK) should use s.ip().to_canonical().is_loopback(), but this isn't clear from the documentation. And this isn't an issue for just is_loopback(), but also other methods like is_unspecified() and the various nightly methods.

There is some top-level documentation in Ipv6Addr, but it doesn't mention to_canonical() at all and is easy to miss.

So I think it would be helpful (and remove a footgun) for IpAddr and Ipv6Addr if the helper is_foo() methods had a sentence explaining that they don't handle IPv4-mapped IPv6 addresses, and linked to a paragraph for how to handle them properly.

[bjorn3]

connect_if_not_loopback

Would would you want a denylist for specifically the loopback address? If you want to avoid connecting to local addresses, wouldn't an allow list using something like .is_global() make more sense? That also catches the ip ranges used by docker, devices on your local network and such. Looks like .is_global() is currently unstable though, but you can copy the implementation.

[opara-tor]

I agree, but I would imagine that the .is_loopback() check would be in addition to other checks, since .is_global() isn't usable on stable and people might roll their own equivalent rather than copy-paste code from a code base with different licenses/copyright. For example here's one that looks like it would have this bug.

But is_global() doesn't handle IPv4-mapped IPv6 addresses, so that gives some unexpected results too:

// both addresses are considered non-global
assert!(!IpAddr::V6(Ipv4Addr::LOCALHOST.to_ipv6_mapped()).is_global());
assert!(!IpAddr::V6(Ipv4Addr::new(8, 8, 8, 8).to_ipv6_mapped()).is_global());

Even in other contexts, it's common for code to be subtly broken. Here are some shortened examples I've found with a quick search in our code:

let endpoint = word
    .parse::<SocketAddr>()
    .map_err(|e| format!("failed to parse endpoint: {e}"))?;
if !endpoint.ip().is_loopback() {
    return Err(Cow::from(format!(
        "CMETHOD endpoint {endpoint} was not localhost"
    )));
}

fn from_str(s: &str) -> Result<Self, Self::Err> {
    let Some(addr_part) = s.strip_prefix("inet-auto:") else {
        return Err(ParseError::UnrecognizedFormat);
    };
    if addr_part == "auto" {
        return Ok(InetAutoAddress { bind: None });
    }
    let Ok(addr) = IpAddr::from_str(addr_part) else {
        return Err(ParseError::UnrecognizedFormat);
    };
    if addr.is_loopback() {
        Ok(InetAutoAddress { bind: Some(addr) })
    } else {
        Err(ParseError::AutoAddressNotLoopback)
    }
}

// Warn if outbound_proxy is configured to a non-loopback address
if let Some(ref proxy) = config.cfg.outbound_proxy {
    if !proxy.is_loopback() {
        warn!("outbound_proxy is configured to a non-loopback address");
    }
}

/// Helper for binding a listener: Picks the socketaddr to use
/// when told to bind to `addr`.
///
/// If addr is `0.0.0.0` or `[::]`, then we pick one of our own
/// addresses with the same family. Otherwise we fail unless `addr` is
/// one of our own addresses.
///
/// If port is 0, we pick a new arbitrary port we haven't used as
/// an arbitrary port before.
fn get_listener_addr(&self, spec: &SocketAddr) -> IoResult<SocketAddr> {
    let ipaddr = {
        let ip = spec.ip();
        if ip.is_unspecified() {
            self.get_addr_in_family(&ip)
                .ok_or_else(|| err(ErrorKind::AddrNotAvailable))?
        } else if self.inner.addrs.iter().any(|a| a == &ip) {
            ip
        } else {
            return Err(err(ErrorKind::AddrNotAvailable));
        }
    };
    [...]
}

These aren't security issues, but they're likely bugs since they don't behave as intended for IPv4-mapped IPv6 addresses. My guess is that the authors of these code snippets weren't considering IPv4-mapped IPv6 addresses when they wrote the code. I think mentioning this edge case more prominently in the rust documentation would help avoid these types of footguns.

[bjorn3]

All four snippets seem fine to me. An ipv6 mapped ipv4 loopback would just give an error/warning. With the exception of the first (which maybe gets the ip from the network) ,they probably just expect the user to either specify an ipv4 loopback or ipv6 loopback in their config. Why would a user specify an ipv6 mapped ipv4 loopback in a config file?

For example here's one that looks like it would have this bug.

Looks like it.

I'm fine with adding docs for this.

[opara-tor]

All four snippets seem fine to me. An ipv6 mapped ipv4 loopback would just give an error/warning.

Right, but I don't think this was the intended behaviour. Or at least it would have been nice if we handled that edge case more explicitly.

Why would a user specify an ipv6 mapped ipv4 loopback in a config file?

Many users of our software copy-paste configuration files from the Internet without reading/understanding them. It's not uncommon for people to use weird configuration options to work around safety/security checks. So we try to be a bit defensive when handling configuration options, to make sure adequate warnings are given. And it would be nice to be more confident that we're handling the edge cases properly.