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

net: reject leading zeros in IP address parsers [freeze exception] #30999

Closed
ghost opened this issue Mar 22, 2019 · 33 comments
Closed

net: reject leading zeros in IP address parsers [freeze exception] #30999

ghost opened this issue Mar 22, 2019 · 33 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted release-blocker Security
Milestone

Comments

@ghost
Copy link

ghost commented Mar 22, 2019

What version of Go are you using (go version)?

$ go version
go version go1.12.1 linux/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/xxx/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/xxx/gocode"
GOPROXY=""
GORACE=""
GOROOT="/home/xxx/go"
GOTMPDIR=""
GOTOOLDIR="/home/xxx/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build040813268=/tmp/go-build -gno-record-gcc-switches"

What did you do?

package main

import "net/http"

func main() {
	http.Get("http://7.7.7.017")
}

What did you expect to see?

7.7.7.017 is interpreted as 7.7.7.15.

$ ping 7.7.7.017
PING 7.7.7.017 (7.7.7.15) 56(84) bytes of data.

What did you see instead?

The program tries to connect to 7.7.7.17.

@agnivade agnivade changed the title net/http: octal literals in IP addresses are interpreted as decimal ones net/url: Parse interprets octal literals in IP addresses as decimal ones Mar 22, 2019
@agnivade
Copy link
Contributor

This is more of a net/url issue rather than net/http.

What does the RFC say regarding this ?

@ghost
Copy link
Author

ghost commented Mar 22, 2019

I believe that Parse doesn't try to interpret the hostname at all.

@julieqiu julieqiu added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 22, 2019
@julieqiu julieqiu added this to the Go1.13 milestone Apr 22, 2019
@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@secenv
Copy link

secenv commented Mar 31, 2021

As shown in this recent article, this behavior could be used in server-side request forgery, local file inclusion and remote file inclusion vulnerabilities.

@tv42
Copy link

tv42 commented Mar 31, 2021

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

All the forms except for decimal octets are seen as non-standard (despite being quite widely interoperable) and undesirable.

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.

@tv42
Copy link

tv42 commented Mar 31, 2021

And as I discussed with @secenv on IRC, that article is naive. Typical "attacks" that 0127.0.0.1 enables are enabled also by evil.example.com A 127.0.0.1 in DNS, and the fix for both is to check the target IP after resolving, basically &http.Client{Transport: &http.Transport{DialContext: dialOnlySafeIPs}}

@secenv
Copy link

secenv commented Mar 31, 2021

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.

@tv42
Copy link

tv42 commented Mar 31, 2021

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.

@rsc
Copy link
Contributor

rsc commented Apr 5, 2021

I agree about changing Go's IP address parsers
(ParseIP, ParseCIDR, any others) to reject leading zeros (except "0"),
because:

(1) the RFCs are mostly quiet but in a few places hint that decimal is the right interpretation,
(2) Go interprets leading zeros as decimal, and
(3) BSD stacks nonetheless interpret leading zeros as octal.
(4) The fact that basically no one has noticed this divergence implies
that essentially no one uses leading zeros in IP addresses.

It seems like an open question whether this should be done
in a point release or saved for the next major release (Go 1.17).
But to start, we should agree to do it at all.
Adding to the proposal process.

@rsc rsc changed the title net/url: Parse interprets octal literals in IP addresses as decimal ones net/url: reject leading zeros in IP address parsers Apr 5, 2021
@rsc rsc changed the title net/url: reject leading zeros in IP address parsers proposal: net/url: reject leading zeros in IP address parsers Apr 5, 2021
@rsc rsc modified the milestones: Backlog, Proposal Apr 5, 2021
@rsc rsc removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 5, 2021
@rsc rsc added this to Incoming in Proposals (old) Apr 5, 2021
@rsc rsc added the Proposal label Apr 5, 2021
@FiloSottile
Copy link
Contributor

And as I discussed with @secenv on IRC, that article is naive. Typical "attacks" that 0127.0.0.1 enables are enabled also by evil.example.com A 127.0.0.1 in DNS, and the fix for both is to check the target IP after resolving, basically &http.Client{Transport: &http.Transport{DialContext: dialOnlySafeIPs}}

I find this pretty convincing, especially given that net.Dial and net.Listen will parse the IPs as decimal.

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.

@bradfitz
Copy link
Contributor

bradfitz commented Apr 7, 2021

Related: #43389 ("net: limit the size of ParseIP input?")

@rsc
Copy link
Contributor

rsc commented Apr 7, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Active in Proposals (old) Apr 7, 2021
@FiloSottile FiloSottile changed the title proposal: net/url: reject leading zeros in IP address parsers proposal: net: reject leading zeros in IP address parsers Apr 8, 2021
halstead pushed a commit to openembedded/openembedded-core that referenced this issue Sep 7, 2021
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>
kraj pushed a commit to YoeDistro/poky-old that referenced this issue Sep 7, 2021
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>
kraj pushed a commit to YoeDistro/poky-old that referenced this issue Sep 7, 2021
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>
seambot pushed a commit to seamapi/poky that referenced this issue Sep 7, 2021
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>
halstead pushed a commit to openembedded/openembedded-core that referenced this issue Sep 14, 2021
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>
seambot pushed a commit to seamapi/poky that referenced this issue Sep 14, 2021
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>
jpuhlman pushed a commit to MontaVista-OpenSourceTechnology/poky that referenced this issue Sep 16, 2021
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>
halstead pushed a commit to openembedded/openembedded-core that referenced this issue Sep 17, 2021
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>
seambot pushed a commit to seamapi/poky that referenced this issue Sep 17, 2021
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>
@benjsmi
Copy link

benjsmi commented Sep 22, 2021

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?

@ianlancetaylor
Copy link
Contributor

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.

@gopherbot
Copy link

Change https://golang.org/cl/361534 mentions this issue: net/netip: don't accept ParseAddr with leading zeros

gopherbot pushed a commit that referenced this issue Nov 5, 2021
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>
wolffberg added a commit to wolffberg/terraform-provider-dns that referenced this issue Sep 15, 2022
net.ParseIP no longer supports parsing IP addresses with leading zeroes: golang/go#30999
wolffberg added a commit to danskespil/terraform-provider-dns that referenced this issue Sep 30, 2022
net.ParseIP no longer supports parsing IP addresses with leading zeroes: golang/go#30999
wolffberg added a commit to danskespil/terraform-provider-dns that referenced this issue Sep 30, 2022
net.ParseIP no longer supports parsing IP addresses with leading zeroes: golang/go#30999
nmbath pushed a commit to victronenergy/openembedded-core that referenced this issue Mar 5, 2023
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)
@golang golang locked and limited conversation to collaborators Jun 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted release-blocker Security
Projects
No open projects
Development

No branches or pull requests