From e9f3bff9e90e794bbe240357a216027e365ba9cf Mon Sep 17 00:00:00 2001 From: Marc van der Wal Date: Mon, 20 Nov 2023 10:23:45 +0100 Subject: [PATCH 1/2] Make Zonemaster::LDNS::RR::NSEC3::salt work MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The salt() method in the Zonemaster::LDNS::RR::NSEC3 module never worked and often caused the Perl interpreter to crash. This commit fixes many long-standing issues with the affected code. Firstly, the root cause of the crash is a double free resulting from the inappropriate use of ldns_rdf_deep_free() in the code. The ldns_nsec3_salt() function returns a pointer to a ldns_rdf structure which is just a window into the salt field, not a copy of the data. So calling ldns_rdf_deep_free() on that ldns_rdf object causes a part of the original resource record structure to be freed instead. This then results in a double free when the memory for the resource record object is deallocated. Calling ldns_rdf_free() instead fixes the crashing. Secondly, the method doesn’t quite return the salt: it actually returns a string containing the salt preceded by its length byte. This is surprising, not as documented and unlikely to be useful. This problem is fixed by rewriting the entire function so as to return the salt, all the salt and nothing but the salt. Thirdly, the method was also insufficiently covered by unit tests. Tests were added, first to help reproduce the crashes, but also to cover the case of an NSEC3 with non-empty salt. Finally, the method returns undef if the salt is empty. Not only is that documented nowhere, but the choice of doing so is questionable. This commit changes the behavior somewhat in this case: if the salt is empty, an empty string is returned instead; the method only returns undef if there was a problem accessing the salt field. --- lib/Zonemaster/LDNS/RR/NSEC3.pm | 2 +- src/LDNS.xs | 15 +++++++++------ t/rr.t | 19 +++++++++++++++++-- 3 files changed, 27 insertions(+), 9 deletions(-) diff --git a/lib/Zonemaster/LDNS/RR/NSEC3.pm b/lib/Zonemaster/LDNS/RR/NSEC3.pm index 455c497a..5591996a 100644 --- a/lib/Zonemaster/LDNS/RR/NSEC3.pm +++ b/lib/Zonemaster/LDNS/RR/NSEC3.pm @@ -37,7 +37,7 @@ Returns the iteration count. =item salt() -Returns the cryptographic salt, in binary form. +Returns the contents of the salt field as a binary string, if non-empty; otherwise, returns an empty string. If there was a problem accessing the salt field, returns undef. =item next_owner() diff --git a/src/LDNS.xs b/src/LDNS.xs index 9397f145..ab5a34ef 100644 --- a/src/LDNS.xs +++ b/src/LDNS.xs @@ -2275,13 +2275,16 @@ rr_nsec3_iterations(obj) SV * rr_nsec3_salt(obj) Zonemaster::LDNS::RR::NSEC3 obj; - PPCODE: - if(ldns_nsec3_salt_length(obj) > 0) - { - ldns_rdf *buf = ldns_nsec3_salt(obj); - ST(0) = sv_2mortal(newSVpvn((char *)ldns_rdf_data(buf), ldns_rdf_size(buf))); - ldns_rdf_deep_free(buf); + CODE: + { + uint8_t *salt = ldns_nsec3_salt_data(obj); + if (salt) { + RETVAL = newSVpvn((char *)salt, ldns_nsec3_salt_length(obj)); + LDNS_FREE(salt); } + } + OUTPUT: + RETVAL SV * rr_nsec3_next_owner(obj) diff --git a/t/rr.t b/t/rr.t index 2bda75d2..673a5dc9 100644 --- a/t/rr.t +++ b/t/rr.t @@ -203,7 +203,7 @@ subtest 'DS' => sub { } }; -subtest 'NSEC3' => sub { +subtest 'NSEC3 without salt' => sub { my $nsec3 = Zonemaster::LDNS::RR->new_from_string( 'VD0J8N54V788IUBJL9CN5MUD416BS5I6.com. 86400 IN NSEC3 1 1 0 - VD0N3HDL5MG940MOUBCF5MNLKGDT9RFT NS DS RRSIG' ); isa_ok( $nsec3, 'Zonemaster::LDNS::RR::NSEC3' ); @@ -211,13 +211,28 @@ subtest 'NSEC3' => sub { is( $nsec3->flags, 1 ); ok( $nsec3->optout ); is( $nsec3->iterations, 0 ); - is( $nsec3->salt, undef ); + is( $nsec3->salt, '' ); is( encode_base64( $nsec3->next_owner ), "FPtBccW1LaCSAtjy2PLa9aQb1O39\n" ); is( $nsec3->typelist, 'NS DS RRSIG ' ); is_deeply( [ sort keys %{ $nsec3->typehref } ], [qw(DS NS RRSIG)] ); }; +subtest 'NSEC3 with salt' => sub { + my $nsec3 = Zonemaster::LDNS::RR->new_from_string( + 'BP7OICBR09FICEULBF46U8DMJ1J1V8R3.bad-values.dnssec03.xa. 900 IN NSEC3 2 1 1 8104 c91qe244nd0q5qh3jln35a809mik8d39 A NS SOA MX TXT RRSIG DNSKEY NSEC3PARAM' ); + isa_ok( $nsec3, 'Zonemaster::LDNS::RR::NSEC3' ); + is( $nsec3->algorithm, 2 ); + is( $nsec3->flags, 1 ); + ok( $nsec3->optout ); + is( $nsec3->iterations, 1 ); + is( unpack('H*', $nsec3->salt), '8104' ); + is( encode_base64( $nsec3->next_owner ), "FGJDpwiEu0Gi6iOdbjKpAE2lRDRp\n" ); + is( $nsec3->typelist, 'A NS SOA MX TXT RRSIG DNSKEY NSEC3PARAM ' ); + + is_deeply( [ sort keys %{ $nsec3->typehref } ], [qw(A DNSKEY MX NS NSEC3PARAM RRSIG SOA TXT)] ); +}; + subtest 'NSEC3PARAM' => sub { my $nsec3param = Zonemaster::LDNS::RR->new_from_string( 'whitehouse.gov. 3600 IN NSEC3PARAM 1 0 1 B2C19AB526819347' ); isa_ok( $nsec3param, 'Zonemaster::LDNS::RR::NSEC3PARAM' ); From b8f44fe73eb40eff7c70c47260e56898fbf8a4f8 Mon Sep 17 00:00:00 2001 From: Marc van der Wal Date: Mon, 20 Nov 2023 13:49:32 +0100 Subject: [PATCH 2/2] Remove newlines from base64-encoded data --- t/rr.t | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/t/rr.t b/t/rr.t index 673a5dc9..929ebce5 100644 --- a/t/rr.t +++ b/t/rr.t @@ -212,7 +212,7 @@ subtest 'NSEC3 without salt' => sub { ok( $nsec3->optout ); is( $nsec3->iterations, 0 ); is( $nsec3->salt, '' ); - is( encode_base64( $nsec3->next_owner ), "FPtBccW1LaCSAtjy2PLa9aQb1O39\n" ); + is( encode_base64( $nsec3->next_owner, '' ), "FPtBccW1LaCSAtjy2PLa9aQb1O39" ); is( $nsec3->typelist, 'NS DS RRSIG ' ); is_deeply( [ sort keys %{ $nsec3->typehref } ], [qw(DS NS RRSIG)] ); @@ -227,7 +227,7 @@ subtest 'NSEC3 with salt' => sub { ok( $nsec3->optout ); is( $nsec3->iterations, 1 ); is( unpack('H*', $nsec3->salt), '8104' ); - is( encode_base64( $nsec3->next_owner ), "FGJDpwiEu0Gi6iOdbjKpAE2lRDRp\n" ); + is( encode_base64( $nsec3->next_owner, '' ), "FGJDpwiEu0Gi6iOdbjKpAE2lRDRp" ); is( $nsec3->typelist, 'A NS SOA MX TXT RRSIG DNSKEY NSEC3PARAM ' ); is_deeply( [ sort keys %{ $nsec3->typehref } ], [qw(A DNSKEY MX NS NSEC3PARAM RRSIG SOA TXT)] ); @@ -239,7 +239,7 @@ subtest 'NSEC3PARAM' => sub { is( $nsec3param->algorithm, 1 ); is( $nsec3param->flags, 0 ); is( $nsec3param->iterations, 1, "Iterations" ); - is( encode_base64( $nsec3param->salt ), "CLLBmrUmgZNH\n", "Salt" ); + is( encode_base64( $nsec3param->salt, '' ), "CLLBmrUmgZNH", "Salt" ); is( lc($nsec3param->owner), 'whitehouse.gov.' ); };