Opened 8 years ago

Closed 8 years ago

#2027 closed enhancement (complete)

ddns: move acl up

Reported by: jelte Owned by: jelte
Priority: medium Milestone: Sprint-20120619
Component: ddns Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 3 Add Hours to Ticket: 0
Total Hours: 2.6 Internal?: no

Description (last modified by jinmei)

as (being) discussed on bind10-dev; we currently have defaulted to using the order of operations defined in RFC2136, but it makes sense to move the ACL check to before the prerequisite checks.

This should be a very simple change, the code is already separated, it *should* be a one-line change plus a few tests.

(don't forget mentioning this in the doc, both as code comments and in
the guide)

(and don't forget adding a test to confirm the behavior, i.e, try to
handle a request that would be rejected due to prerequisite check
if ACLs were checked after that).

Subtickets

Change History (13)

comment:1 Changed 8 years ago by jinmei

  • Description modified (diff)

comment:2 Changed 8 years ago by jinmei

  • Description modified (diff)

comment:3 Changed 8 years ago by jelte

  • Milestone changed from New Tasks to Sprint-20120619

comment:4 Changed 8 years ago by jelte

  • Owner set to jelte
  • Status changed from new to assigned

comment:5 Changed 8 years ago by jelte

  • Owner changed from jelte to UnAssigned
  • Status changed from assigned to reviewing

Ready for review, should be pretty straightforward.

comment:6 Changed 8 years ago by jinmei

  • Owner changed from UnAssigned to jinmei

comment:7 Changed 8 years ago by jinmei

The code and test look okay.

But I have comments on the rationale comment:

            # Contrary to what RFC2136 specifies, we do ACL checks before
            # prerequisites. Following the spec, information could leak,
            # and we decided not to do so (as do other implementations)

First, "as do other implementations" is a bit ambiguous to me. Does
this mean "other implementations do follow the check but we decided
not to do so", or "other implementations do not follow the check and
we decided not to do so either"? I guess the intent is the former,
and maybe grammar-wise that can be the only valid interpretation, but
at least to me it was not super clear.

Secondly, in my understanding the information leak is not the only
reason why the RFC2136 ordering is bad. My understanding is that it
primarily simply doesn't make sense in that ACL should generally be
checked as soon as possible, and in practice we have had other
security issues (in BIND 9) that could have been more easily worked
around if the ACL check could take place before prerequisite check.

And, as an related point to the above two points, BIND 9 actually
prevents information leak by checking the general "query ACL" before
prerequisite check (but it still checks the update ACL after
prerequisite handling). So, if this comment means "other
implementations follow the spec but we won't because it would leak
information", it doesn't make perfect sense because "other
implementations actually wouldn't be considered to follow the spec if
it's mainly about information leak (btw what are "other
implementations"? Obviously it should include BIND 9, but are there
others? I think Nominum's auth server supports ddns and I guess it
actually follows the spec on this point, but I'm not sure).

So, if we want to be super precise (at least in terms of my
understanding), why we reverse the order is:

  • there's a general consensus that what RFC literally says doesn't make sense
  • doing the ACL check after prerequisite can cause various troubles in practice, such as information leak or making the defense more difficult if and when a vulnerability is found in the prerequisite check.
  • while BIND 9 prevents the information leak by introducing two sets of ACLs, it still has other problems, and so just avoiding leak this way wouldn't be worth the complexity of maintaining and performing two sets of ACLs.

The code comment won't have to be in this detail, but if I were to
write it, I'd say something like:

            # Contrary to what RFC2136 specifies, we do ACL checks before
            # prerequisites. It's now generally considered to be a bad
            # idea, and actually does harm such as information
            # leak. It should make more sense to prevent any security issues
            # by performing ACL check as early as possible.

(this example comment doesn't mention "other implementations". It's
not necessarily because I don't think we have to, but because I was
not sure how we mention it).

comment:8 Changed 8 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:9 follow-up: Changed 8 years ago by jelte

  • Owner changed from jelte to jinmei

ok, thanks, copied your rationale comment verbatim

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

Replying to jelte:

ok, thanks, copied your rationale comment verbatim

okay, then please feel free to merge.

comment:11 Changed 8 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:12 Changed 8 years ago by jinmei

  • Total Hours changed from 0 to 0.6

comment:13 Changed 8 years ago by jelte

  • Resolution set to complete
  • Status changed from reviewing to closed
  • Total Hours changed from 0.6 to 2.6

thanks, merged, closing ticket

Note: See TracTickets for help on using tickets.