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: infinite loop in LookupAddr() #34660

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

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

andybons commented Oct 2, 2019

@andybons andybons added this to the Unplanned milestone Oct 2, 2019
@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented Oct 2, 2019

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

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Oct 2, 2019

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.

@bradfitz

This comment has been minimized.

Copy link
Member

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 and removed NeedsFix labels Oct 2, 2019
@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Oct 2, 2019

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

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Oct 2, 2019

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

@gopherbot gopherbot closed this in f0e940e Oct 2, 2019
@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Oct 2, 2019

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
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
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.