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
net: reject leading zeros in IP address parsers [freeze exception] #30999
Comments
|
This is more of a net/url issue rather than net/http. What does the RFC say regarding this ? |
|
I believe that |
|
As shown in this recent article, this behavior could be used in server-side request forgery, local file inclusion and remote file inclusion vulnerabilities. |
|
There is no real RFC on textual IP address representation. The best we have is https://tools.ietf.org/html/draft-main-ipaddr-text-rep-02 which says
I'd argue Go net.ParseIP/ParseCIDR etc should return an error on zero-prefixed input. It avoids ambiguity and since Go has historically parsed them differently than BSD, an error is a safer change in behavior than silently giving different results. See also https://man7.org/linux/man-pages/man3/inet_pton.3.html which does not accept zero-prefixed IPs. |
|
And as I discussed with @secenv on IRC, that article is naive. Typical "attacks" that 0127.0.0.1 enables are enabled also by |
|
I forgot to add that it is indeed an issue that affects net.ParseCIDR https://play.golang.org/p/HpWqhr9tZ53 . I agree with @tv42, those functions should return errors. The documentation should at least warn the developer. Guess I should mention @FiloSottile for further discussion on the security impact of this issue. |
|
I found a more authoritative RFC on IP address textual representation -- although it's only Informational not Standards Track: https://tools.ietf.org/html/rfc6943#section-3.1.1 Since Go doesn't use the "loose" syntax of RFC6943, it's non-conforming already. Rejecting non-dotted-decimal inputs would make Go use the "strict" syntax. |
|
I agree about changing Go's IP address parsers (1) the RFCs are mostly quiet but in a few places hint that decimal is the right interpretation, It seems like an open question whether this should be done |
I find this pretty convincing, especially given that To end up vulnerable due to this mismatch, an application would have to parse the IP with Go, reject any hostnames, apply security-relevant logic to the return value, and then pass the input (not the encoding of the return value) to a different, non-Go application which is happy to parse the IP as octal. Generally, this is another instance where relying on parser alignment instead of re-encoding outputs is a fragile design. We are not aware of any application for which this leads to a security issue, if anyone does please let us know at security@golang.org as that would help evaluate whether to backport the fix. In any case, I definitely agree we should just consider these inputs invalid in Go 1.17. |
|
Related: #43389 ("net: limit the size of ParseIP input?") |
|
This proposal has been added to the active column of the proposals project |
|
We will not be backporting this issue. We are treating the change as a robustness improvement and not a security fix due to its potential for breaking working use cases. The situation is not nearly so clear cut as the advocates of CVE-2021-29923 would have people believe. They present it as a bug, plain and simple, not to treat leading zeros in IP addresses as indicating octal numbers, but that's not obvious. The BSD TCP/IP stack introduced the octal parsing, perhaps even accidentally, and because BSD is the most commonly used code, that interpretation is also the most common one. But it's not the only interpretation. In fact, the earliest IP RFCs directly contradict the BSD implementation - they are pretty clear that IP addresses with leading zeros are meant to be interpreted as decimal. Furthermore, the claimed vulnerability is like a TOCTTOU problem where the check and use are handled by different software with differing interpretation of leading zeros. The right fix is, as it always is, to put the check and use together. Rejecting the leading zeros entirely avoids resolving the radix ambiguity the wrong way, which improves robustness. But it can also break existing code that might be processing config files that contain leading zeros and were happy with the radix-10 interpretation. Given that
we chose to make the change only in a new major version, as a robustness fix, rather than treat it as an urgent security fix that would require backporting. We do not want to push a breaking change that will keep people from being able to pick up critical Go 1.16 patches later. |
|
Thanks for the responses everyone. This is an unfortunate situation but I completely understand why you're handling it this way. |
Upstream don't believe it is a signifiant real world issue and will only fix in 1.17 onwards. Therefore exclude it from our reports. golang/go#30999 (comment) Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
Upstream don't believe it is a signifiant real world issue and will only fix in 1.17 onwards. Therefore exclude it from our reports. golang/go#30999 (comment) (From OE-Core rev: 5bd5faf0c34b47b2443975d66b71482d2380a01a) Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
Upstream don't believe it is a signifiant real world issue and will only fix in 1.17 onwards. Therefore exclude it from our reports. golang/go#30999 (comment) (From OE-Core rev: 5bd5faf0c34b47b2443975d66b71482d2380a01a) Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
Upstream don't believe it is a signifiant real world issue and will only fix in 1.17 onwards. Therefore exclude it from our reports. golang/go#30999 (comment) (From OE-Core rev: 5bd5faf0c34b47b2443975d66b71482d2380a01a) Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
Upstream don't believe it is a signifiant real world issue and will only fix in 1.17 onwards. Therefore exclude it from our reports. golang/go#30999 (comment) Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org> (cherry picked from commit 5bd5faf) Signed-off-by: Steve Sakoman <steve@sakoman.com>
Upstream don't believe it is a signifiant real world issue and will only fix in 1.17 onwards. Therefore exclude it from our reports. golang/go#30999 (comment) (From OE-Core rev: 9dfc6abbb83f8792fbfa1acb9c0fe4ab23872d8f) Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org> (cherry picked from commit 5bd5faf0c34b47b2443975d66b71482d2380a01a) Signed-off-by: Steve Sakoman <steve@sakoman.com> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
Source: poky MR: 112675, 112421 Type: Security Fix Disposition: Merged from poky ChangeID: 7f73831fde4c3e31bda5631ed966f33d8e9c9d45 Description: Upstream don't believe it is a signifiant real world issue and will only fix in 1.17 onwards. Therefore exclude it from our reports. golang/go#30999 (comment) (From OE-Core rev: 9dfc6abbb83f8792fbfa1acb9c0fe4ab23872d8f) Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org> (cherry picked from commit 5bd5faf0c34b47b2443975d66b71482d2380a01a) Signed-off-by: Steve Sakoman <steve@sakoman.com> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org> Signed-off-by: Jeremy A. Puhlman <jpuhlman@mvista.com>
Upstream don't believe it is a signifiant real world issue and will only fix in 1.17 onwards. Therefore exclude it from our reports. golang/go#30999 (comment) Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org> (cherry picked from commit 5bd5faf) Signed-off-by: Anuj Mittal <anuj.mittal@intel.com>
Upstream don't believe it is a signifiant real world issue and will only fix in 1.17 onwards. Therefore exclude it from our reports. golang/go#30999 (comment) (From OE-Core rev: 573337b8432677fa3a7643e74045ae7d7b331b3f) Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org> (cherry picked from commit 5bd5faf0c34b47b2443975d66b71482d2380a01a) Signed-off-by: Anuj Mittal <anuj.mittal@intel.com> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
|
So I'm not seeing this issue specifically mentioned in the Go 1.16 release notes -- is CVE-2021-29923 addressed in Go 1.16.x? And if so, which x? |
|
The net.ParseIP function rejects IPv4 addresses that contain decimal components with leading zeros in Go 1.17 but not in Go 1.16. Per #30999 (comment) we do not plan to backport this change to Go 1.16. |
|
Change https://golang.org/cl/361534 mentions this issue: |
Fixes #49365 Updates #30999 Change-Id: Ic92bce01b435baf70574c65524bde82f9cee3d8d Reviewed-on: https://go-review.googlesource.com/c/go/+/361534 Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com> Trust: Josh Bleecher Snyder <josharian@gmail.com> Trust: Brad Fitzpatrick <bradfitz@golang.org>
net.ParseIP no longer supports parsing IP addresses with leading zeroes: golang/go#30999
net.ParseIP no longer supports parsing IP addresses with leading zeroes: golang/go#30999
net.ParseIP no longer supports parsing IP addresses with leading zeroes: golang/go#30999
Upstream don't believe it is a signifiant real world issue and will only fix in 1.17 onwards. Therefore exclude it from our reports. golang/go#30999 (comment) Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org> (cherry picked from commit 5bd5faf)
What version of Go are you using (
go version)?Does this issue reproduce with the latest release?
Yes.
What operating system and processor architecture are you using (
go env)?go envOutputWhat did you do?
What did you expect to see?
7.7.7.017is interpreted as7.7.7.15.What did you see instead?
The program tries to connect to
7.7.7.17.The text was updated successfully, but these errors were encountered: