Opened 10 years ago

Closed 9 years ago

Last modified 9 years ago

#117 closed defect (fixed)

NSEC3 RDATA needs more tests and has serious bugs

Reported by: jinmei Owned by: jinmei
Priority: high Milestone: A-Team-Sprint-20110223
Component: libdns++ Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 5.0 Add Hours to Ticket: 0
Total Hours: 5.0 Internal?: no

Description

See ticket #116. From a quick look at the code the NSEC3 implementation has the same bugs.

I'm assigning it to Evan, who first wrote the code.

Subtickets

Change History (22)

comment:1 Changed 50 years ago by jinmei

  • Add Hours to Ticket changed from 0.0 to 5.0
  • Total Hours changed from 0.0 to 5.0

comment:1 Changed 9 years ago by shane

  • Owner changed from each to jinmei
  • Status changed from new to assigned

May be easy to fix since you did this for NSEC.

comment:2 Changed 9 years ago by shane

  • Component changed from Unclassified to DNSPacket API
  • Milestone changed from 03. 1st Incremental Release to feature backlog item

Moving to backlog.

comment:3 Changed 9 years ago by larissas

  • billable set to 0
  • Estimated Difficulty set to 0.0
  • Internal? unset
  • Owner changed from jinmei to UnAssigned

I'm setting this unassigned but leaving everything else as is.

comment:4 Changed 9 years ago by stephen

  • Milestone changed from feature backlog item to A-Team-Sprint-20110223

comment:5 Changed 9 years ago by jinmei

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

comment:6 Changed 9 years ago by jinmei

  • Add Hours to Ticket set to 5.0

comment:7 Changed 9 years ago by jinmei

my gut-feeling size of this task is 5.

comment:8 Changed 9 years ago by jinmei

Branch trac117 is ready for review.

This branch contains several bug fixes:

  • reject invalid type bitmaps
  • reject other invalid parameters (too large salt/hash, etc), both "from wire" and "from text" cases

For the bitmap fix, I've unitified the check code for both NSEC and
NSEC3, and extracts it as a single public (but effectively private)
function: checkRRTypeBitmaps(). It's essentially a bare copy of the
check logic of the NSEC class, in which sense it only has to be
reviewed lightly (for reducing review load; intensive review is always
welcome nevertheless).

I thought we should share more code for NSEC and NSEC3 this way, but
as this branch has been getting big, I'll stop here and leave further
changes to a separate ticket.

This is the proposed changelog entry:

  167.?	[bug]		jinmei
	Tightened validity checks in the NSEC3 constructors, both "from
	"text" and "from wire".  Specifically, wire data containing
	invalid type bitmaps or invalid lengths of salt or hash is now
	correctly rejected.
	(Trac #117, git TBD)

comment:9 Changed 9 years ago by jinmei

  • Owner changed from jinmei to UnAssigned
  • Status changed from accepted to reviewing

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

Hmm, this version seems to have its own bug. I'll give the ticket back for now.

comment:11 Changed 9 years ago by jinmei

  • Owner changed from UnAssigned to jinmei

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

Replying to jinmei:

Hmm, this version seems to have its own bug. I'll give the ticket back for now.

This has been fixed, and the branch is now really ready for review.

comment:13 Changed 9 years ago by jinmei

  • Owner changed from jinmei to UnAssigned

comment:14 Changed 9 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:15 follow-up: Changed 9 years ago by vorner

  • Owner changed from vorner to jinmei

I had little problems with compiling it (missing boost:: in front of lexical_cast) and there were some problems with make distcheck (the nsec_bitmap.* can't be nodist, that would mean they are not included in the distribution tarball). I fixed both of these, could you have a look if it is OK?

With the code itself, it is OK. I have just two minor points:

This is in the nsec_bitmap.cc:

for (int i = 0; i < total_len; i += len) 

I do understand it and I like compressed code, but I noticed there's inclination to more wordy ways. As the len changes each iteration and the i moves by another 2 inside the body, use of for might be slightly misleading (it is usually used when the only one changing the i is the for cycle itself and with fixed length). While cycle might be more intuitive for that, but I personally don't mind this either.

As you note the NSEC3 tests are the same as with NSEC at one place ‒ would it be possible to have them unified to have the code only once, let's say in some kind of template manner?

Anyway, if you like this is the way you want it to be, the code is good enough to be merged.

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

Replying to vorner:

I had little problems with compiling it (missing boost:: in front of lexical_cast) and there were some problems with make distcheck (the nsec_bitmap.* can't be nodist, that would mean they are not included in the distribution tarball). I fixed both of these, could you have a look if it is OK?

It looks okay, thanks for fixing it.

With the code itself, it is OK. I have just two minor points:

This is in the nsec_bitmap.cc:

for (int i = 0; i < total_len; i += len) 

I do understand it and I like compressed code, but I noticed there's inclination to more wordy ways. As the len changes each iteration and the i moves by another 2 inside the body, use of for might be slightly misleading (it is usually used when the only one changing the i is the for cycle itself and with fixed length). While cycle might be more intuitive for that, but I personally don't mind this either.

Changed in 73b2eae. I'm not sure if the change from for to while
improved the clarity of the code, but I agree using two mutable
parameters for a single for loop is more error prone. As a bonus side
effect I can change 'len' to const, which is a clear win. I also
noticed 'block' can also be const so I did it.

As you note the NSEC3 tests are the same as with NSEC at one place ‒ would it be possible to have them unified to have the code only once, let's say in some kind of template manner?

I actually thought about that possibility, but didn't do it in the
first push. We still need to prepare test data separately, which have
different file names, so I'm still not sure if it's an easy and
reasonable way to share the same test code for both cases. But I
agree it would be better to centralize the two cases, so I created a
separate test case in a separate .cc file, and moved bitmap related
tests there. If we add more test cases and find the duplicate too
noisy in future, we can then consider sharing the code. This change
is aa32fd5.

I also made one minor change: d0a29d0

comment:17 Changed 9 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:18 follow-up: Changed 9 years ago by vorner

  • Owner changed from vorner to jinmei

I think it can be merged.

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

Replying to vorner:

I think it can be merged.

Thanks, merged, closing ticket.

comment:20 Changed 9 years ago by jinmei

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

comment:21 Changed 9 years ago by jinmei

  • Estimated Difficulty changed from 0.0 to 5.0
Note: See TracTickets for help on using tickets.