Opened 8 years ago

Closed 8 years ago

#1638 closed defect (fixed)

NSEC3PARAM rdata should have more tests

Reported by: jinmei Owned by: jinmei
Priority: medium Milestone: Sprint-20120221
Component: libdns++ Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 6 Add Hours to Ticket: 0
Total Hours: 6.25 Internal?: no

Description

I've found one bug in the NSEC3PARAM rdata implementation while I
worked on #1575 (see the ticket message), and also noticed the amount
and quality of tests were quite poor (I suspect we'll find some other
subtle bugs if we think about more detailed tests).

Since we are now working on NSEC3, it's probably a good time to
improve the test (and implementation) quality of this RDATA.

The test should at least cover:

  • more various types of bogus text data
  • bad wire format data
  • empty salt
  • maybe also very large salt
  • unusually large number of iterations

Subtickets

Change History (12)

comment:1 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 6

comment:2 Changed 8 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20120221

comment:3 Changed 8 years ago by jinmei

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

comment:4 Changed 8 years ago by jinmei

trac1638 is ready for review.

The diff is large, but many parts of it should be quite
straightforward refactoring:

  • The first commit (a2bc586) is to convert some of NSEC3 tests to TYPED_TESTs so they can be shared with NSEC3PARAM. The diff of this commit is large, but should be straightforward.
  • 95b8b13 to 6292462 (inclusive) are the main part of the branch. They fix a number of bugs in the NSEC3PARAM implementation so that it will pass the shared tests. Some new tests were added.
  • bf67156 is an independent (and not directly related) bug fix to NSEC3 I happened to notice during the work.
  • 001717c is to unify common (and reasonably complicated) code logic for NSEC3 and NSEC3PARAM into a single (essentially private) helper module. The diff of this commit is large, but essentially it should be straightforward.

One final note: distcheck fails due to unrelated bug I pushed to
master with #1641. Cherry-picking 8c5e01a will solve it (I confirmed
distcheck passed for this branch with it), but I didn't merge it to
the branch to avoid confusion.

This is the proposed changelog entry:

378.?	[bug]		jinmei
	libdns++: miscellaneous bug fixes for the NSECPARAM RDATA
	implementation, including incorrect handling for empty salt and
	incorrect comparison logic.
	(Trac #1638, git TBD)

comment:5 Changed 8 years ago by jinmei

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

comment:6 Changed 8 years ago by jelte

  • Owner changed from UnAssigned to jelte

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

  • Owner changed from jelte to jinmei

Looks good, two minor comments;

nsec3param_common.cc:

If i read parseNSEC3ParamText() correctly, and salt is too long, it is still put into the passed vector. Perhaps not much of a problem (since this is in essence a private function and we control the vector), but technically not exception-safe. Perhaps do the length check on salthex instead of the result vector (whitespace is not allowed anyway)? (or, alternatively, mention this explicitely in the doxygen)

nsec3_50.cc:

trivial inline comment suggestion: change 'It must not be a padded base32hex string.' to 'It must be an unpadded base32hex string" (current text might be interpreted as 'must not be base32')

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

Replying to jelte:

Thanks for the prompt review.

Looks good, two minor comments;

nsec3param_common.cc:

If i read parseNSEC3ParamText() correctly, and salt is too long, it is still put into the passed vector. Perhaps not much of a problem (since this is in essence a private function and we control the vector), but technically not exception-safe. Perhaps do the length check on salthex instead of the result vector (whitespace is not allowed anyway)? (or, alternatively, mention this explicitely in the doxygen)

Hmm, good point. In terms of exception safety the original code was
actually "safe", in that it didn't cause any resource leak on
exception (it still didn't provide the strong guarantee, but that is
not specific to this error case), but being proactive would be
generally good. I checked the BIND 9 implementation and found it
checked too long salt before decoding it, too. So I changed it.

BTW: in NSEC3 we check too long hash after decoding. BIND 9 also does
that check after decoding. That's probably because it's not so
obvious to calculate the possible maximum base32 size (space isn't
allowed for hash either so in theory it shouldn't be so different -
but the result would be less readable compared to the case of HEX).

nsec3_50.cc:

trivial inline comment suggestion: change 'It must not be a padded base32hex string.' to 'It must be an unpadded base32hex string" (current text might be interpreted as 'must not be base32')

Okay, changed.

Giving the ticket back to you just in case.

comment:9 Changed 8 years ago by jinmei

  • Owner changed from jinmei to jelte

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

  • Owner changed from jelte to jinmei

check, please merge :)

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

Replying to jelte:

check, please merge :)

Thanks, merged, closing.

comment:12 Changed 8 years ago by jinmei

  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 0 to 6.25
Note: See TracTickets for help on using tickets.