Opened 10 years ago

Closed 7 years ago

#145 closed defect (wontfix)

DNAME-CNAME synthesis can throw if the result is too long.

Reported by: jinmei Owned by: UnAssigned
Priority: medium Milestone:
Component: data source Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 0.0 Add Hours to Ticket:
Total Hours: Internal?: no

Description

see r1727.

BIND 9 catches this case in dns_name_concatenate() and results in YXDOMAIN.

In our case I think we should explicitly identify it rather than let Name::concatenate() throw.

Note that this bug could be "serious" because if the server manages a zone containing DNAME (whose target name is longer than owner name), a malicious remote node can trigger the bug. for our current auth server implementation catches the exception at a higher level, so it cannot actually be used as a DoS, but it's potentially dangerious. So I categorize it as "critical".

Giving it to Evan.

Subtickets

Attachments (1)

trac145.diff (2.4 KB) - added by each 9 years ago.

Download all attachments as: .zip

Change History (24)

comment:1 Changed 9 years ago by shane

  • Component changed from Unclassified to data source

Changed 9 years ago by each

comment:2 Changed 9 years ago by each

Please review the attached diff. This addresses the problem by skipping the CNAME synthesis when it would result in an illegal name.

comment:3 follow-up: Changed 9 years ago by each

  • Owner changed from each to UnAssigned
  • Status changed from new to reviewing

comment:4 in reply to: ↑ 3 ; follow-up: Changed 9 years ago by jinmei

Replying to each:

As noted in the ticket, BIND 9 returns an YXDOMAIN rcode.

				/*
				 * RFC2672, section 4.1, subsection 3c says
				 * we should return YXDOMAIN if the constructed
				 * name would be too long.
				 */
				client->message->rcode = dns_rcode_yxdomain;

If the RFC says this, we should follow it.

I'd also add another test on the boundary condition: the case where the synthesized name has the possible maximum length.

And please add a comment about the condition:

    if (prefix.getLength() + dname_target.getLength() - 1 > Name::MAX_WIRE) {

because why we need "- 1" may not be so obvious.

comment:5 Changed 9 years ago by jinmei

  • Owner changed from UnAssigned to each

oh, btw, technically, this may not be in the next release because it missed the review request deadline.

but I'm personally okay with including it because it's a quite small change.

comment:6 in reply to: ↑ 4 Changed 9 years ago by each

As noted in the ticket, BIND 9 returns an YXDOMAIN rcode.

Whoops, thanks, I missed this.

Unfortunately I see now that it requires a significantly bigger change to do it right. There's currently no way for synthesizeCname() (which is called by hasDelegation()) to terminate query processing, other than an exception.

I hadn't actually intended for this to be in the next release anyway, though.

comment:7 Changed 9 years ago by shane

  • billable set to 0
  • Estimated Difficulty set to 0.0
  • Internal? unset
  • Milestone set to feature backlog item
  • Owner changed from each to shane
  • Status changed from reviewing to accepted

This is not actually in review, but seems to require code. Moving out of reviewing status and putting in backlog.

comment:8 Changed 9 years ago by shane

  • Owner changed from shane to UnAssigned
  • Status changed from accepted to assigned

comment:9 follow-up: Changed 9 years ago by jreed

This is marked as critical. If I am reading the description correctly, b10-auth will not crash but just SERVFAIL?

Can someone please provide a test example for this?

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

Replying to jreed:

This is marked as critical. If I am reading the description correctly, b10-auth will not crash but just SERVFAIL?

Not tested right now (should be easy), but that's my understanding.

Can someone please provide a test example for this?

See DataSrcTest::DISABLED_synthesizedCnameTooLong in
src/lib/datasrc/tests/datasrc_unittest.cc.

comment:11 Changed 9 years ago by jreed

I tested using the b10-resolver in default interative mode. I set up two labels elsewhere that are too long and one that is shorter that works.

Yes, it does return SERVFAIL. The authoritative server sent back the DNAME (but no CNAME) for when the synthesized DNAME is too long.

But it also returns a SERVFAIL for shorter DNAME that synthesized CNAME is short enough. The authoritative server sent back a DNAME and a CNAME (I also watch with tcpdump). But the b10-resolver only returned a SERVFAIL. I doesn't seem to know how to handle that response. Should this be a different ticket?

comment:12 follow-ups: Changed 9 years ago by jreed

b10-resolver acting as a forwarder also has a problem: if the synthesized result is too long, upstream BIND 9 resolver would return a SERVFAIL. But the b10-resolver returned NOERROR (no answers). Maybe I should open a new ticket for this also.

If the synthesized CNAME is not too long (it is okay), b10-resolver (still doing forwarding), sets status to NXDOMAIN but provides the DNAME and the synthesized CNAME in the ANSWER section. I think the NXDOMAIN is accurate in this case, since the target inm my tests actually does not exist. (Even BIND 9 gives same results for this scenario.)

comment:13 in reply to: ↑ 12 Changed 9 years ago by jreed

Replying to jreed:

If the synthesized CNAME is not too long (it is okay), b10-resolver (still doing forwarding), sets status to NXDOMAIN but provides the DNAME and the synthesized CNAME in the ANSWER section. I think the NXDOMAIN is accurate in this case, since the target inm my tests actually does not exist. (Even BIND 9 gives same results for this scenario.)

I confirmed the NXDOMAIN. I made a new test with a synthesized CNAME pointed to a label that did exist and it sent back the DNAME, CNAME, and the A in the NOERROR answer as expected. b10-resolver acting as a forwarder is correct. b10-resolver acting as an interator is broken still (SERVFAIL and no results). For this particular test, the auth server also hosted the target zone, so it returns the A record too (I verified with tcpdump).

comment:14 Changed 9 years ago by shane

  • Priority changed from critical to major

comment:15 in reply to: ↑ 12 Changed 9 years ago by jreed

Replying to jreed:

b10-resolver acting as a forwarder also has a problem: if the synthesized result is too long, upstream BIND 9 resolver would return a SERVFAIL. But the b10-resolver returned NOERROR (no answers). Maybe I should open a new ticket for this also.

I opened #685 for that.

comment:16 Changed 9 years ago by stephen

  • Milestone feature backlog item deleted

Milestone feature backlog item deleted

comment:17 Changed 8 years ago by shane

  • Defect Severity set to N/A
  • Owner changed from UnAssigned to jreed
  • Sub-Project set to DNS

The original ticket was about a problem in the authoritative server, and this one seems to be about incorrect resolver behavior. Jeremy, if this is true then probably this needs to be extracted into a separate ticket and the one about DNAME-CNAME synthesis resolved. Please advise.

comment:18 Changed 8 years ago by jreed

  • Milestone set to New Tasks
  • Owner changed from jreed to UnAssigned

The resolver ticket exists. #685

But this #145 does not seem to be done. (I did not test.) No changelog entry for it and the test is still disabled: DISABLED_synthesizedCnameTooLong

comment:19 Changed 8 years ago by shane

  • Milestone New Tasks deleted

comment:20 Changed 7 years ago by jinmei

  • Resolution set to wontfix
  • Status changed from assigned to closed

comment:21 Changed 7 years ago by jinmei

  • Resolution wontfix deleted
  • Status changed from closed to reopened

comment:22 Changed 7 years ago by jinmei

we should be able to close this ticket. not exist in the new data source API/impl.

comment:23 Changed 7 years ago by jinmei

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