code review 4432089: net: don't crash on unexpected DNS SRV responses

net: don't crash on unexpected DNS SRV responses

Fixes issue 1350

[bradfitz, 2011-05-02 21:36:18]

Hello rsc (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg

[rsc, 2011-05-02 21:38:45]

There are others in this file.

[bradfitz, 2011-05-02 21:40:34]

Oh, indeed.

And other bugs:

        if len(rr) >= 0 {
                cname = rr[0].(*dnsRR_CNAME).Cname
        }

On Mon, May 2, 2011 at 2:38 PM, Russ Cox <rsc@golang.org> wrote:

> There are others in this file.
>

[rsc, 2011-05-02 21:44:15]

actually hang on

func answer in dnsclient.go only returns dnsRR
that have the given qtype.  if you are only
returning the dnsRR with qtype == TypeSRV
how can the rr not have go type *dnsRR_SRV?

[bradfitz, 2011-05-02 21:46:35]

Hello rsc (cc: golang-dev@googlegroups.com),

Please take another look.

[bradfitzgoog, 2011-05-02 21:48:17]

looking.

I was just going off the stack trace in the bug.

On Mon, May 2, 2011 at 2:44 PM, Russ Cox <rsc@golang.org> wrote:

> actually hang on
>
> func answer in dnsclient.go only returns dnsRR
> that have the given qtype.  if you are only
> returning the dnsRR with qtype == TypeSRV
> how can the rr not have go type *dnsRR_SRV?
>

[rsc, 2011-05-02 22:01:32]

weird.  that trace would happen if the map lookup failed in

	// make an rr of that type and re-unpack.
	// again inefficient but doesn't need to be fast.
	mk, known := rr_mk[int(h.Rrtype)]
	if !known {
		return &h, end, true
	}

but it would only be taken by answer if subsequently

		for i := 0; i < len(dns.answer); i++ {
			rr := dns.answer[i]
			h := rr.Header()
			if h.Class == dnsClassINET && h.Name == name {
				switch h.Rrtype {
				case qtype:
					n := len(addrs)
					addrs = addrs[0 : n+1]
					addrs[n] = rr
				case dnsTypeCNAME:
					// redirect to cname
					name = rr.(*dnsRR_CNAME).Cname
					continue Cname
				}
			}
		}

h.Rrtype == qtype, which is to say == dnsTypeSRV.  and yet the rr_mk map says

	dnsTypeSRV:   func() dnsRR { return new(dnsRR_SRV) },

[bradfitz, 2011-05-02 22:04:50]

But if unpackRR returns false:

        dns.answer = make([]dnsRR, dh.Ancount)
...
        for i := 0; i < len(dns.answer); i++ {
                dns.answer[i], off, ok = unpackRR(msg, off)
        }

All this codes is pre-append.

Should this be:

        dns.answer = make([]dnsRR, 0, dh.Ancount)
        for i := 0; i < dh.Ancount; i++ {
                if rec, off, ok = unpackRR(msg, off); ok {
                       dns.answer = append(dns.answer, rec)
                }
        }

?


On Mon, May 2, 2011 at 3:01 PM, Russ Cox <rsc@golang.org> wrote:

> weird.  that trace would happen if the map lookup failed in
>
>        // make an rr of that type and re-unpack.
>        // again inefficient but doesn't need to be fast.
>        mk, known := rr_mk[int(h.Rrtype)]
>        if !known {
>                return &h, end, true
>        }
>
> but it would only be taken by answer if subsequently
>
>                for i := 0; i < len(dns.answer); i++ {
>                        rr := dns.answer[i]
>                        h := rr.Header()
>                        if h.Class == dnsClassINET && h.Name == name {
>                                switch h.Rrtype {
>                                case qtype:
>                                        n := len(addrs)
>                                        addrs = addrs[0 : n+1]
>                                        addrs[n] = rr
>                                case dnsTypeCNAME:
>                                        // redirect to cname
>                                        name = rr.(*dnsRR_CNAME).Cname
>                                        continue Cname
>                                }
>                        }
>                }
>
> h.Rrtype == qtype, which is to say == dnsTypeSRV.  and yet the rr_mk map
> says
>
>        dnsTypeSRV:   func() dnsRR { return new(dnsRR_SRV) },
>

[rsc, 2011-05-02 22:19:27]

On Mon, May 2, 2011 at 18:04, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> But if unpackRR returns false:

Then the rr is nil, no?

>
>         dns.answer = make([]dnsRR, dh.Ancount)
> ...
>         for i := 0; i < len(dns.answer); i++ {
>                 dns.answer[i], off, ok = unpackRR(msg, off)
>         }
> All this codes is pre-append.
> Should this be:
>
>         dns.answer = make([]dnsRR, 0, dh.Ancount)
>         for i := 0; i < dh.Ancount; i++ {
>                 if rec, off, ok = unpackRR(msg, off); ok {
>                        dns.answer = append(dns.answer, rec)
>                 }
>         }

The existing code

	for i := 0; i < len(dns.answer); i++ {
		dns.answer[i], off, ok = unpackRR(msg, off)
	}
	etc

assumes that the only reason unpackRR fails is
that it runs out of message to decode.  When it does
fail it returns off == len(msg), which will cause any
future calls to fail.

So only the last ok needs to be checked.
So then the if !ok at the end of Unpack should
take care of it.

I think I might have been a little carried away
with multiple return values here - this is some of
the very earliest Go code I wrote - but I also think
it's correct.

I just can't see any way that you can end up with
header type dnsTypeSRV (so that answer will
return it) and at the same time not have type *dnsRR_SRV
(because the type must be in rr_mk).

Obviously it happens.

Maybe it's time to ask the issue reporter if it is
reproducible and if so send some code to save
the DNS packet?

Russ

[bradfitz, 2011-05-02 22:19:37]

Hello rsc, bradfitzwork (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2011-05-02 22:21:43]

I like the range loops but don't want to put the interface
checks in until we know why the assertions are wrong.

[bradfitz, 2011-05-02 22:44:37]

Hello rsc, bradfitzwork (cc: golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2011-05-02 22:50:50]

This CL's mostly paranoia (some unnecessary) and append cleanup, but my
leading theory for the actual issue is in dnsmsg.go, unpackRR.  If a RR
header says it's of some known type and has a h.Rdlength that's too long,
that's then stored in &h.  We then lookup the maker in rr_mk, create one,
and call unpackStruct on it, which returns a different ending offset.  But:

        off, ok = unpackStruct(rr, msg, off0)
        if off != end {
                return &h, end, true
        }

We now return a header that says it's, say, a dnsTypeSRV, but the type of it
is actually just *dnsRR_Header, since we hit the error case.

I'll craft such a response packet and put it into a test to confirm.

On Mon, May 2, 2011 at 3:44 PM, <bradfitz@golang.org> wrote:

> Hello rsc, bradfitzwork (cc: golang-dev@googlegroups.com),
>
> Please take another look.
>
>
> http://codereview.appspot.com/4432089/
>

[bradfitz, 2011-05-02 23:22:38]

Theory confirmed.

CL updated with a failing test:

http://codereview.appspot.com/4432089/diff/4005/src/pkg/net/dnsmsg_test.go

=== RUN  net.TestDNSParseSRVReply
--- PASS: net.TestDNSParseSRVReply (0.00 seconds)
=== RUN  net.TestDNSParseCorruptSRVReply
--- FAIL: net.TestDNSParseCorruptSRVReply (0.00 seconds)
answer[4] = *net.dnsRR_Header; want *dnsRR_SRV
FAIL

<http://codereview.appspot.com/4432089/diff/16001/src/pkg/net/dnsmsg_test.go>...
but all the headers indicate that the types are SRV.

On Mon, May 2, 2011 at 3:50 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> This CL's mostly paranoia (some unnecessary) and append cleanup, but my
> leading theory for the actual issue is in dnsmsg.go, unpackRR.  If a RR
> header says it's of some known type and has a h.Rdlength that's too long,
> that's then stored in &h.  We then lookup the maker in rr_mk, create one,
> and call unpackStruct on it, which returns a different ending offset.  But:
>
>         off, ok = unpackStruct(rr, msg, off0)
>         if off != end {
>                 return &h, end, true
>         }
>
> We now return a header that says it's, say, a dnsTypeSRV, but the type of
> it is actually just *dnsRR_Header, since we hit the error case.
>
> I'll craft such a response packet and put it into a test to confirm.
>
> On Mon, May 2, 2011 at 3:44 PM, <bradfitz@golang.org> wrote:
>
>> Hello rsc, bradfitzwork (cc: golang-dev@googlegroups.com),
>>
>> Please take another look.
>>
>>
>> http://codereview.appspot.com/4432089/
>>
>
>

[rsc, 2011-05-02 23:27:05]

Cool lets just not return the Headers in the rr list.  Then the assertions
are correct.
On May 2, 2011 7:22 PM, "Brad Fitzpatrick" <bradfitz@golang.org> wrote:
> Theory confirmed.
>
> CL updated with a failing test:
>
> http://codereview.appspot.com/4432089/diff/4005/src/pkg/net/dnsmsg_test.go
>
> === RUN net.TestDNSParseSRVReply
> --- PASS: net.TestDNSParseSRVReply (0.00 seconds)
> === RUN net.TestDNSParseCorruptSRVReply
> --- FAIL: net.TestDNSParseCorruptSRVReply (0.00 seconds)
> answer[4] = *net.dnsRR_Header; want *dnsRR_SRV
> FAIL
>
> <
http://codereview.appspot.com/4432089/diff/16001/src/pkg/net/dnsmsg_test.go
>...
> but all the headers indicate that the types are SRV.
>
> On Mon, May 2, 2011 at 3:50 PM, Brad Fitzpatrick <bradfitz@golang.org
>wrote:
>
>> This CL's mostly paranoia (some unnecessary) and append cleanup, but my
>> leading theory for the actual issue is in dnsmsg.go, unpackRR. If a RR
>> header says it's of some known type and has a h.Rdlength that's too long,
>> that's then stored in &h. We then lookup the maker in rr_mk, create one,
>> and call unpackStruct on it, which returns a different ending offset.
But:
>>
>> off, ok = unpackStruct(rr, msg, off0)
>> if off != end {
>> return &h, end, true
>> }
>>
>> We now return a header that says it's, say, a dnsTypeSRV, but the type of
>> it is actually just *dnsRR_Header, since we hit the error case.
>>
>> I'll craft such a response packet and put it into a test to confirm.
>>
>> On Mon, May 2, 2011 at 3:44 PM, <bradfitz@golang.org> wrote:
>>
>>> Hello rsc, bradfitzwork (cc: golang-dev@googlegroups.com),
>>>
>>> Please take another look.
>>>
>>>
>>> http://codereview.appspot.com/4432089/
>>>
>>
>>

[bradfitz, 2011-05-02 23:35:51]

Agreed. Patching up answer() and tests now, reverting comma-ok changes.

On Mon, May 2, 2011 at 4:27 PM, Russ Cox <rsc@golang.org> wrote:

> Cool lets just not return the Headers in the rr list.  Then the assertions
> are correct.
> On May 2, 2011 7:22 PM, "Brad Fitzpatrick" <bradfitz@golang.org> wrote:
> > Theory confirmed.
> >
> > CL updated with a failing test:
> >
> >
> http://codereview.appspot.com/4432089/diff/4005/src/pkg/net/dnsmsg_test.go
> >
> > === RUN net.TestDNSParseSRVReply
> > --- PASS: net.TestDNSParseSRVReply (0.00 seconds)
> > === RUN net.TestDNSParseCorruptSRVReply
> > --- FAIL: net.TestDNSParseCorruptSRVReply (0.00 seconds)
> > answer[4] = *net.dnsRR_Header; want *dnsRR_SRV
> > FAIL
> >
> > <
> http://codereview.appspot.com/4432089/diff/16001/src/pkg/net/dnsmsg_test.go
> >...
>
> > but all the headers indicate that the types are SRV.
> >
> > On Mon, May 2, 2011 at 3:50 PM, Brad Fitzpatrick <bradfitz@golang.org
> >wrote:
> >
> >> This CL's mostly paranoia (some unnecessary) and append cleanup, but my
> >> leading theory for the actual issue is in dnsmsg.go, unpackRR. If a RR
> >> header says it's of some known type and has a h.Rdlength that's too
> long,
> >> that's then stored in &h. We then lookup the maker in rr_mk, create one,
> >> and call unpackStruct on it, which returns a different ending offset.
> But:
> >>
> >> off, ok = unpackStruct(rr, msg, off0)
> >> if off != end {
> >> return &h, end, true
> >> }
> >>
> >> We now return a header that says it's, say, a dnsTypeSRV, but the type
> of
> >> it is actually just *dnsRR_Header, since we hit the error case.
> >>
> >> I'll craft such a response packet and put it into a test to confirm.
> >>
> >> On Mon, May 2, 2011 at 3:44 PM, <bradfitz@golang.org> wrote:
> >>
> >>> Hello rsc, bradfitzwork (cc: golang-dev@googlegroups.com),
> >>>
> >>> Please take another look.
> >>>
> >>>
> >>> http://codereview.appspot.com/4432089/
> >>>
> >>
> >>
>

[bradfitz, 2011-05-02 23:52:26]

Hello rsc, bradfitzwork (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2011-05-03 03:06:52]

LGTM

http://codereview.appspot.com/4432089/diff/8003/src/pkg/net/dnsclient.go
File src/pkg/net/dnsclient.go (right):

http://codereview.appspot.com/4432089/diff/8003/src/pkg/net/dnsclient.go#newc...
src/pkg/net/dnsclient.go:389: if len(rr) == 0 {
can't happen

http://codereview.appspot.com/4432089/diff/8003/src/pkg/net/dnsmsg.go
File src/pkg/net/dnsmsg.go (right):

http://codereview.appspot.com/4432089/diff/8003/src/pkg/net/dnsmsg.go#newcode729
src/pkg/net/dnsmsg.go:729: if rec, off, ok = unpackRR(msg, off); ok {
rec, off, ok = unpackRR(msg, off)
if !ok {
    return false
}
dns.answer = append(dns.answer, rec)

etc

[bradfitz, 2011-05-03 14:10:38]

Hello rsc, bradfitzwork (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitz, 2011-05-03 14:11:02]

*** Submitted as http://code.google.com/p/go/source/detail?r=05140e700807 ***

net: don't crash on unexpected DNS SRV responses

Fixes issue 1350

R=rsc, bradfitzwork
CC=golang-dev
http://codereview.appspot.com/4432089