Bug 10471 - Incorrect NXDOMAIN response when records of another type exist
Summary: Incorrect NXDOMAIN response when records of another type exist
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.0
Classification: Unclassified
Component: DNS server (show other bugs)
Version: 4.0.7
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-02-25 16:36 UTC by Zoe O'Connell
Modified: 2014-04-04 18:55 UTC (History)
2 users (show)

See Also:


Attachments
Example of correct NODATA query response (2.05 KB, application/octet-stream)
2014-02-26 10:00 UTC, Zoe O'Connell
no flags Details
Bad DNS behavior (1.61 KB, application/octet-stream)
2014-02-26 10:05 UTC, Zoe O'Connell
no flags Details
Fix the invalid NXDOMAIN return. (3.22 KB, patch)
2014-02-27 22:51 UTC, Kai Blin
no flags Details
Patch for 4.1 and 4.0 (5.04 KB, patch)
2014-03-13 08:10 UTC, Kai Blin
kai: review? (abartlet)
jra: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Zoe O'Connell 2014-02-25 16:36:25 UTC
SAMBA DNS server incorrectly responds with NXDOMAIN when data for the domain name does exist but is not of the type requested.

The example most likely to arise in production is an IPv6 only entry in DNS, where a host will query for the A record, receive NXDOMAIN and not proceed any further because it believes the host does not exist.

Example wireshark output talking to a Samba server: (Using ping from a Windows client)
  1 DNS Standard query A servername.XXX.XXX.org
  2 DNS Standard query response, No such name

The same server from the same client, making an IPv6 specific request: (In this case, ping -6)
  1 DNS Standard query AAAA servername.XXX.XXX.org
  2 DNS Standard query response AAAA 2001:xxxx:xxxx:2::1:15

Correct behavior - note empty non-error response at packet 2.
  1 DNS Standard query A ipv6.l.google.com
  2 DNS Standard query response
  3 DNS Standard query AAAA ipv6.l.google.com
  4 DNS Standard query response AAAA 2a00:1450:4009:809::1004

Tests above performed against 4.0.7, but bug appears to be present in all versions.

The trouble seems to stem from handle_question in dns_query.c, which looks as if it returns NAME_ERROR if no records are to be returned.
Comment 1 Kai Blin 2014-02-25 23:23:46 UTC
Hi Zoe,

that seems weird, I'm pretty sure the NXDOMAIN reply complies with the RFC. Could you attach the wireshark capture so I could have a look at that?

Cheers,
Kai
Comment 2 Zoe O'Connell 2014-02-26 10:00:02 UTC
Created attachment 9724 [details]
Example of correct NODATA query response
Comment 3 Zoe O'Connell 2014-02-26 10:05:48 UTC
Created attachment 9725 [details]
Bad DNS behavior

Note times - this is two separate attempts to ping the host, one with just "ping" which fails and the other with "ping -6".
Comment 4 Zoe O'Connell 2014-02-26 10:08:00 UTC
Hi Kai,

I've attached two pcaps, one showing automatic query of the AAAA record using google, the other where it has to be done by hand due to the NXDOMAIN.

RFC1034 section 6.2.4 covers this particular case.

Thanks,

Zoe.
Comment 5 Björn Jacke 2014-02-26 10:40:23 UTC
reading the rfc i think zoe is right. if a name in dns exists (with whatever record type) we should not return any DNS error but an emtpy answer instead. This is what other name servers also do. This is also why the "host" command against Samba 4 servers usually return even for existing A records two NXDOMAIN errors - which come from the MX and AAAA lookups that "host" does by default.
Comment 6 Kai Blin 2014-02-27 06:41:30 UTC
Thanks for the captures, and the RFC reference. You're clearly correct, this is a bug in the DNS server. I'll work on a patch.
Comment 7 Kai Blin 2014-02-27 22:51:50 UTC
Created attachment 9734 [details]
Fix the invalid NXDOMAIN return.

This patch should fix the observed issue.
Comment 8 Zoe O'Connell 2014-03-03 16:34:28 UTC
Tested this patch and it resolves the issue. Will it make it into 4.1.6 and/or 4.0.16?

Thanks,

Zoe.
Comment 9 Kai Blin 2014-03-04 06:51:31 UTC
Once I've got this patch into master, I'll prepare the patches for 4.0.next and 4.1.next. The code won't have changed much, I don't expect any trouble.
Comment 10 Jeremy Allison 2014-03-12 21:20:31 UTC
(In reply to comment #9)
> Once I've got this patch into master, I'll prepare the patches for 4.0.next and
> 4.1.next. The code won't have changed much, I don't expect any trouble.

Kai, This is now in master. Do you need help getting this back-ported ?

Cheers,

Jeremy.
Comment 11 Kai Blin 2014-03-12 23:55:11 UTC
Jeremy, I was on the road the past days, I'll try to make the time to look at this tomorrow. Should be straightforward, the DNS code didn't change much in that area.
Comment 12 Kai Blin 2014-03-13 08:10:55 UTC
Created attachment 9771 [details]
Patch for 4.1 and 4.0

This patch applies cleanly to both v4-0-test and v4-1-test.
Comment 13 Jeremy Allison 2014-03-13 20:09:30 UTC
Comment on attachment 9771 [details]
Patch for 4.1 and 4.0

LGTM for 4.0.x and 4.1.x.

Jeremy.
Comment 14 Jeremy Allison 2014-03-13 20:09:59 UTC
Re-assigning to Karolin for inclusion in 4.1.next and 4.0.next.

Jeremy.
Comment 15 Jeremy Allison 2014-03-13 20:11:07 UTC
Kai, one external note (nothing to do with this bug but I noticed in test compile). On compiling I get:

[2185/4132] Compiling source4/dns_server/dns_query.c
../source4/dns_server/dns_query.c: In function ‘handle_question’:
../source4/dns_server/dns_query.c:331:23: warning: comparison between ‘enum dns_record_type’ and ‘const enum dns_qtype’ [-Wenum-compare]

which looks incorrect. Any chance you can take a look !

Cheers,

Jeremy.
Comment 16 Kai Blin 2014-03-13 23:53:17 UTC
These enums are coming from two different IDL files, but have identical numerical values. It'd be possible to add casts, but that'd make the code much harder to read. Not sure how to best fix this.
Comment 17 Jeremy Allison 2014-03-14 21:46:39 UTC
Thanks - no problem, let's let Metze's warnings patch take care of it. Doesn't affect Karolin merging this change.

Jeremy.
Comment 18 Karolin Seeger 2014-03-25 10:00:29 UTC
(In reply to comment #17)
> Thanks - no problem, let's let Metze's warnings patch take care of it. Doesn't
> affect Karolin merging this change.
> 
> Jeremy.

Pushed to autobuild-v4-1-test and autobuild-v4-0-test.
Comment 19 Karolin Seeger 2014-04-04 18:55:06 UTC
Pushed to v4-0-test and v4-1-test.
Closing out bug report.

Thanks!