krisprice / ipnet

IpNet, Ipv4Net, and Ipv6Net types and methods for Rust
Apache License 2.0
122 stars 26 forks source link

extended version of `std::net::parser` does not include CVE fix #32

Open peterthejohnston opened 3 years ago

peterthejohnston commented 3 years ago

It looks like the copied and extended version of the std::net::parser module in the ipnet parser doesn't include this recent CVE fix to the standard library that disallows the use of octal format in IPv4 strings: https://github.com/rust-lang/rust/pull/83652.

From the PR to rust-lang/rust:

In its original specification, leading zero in Ipv4 string is interpreted as octal literals. So a IP address 0127.0.0.1 actually means 87.0.0.1.

This confusion can lead to many security vulnerabilities. Therefore, in IETF RFC 6943, it suggests to disallow octal/hexadecimal format in Ipv4 string all together.

If I understand correctly, similarly to std::net::parser, it's not that a leading zero would cause the string to be interpreted as an octal literal in ipnet's parser, as the parser specifies the radix as 10 here; however, it would be good to fully disallow leading-zero octal format in an IPv4 string as suggested in the above RFC, since it's invalid in the strict format.

Would it make sense to apply that change to ipnet? I'm happy to put together a PR.

peterthejohnston commented 3 years ago

Oops, I wrote a test and it looks like the ipnet parser already doesn't allow octal or hex format.

krisprice commented 3 years ago

Sorry I missed the original notification about this last week. Thanks for spotting this and working on it. I’m also a bit surprised it doesn’t have the same problem since it was copied from the std lib (but it was long ago) :)

peterthejohnston commented 3 years ago

Yeah, that's interesting. I have a theory:

That might mean that ipnet is still "exposed" to the CVE in the case of an octet with 3 or fewer digits and a leading zero (e.g. 01.2.3.4).

krisprice commented 3 years ago

Ah, so it would seem. Looks like the parser in std has had quite a few other changes too. Is this something you'd still like to submit a PR for? I'm happy to merge a fix. There's no rush.