Opened 8 years ago

Closed 8 years ago

#1641 closed defect (fixed)

Further bug(s) in NSEC3 RDATA implementation

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: NSEC3
Estimated Difficulty: 5 Add Hours to Ticket: 0
Total Hours: 12 Internal?: no

Description

Its toText() crashes for this input:

1 1 1 5CA1AB1E QBREATAE625G9UGNH0BOAAS79IT1LTPE NS SOA RRSIG DNSKEY NSEC3PARAM TYPE65534

From a quick look it's due to difference on this part between NSEC and
NSEC3 implementations:

        // NSEC
        assert(len > 0 && len <= 32);

        // NSEC3
        assert(len >= 0 && len < 32);

but we should not just fix this specific case, but try to unify the
bitmap related processing between these two classes more (that's
partially done), through which we may find other remaining bugs.
We'll also need more tests.

Subtickets

Change History (12)

comment:1 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 5

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

trac1641 is ready for review.

As I expected I've found a number of other bugs (though most of them
are minor). I fixed all of them in the ticket.

The diff may look big, but important parts are changes up to 5c49671
(inclusive). They generally consist of more tests in
rdata_nsecbitmap_unittest.cc and misc. bug fixes in the NSEC3
implementation. nsecbitmap_unittest was also heavily modified so we
use TYPED_TEST to share essentially same tests for NSEC and NSEC3, but
otherwise it should be quite straightforward.

f10604e and d9640f6 are after-work cleanup: unified the same code
logic for handling NSEC/NSEC3 bitmaps. So, even if the amount of
change may be big, it should be essentially trivial.

This is the proposed changelog entry:

377.?	[bug]		jinmei
	libdns++: miscellaneous bug fixes for the NSEC and NSEC3 RDATA
	implementation, including a crash in NSEC3::toText() for some RR
	types, incorrect handling of empty NSEC3 salt, and incorrect
	comparison logic in NSEC3::compare().
	(Trac #1641, 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 stephen

  • Owner changed from UnAssigned to stephen

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

  • Owner changed from stephen to jinmei

Reviewed commit 78785509428db0da450a2be7e89a342d1200cf0a

The branch failed to build on Ubuntu 10.10 (gcc 4.4.5) because "memset" was not defined in src/lib/dns/rdata/generic/detail/nsec_bitmap.cc. I've included the header file (string.h) and pushed the change (commit 9e3c7d5).

src/lib/dns/rdata/generic/detail/nsec_bitmap.cc
buildBitmapsFromText: in the search for the a non-empty window, inverting the sense of the test for "octet" after the loop avoids the need for a "continue".

bitmapsToText: "continue"s can be avoided by inverting the sense of the tests.

src/lib/dns/rdata/generic/nsec3_50.cc
compareVectors: Needs a header. (The content is obvious but it took me a little time to work out that your would sort short vectors first - regardless of content - when comparing numbers.)

compareVectors: when doing the length check, why not just return "len1 - len2" as is done later in the function?

compareVectors: The "if (cmp != 0)" statement is not needed:

return(cmplen == 0 ? (len1 - len2) : memcmp(...));

src/lib/dns/tests/rdata_nsecbitmap_unittest.cc
The comment "Make sure all possible bits in a one-octet bitmap correctly." does not parse.

ChangeLog entry
This is OK

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

Replying to stephen:

Thanks for the review.

The branch failed to build on Ubuntu 10.10 (gcc 4.4.5) because "memset" was not defined in src/lib/dns/rdata/generic/detail/nsec_bitmap.cc. I've included the header file (string.h) and pushed the change (commit 9e3c7d5).

Ah, okay, thanks. I've changed it to <cstring> as it's probably a bit
more C++-ish.

src/lib/dns/rdata/generic/detail/nsec_bitmap.cc
buildBitmapsFromText: in the search for the a non-empty window, inverting the sense of the test for "octet" after the loop avoids the need for a "continue".

bitmapsToText: "continue"s can be avoided by inverting the sense of the tests.

Do you mean

        if (octet >= 0) {
            typebits.push_back(window);
            typebits.push_back(octet + 1);
            for (int i = 0; i <= octet; ++i) {
                typebits.push_back(bitmap[window * 32 + i]);
            }
        }

instead of this (and do the similar thing for bitmapsToText)?

        if (octet < 0) {
            continue;
        }
        typebits.push_back(window);
        ...

If so, hmm, I don't mind changing them as suggested, but for now I've
not changed these parts. On one hand, I generally prefer shorter
code, and in that sense the suggest version is better. On the other
hand, especially when the "octet >= 0" case is long, I'd rather make
it clearer that the simpler case is completed at an earlier part of
code (otherwise, if I were first reading/reviewing the code I'd have
to push the case of octet < 0 case in my brain stack while I'm reading
the if block). In these particular cases, it's not clear which point
is more important: The code is quite short and comprehensive in either
way, and the total number of lines are not different in either way.

Also, this implementation is a quite straightforward port of BIND 9's
implementation, and these "continue"s are derived from BIND 9, too.
Considering the two styles are not so different in terms of
readability (not at least to me), I think it makes sense to keep the
organization as compatible as BIND 9 (e.g. so if we happen to find a
bug in one version it'll be easier to apply the same fix to the
other).

In any case, these are not a strong opinion. If you strongly feel the
suggested version is better, I'll change that.

src/lib/dns/rdata/generic/nsec3_50.cc
compareVectors: Needs a header. (The content is obvious but it took me a little time to work out that your would sort short vectors first - regardless of content - when comparing numbers.)

Do you mean descriptive comments by "a header"? I added some.

compareVectors: when doing the length check, why not just return "len1 - len2" as is done later in the function?

Good point, changed.

compareVectors: The "if (cmp != 0)" statement is not needed:

return(cmplen == 0 ? (len1 - len2) : memcmp(...));

If you mean replacing:

    const int cmp = cmplen == 0 ? 0 : memcmp(&v1.at(0), &v2.at(0), cmplen);
    if (cmp != 0) {
        return (cmp);
    } else {
        return (len1 - len2);
    }

with

    return (cmplen == 0 ? (len1 - len2) : memcmp(&v1.at(0), &v2.at(0), cmplen));

then we cannot do this because it will incorrectly return 0 if cmplen

0, len1 != len2 and memcmp returns 0 (and check_length_first is

false).

Or did you mean something else?

comment:9 Changed 8 years ago by jinmei

  • Owner changed from jinmei to stephen

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

  • Owner changed from stephen to jinmei

Do you mean descriptive comments by "a header"? I added some.

Yes and it is what I was after, thanks.

bitmapsToText: "continue"s can be avoided by inverting the sense of the tests.

:

That is what I meant and admittedly it is a bit subjective.

Also, this implementation is a quite straightforward port of BIND 9's implementation, and these "continue"s are derived from BIND 9, too.

In that case keep it as is.

If you mean replacing:
:
then we cannot do this because it will incorrectly return 0 if cmplen...

Doh! Missed the case where memcmp() returns 0.

All OK, please merge.

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

Replying to stephen:

All OK, please merge.

Thanks, merge done, 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 12
Note: See TracTickets for help on using tickets.