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

Jump to bottom

net: infinite loop in LookupAddr() #34660

Closed
mndrix opened this issue Oct 2, 2019 · 8 comments
Closed

net: infinite loop in LookupAddr() #34660

mndrix opened this issue Oct 2, 2019 · 8 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unplanned

Comments

@mndrix
Copy link
Contributor

mndrix commented Oct 2, 2019

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

$ go version
go version go1.13.1 openbsd/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
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/mndrix/.cache/go-build"
GOENV="/home/mndrix/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="openbsd"
GONOPROXY=""
GONOSUMDB=""
GOOS="openbsd"
GOPATH="/home/mndrix/gopath"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/mndrix/src/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/mndrix/src/go/pkg/tool/openbsd_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/mndrix/src/go/src/go.mod"
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"
GOROOT/bin/go version: go version go1.13.1 openbsd/amd64
GOROOT/bin/go tool compile -V: compile version go1.13.1
uname -v: GENERIC.MP#1
gdb --version: GNU gdb 6.3

What did you do?

package main

import (
	"fmt"
	"net"
)

func main() {
	addr := "104.196.215.101"
	fmt.Printf("looking up %s\n", addr)
	names, err := net.LookupAddr(addr)
	if err != nil {
		panic(err)
	}
	fmt.Printf("%v\n", names)
}

What did you expect to see?

looking up 104.196.215.101
[smtp2.shopify.com.]

What did you see instead?

looking up 104.196.215.101

Hangs with 100% CPU.
@mndrix
Copy link
Contributor Author

mndrix commented Oct 2, 2019

It looks like the problem is caused by a DNS response that includes the requested PTR record along with a RRSIG record. Resolver.goLookupPTR doesn't correctly ignore non-PTR records so it enters an infinite loop.

This patch fixes the problem for me:

diff --git a/src/net/dnsclient_unix.go b/src/net/dnsclient_unix.go
index e0a7ef8552..ce59e7861a 100644
--- a/src/net/dnsclient_unix.go
+++ b/src/net/dnsclient_unix.go
@@ -765,6 +765,7 @@ func (r *Resolver) goLookupPTR(ctx context.Context, addr string) ([]string, erro
 			}
 		}
 		if h.Type != dnsmessage.TypePTR {
+			p.SkipAnswer()
 			continue
 		}
 		ptr, err := p.PTRResource()

I have a change request ready, but I have a couple questions first:

  • p.SkipAnswer() can't return an error in this case, but should the error be checked anyway to be safe against future changes?
  • The tests I wrote for this issue hard code some shopify.com IP addresses. Is there a better way to test this kind of thing that isn't so fragile?

@andybons
Copy link
Member

andybons commented Oct 2, 2019

@bradfitz @mikioh

@andybons andybons added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 2, 2019
@andybons andybons added this to the Unplanned milestone Oct 2, 2019
@bradfitz
Copy link
Contributor

bradfitz commented Oct 2, 2019

@gopherbot, please backport to Go 1.12 and Go 1.13. This looks like a DoS vector.

@gopherbot gopherbot mentioned this issue Oct 2, 2019
Closed
@gopherbot
Copy link

Backport issue(s) opened: #34661 (for 1.12), #34662 (for 1.13).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@gopherbot gopherbot mentioned this issue Oct 2, 2019
Closed
@bradfitz
Copy link
Contributor

bradfitz commented Oct 2, 2019

@mndrix, I can't reproduce with my DNS server(s). But the bug looks real & fix looks correct.

The tests I wrote for this issue hard code some shopify.com IP addresses. Is there a better way to test this kind of thing that isn't so fragile?

We have tests that work with in-memory fake servers. See lookupWithFake and fakeDNSServer in dnsclient_unix_test.go.

@bradfitz bradfitz added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsFix The path to resolution is known, but the work has not been done. labels Oct 2, 2019
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 2, 2019
@gopherbot
Copy link

Change https://golang.org/cl/198460 mentions this issue: net: avoid an infinite loop in LookupAddr()

@gopherbot
Copy link

Change https://golang.org/cl/198489 mentions this issue: [release-branch.go1.13] net: avoid an infinite loop in LookupAddr

@gopherbot
Copy link

Change https://golang.org/cl/198497 mentions this issue: [release-branch.go1.12] net: avoid an infinite loop in LookupAddr

gopherbot pushed a commit that referenced this issue Oct 3, 2019
@mndrix @bradfitz
[release-branch.go1.13] net: avoid an infinite loop in LookupAddr
f262eed 
If a request for a PTR record returned a response with a non-PTR
answer, goLookupPTR would loop forever.  Skipping non-PTR answers
guarantees progress through the DNS response.

Fixes #34662
Updates #34660

Change-Id: I56f9d21e5342d07e7d843d253267e93a29707904
Reviewed-on: https://go-review.googlesource.com/c/go/+/198460
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
(cherry picked from commit f0e940e)
Reviewed-on: https://go-review.googlesource.com/c/go/+/198489
Reviewed-by: Michael Hendricks <michael@ndrix.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
gopherbot pushed a commit that referenced this issue Oct 3, 2019
@bradfitz
[release-branch.go1.12] net: avoid an infinite loop in LookupAddr
273d946 
If a request for a PTR record returned a response with a non-PTR
answer, goLookupPTR would loop forever.  Skipping non-PTR answers
guarantees progress through the DNS response.

Fixes #34661
Updates #34660

Change-Id: Ib5e5263243bc34b9e2f85aa2b913c9cd50dbcaa5
Reviewed-on: https://go-review.googlesource.com/c/go/+/198497
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
@golang golang locked and limited conversation to collaborators Oct 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
4 participants
@bradfitz @mndrix @andybons @gopherbot