Opened 10 years ago

Closed 9 years ago

#70 closed defect (fixed)

auth server doesn't handle NS query at a zone cut correctly

Reported by: jinmei Owned by: each
Priority: medium Milestone: 06. 4th Incremental Release
Component: b10-auth Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 0.0 Add Hours to Ticket:
Total Hours: Internal?: no

Description

Assume you set up jinmei.org, convert the zone file into sqlite3 format and load it with the BIND10 auth server, and in jinmei.org, there's a delegation:

sec.jinmei.org. 600 IN NS ns.sec.jinmei.org.
ns.sec.jinmei.org. 600 IN A 203.178.141.207

With this configuration, the BIND10 auth server returns an authoritative answer for sec.jinmei.org/NS:

;; flags: qr aa rd; QUERY: 1, ANSWER: 1, AUTHORITY: 0, ADDITIONAL: 1
;; WARNING: recursion requested but not available

;; QUESTION SECTION:
;sec.jinmei.org. IN NS

;; ANSWER SECTION:
sec.jinmei.org. 600 IN NS ns.sec.jinmei.org.

;; ADDITIONAL SECTION:
ns.sec.jinmei.org. 600 IN A 203.178.141.207

This is not correct.

It answers correctly for foo.sec.jinmei.org/something, sec.jinmei.org/A, etc.

Subtickets

Change History (17)

comment:1 in reply to: ↑ description Changed 10 years ago by each

Thank you. I believe this is fixed in revision 1136.

comment:2 Changed 10 years ago by each

  • Resolution set to fixed
  • Status changed from new to closed

Unit test added; closing this ticket.

comment:3 Changed 10 years ago by jinmei

  • Resolution fixed deleted
  • Status changed from closed to reopened

It still returns an AA answer for "sec.jinmei.org/DNAME". I'm not actually sure what's the correct behavior in this case, but both BIND 9 and nsd return a referral. Reopening the ticket.

comment:4 Changed 10 years ago by jinmei

  • Status changed from reopened to assigned

comment:5 Changed 10 years ago by each

  • Owner changed from each to jinmei

I believe this is corrected in r1433. Can you confirm?

comment:6 follow-up: Changed 10 years ago by jinmei

is there a test for this?

comment:7 Changed 10 years ago by jinmei

  • Owner changed from jinmei to each

comment:8 in reply to: ↑ 6 Changed 10 years ago by each

is there a test for this?

No, because I wasn't entirely sure I understood the error. I fixed a different bug, then tried to reproduce this one and found that it seemed to have gone away at the same time. I asked you to confirm because I might not have understood the test case correctly.

If I did understand it, then the unit test would be to add a query for subzone.example.com./DNAME to datasrc_unittest.cc, and check for a non-authoritative delegation.

comment:9 follow-up: Changed 10 years ago by jinmei

This specific problem was gone.

I guess it's due to this line of r1433:

((task->qtype == RRType::NSEC()
task->qtype == RRType::DS()

task->qtype == RRType::DNAME()) &&

===> data.findRRset(task->qtype, task->qclass)))) {

(Please don't naively treat the return value of findRRset() as a boolean, btw. C++ has an explicit boolean type, and implict type conversion or counter-intuitive operator overloading magic happen quite often, so this could be a real issue unlike plain old C)

I'm not sure if this is the currect fix. If findRRset() indicates a SUCCESS, it means a referral flag is set but DNAME RR was found. Doesn't this mean the underlying zone is broken? If so, we should treat the result that way.

comment:10 in reply to: ↑ 9 ; follow-up: Changed 10 years ago by each

(Please don't naively treat the return value of findRRset() as a boolean, btw. C++ has an explicit boolean type, and implict type conversion or counter-intuitive operator overloading magic happen quite often, so this could be a real issue unlike plain old C)

I thought we decided some time ago that we would do this, because a shared_ptr (such as the RRsetPtr that findRRset() returns) is never NULL.

I'm not sure if this is the currect fix. If findRRset() indicates a SUCCESS, it means a referral flag is set but DNAME RR was found. Doesn't this mean the underlying zone is broken?

By design, a DNAME is considered a form of referral, so the concrete data source sets the REFERRAL flag. See q_referral_str in sqlite3_datasrc.cc. The point of this code is that if you specifically query for the DNAME record, the REFERRAL flag is cleared, so that you will simply receive the DNAME without any further processing.

That may not be the best design and I'm certainly open to revisiting it, but it seems to work okay for the nonce.

I'll add a DNAME query to the data source unittest now.

comment:11 in reply to: ↑ 10 ; follow-up: Changed 10 years ago by jinmei

(Please don't naively treat the return value of findRRset() as a boolean, btw. C++ has an explicit boolean type, and implict type conversion or counter-intuitive operator overloading magic happen quite often, so this could be a real issue unlike plain old C)

I thought we decided some time ago that we would do this, because a shared_ptr (such as the RRsetPtr that findRRset() returns) is never NULL.

Ah, I didn't know boost::shared_ptr has operator() that returns bool. I didn't remember the decision (is this perhaps another drawback example of jabber discussion?), but nevertheless this usage is okay.

I'm not sure if this is the currect fix. If findRRset() indicates a SUCCESS, it means a referral flag is set but DNAME RR was found. Doesn't this mean the underlying zone is broken?

By design, a DNAME is considered a form of referral, so the concrete data source sets the REFERRAL flag. See q_referral_str in sqlite3_datasrc.cc. The point of this code is that if you specifically query for the DNAME record, the REFERRAL flag is cleared, so that you will simply receive the DNAME without any further processing.

I'm afraid you missed my point or I was not clear enough. What I tried to point out is that if the zone was incorrectly set up with the following:

sec.jinmei.org. 600 IN NS ns.sec.jinmei.org.

3600 IN DNAME example.com.

and consider the case where we ask for sec.jinmei.org/DNAME.

In this case, BIND 9 ignores the NS and exclusive use DNAME for names equal to under sec.jinmei.org. BIND 10 still returns a NS referral if we ask sec.jinmei.org/NS.

comment:12 in reply to: ↑ 11 ; follow-up: Changed 10 years ago by each

I'm not sure if this is the currect fix. If findRRset() indicates a SUCCESS, it means a referral flag is set but DNAME RR was found. Doesn't this mean the underlying zone is broken?

By design, a DNAME is considered a form of referral, so the concrete data source sets the REFERRAL flag.

I'm afraid you missed my point or I was not clear enough. What I tried to point out is that if the zone was incorrectly set up with the following:
[...]
In this case, BIND 9 ignores the NS and exclusive use DNAME for names equal to under sec.jinmei.org. BIND 10 still returns a NS referral if we ask sec.jinmei.org/NS.

Ah, I see your point now. Yes, you're right about that last point.

If findRRest() returns SUCCESS, that means that there was a DNAME. Since the REFERRAL flag is always set if there's a DNAME, that's not evidence that the zone is broken.

If the zone *is* insane, and there is both a DNAME and NS at the same node, the server will usually do the right thing. For example, if you query for foo.sec.jinmei.org/A, the server will process the DNAME and send you to foo.example.com, and the spurious NS record will be ignored.

However, if you explicitly query for sec.jinmei.org/NS, it will give you an answer. Oops. I think we should make a note of this and address it in Y2, so we have time to be sure the solution is complete.

(BTW, will BIND9 load a zone like that? It seems like it ought to be forbidden at load time.)

comment:13 in reply to: ↑ 12 Changed 10 years ago by jinmei

Replying to each:

I'm afraid you missed my point or I was not clear enough. What I tried to point out is that if the zone was incorrectly set up with the following:
[...]
In this case, BIND 9 ignores the NS and exclusive use DNAME for names equal to under sec.jinmei.org. BIND 10 still returns a NS referral if we ask sec.jinmei.org/NS.

Ah, I see your point now. Yes, you're right about that last point.

If findRRest() returns SUCCESS, that means that there was a DNAME. Since the REFERRAL flag is always set if there's a DNAME, that's not evidence that the zone is broken.

If the zone *is* insane, and there is both a DNAME and NS at the same node, the server will usually do the right thing. For example, if you query for foo.sec.jinmei.org/A, the server will process the DNAME and send you to foo.example.com, and the spurious NS record will be ignored.

However, if you explicitly query for sec.jinmei.org/NS, it will give you an answer. Oops. I think we should make a note of this and address it in Y2, so we have time to be sure the solution is complete.

(Although it's already in Y2:-) that's fine with me. Basically, I think we should revisit the whole query processing logic in Y2 rather than handling each and every bug like this in a case by case manner. That is, we should redesign the architecture of a zone (in BIND10 datasource) taking into account lessons we've learned so far.

(BTW, will BIND9 load a zone like that? It seems like it ought to be forbidden at load time.)

Yes, surprisingly to me. I'd say it's a bug of BIND9.

comment:14 Changed 9 years ago by shane

  • Component changed from Unclassified to b10-auth

comment:15 Changed 9 years ago by jreed

  • billable set to 0
  • Estimated Difficulty set to 0.0
  • Internal? unset

It appears that the ticket may have changed topics somewhat.
What is current status?

comment:16 Changed 9 years ago by jreed

  • Milestone changed from 05. 3rd Incremental Release: Serious Secondary to 06. 4th Incremental Release

comment:17 Changed 9 years ago by larissas

  • Resolution set to fixed
  • Status changed from assigned to closed
Note: See TracTickets for help on using tickets.