ipnet icon indicating copy to clipboard operation
ipnet copied to clipboard

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

Open peterthejohnston opened this issue 3 years ago • 4 comments

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 avatar Aug 26 '21 13:08 peterthejohnston

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

peterthejohnston avatar Sep 01 '21 23:09 peterthejohnston

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) :)

krisprice avatar Sep 02 '21 00:09 krisprice

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

  • in the ipnet copy, when reading an octet in an IPv4 address, max_digits is specified as 3, which would have correctly triggered the error case in the test I wrote ("0127.0.0.1")
  • whereas in std, max_digits is unspecified, so that particular case (four digits with a leading zero) was parsed incorrectly without a manual check until the fix

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).

peterthejohnston avatar Sep 02 '21 13:09 peterthejohnston

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.

krisprice avatar Sep 08 '21 00:09 krisprice