Merge pull request #181 from zonemaster/develop

matsduf · web-flow · commit d302f8c37023 · 2024-03-18T17:39:08.000+01:00
Merge develop into master (Zonemaster-LDNS)
diff --git a/.travis.yml b/.travis.yml
@@ -5,10 +5,9 @@ env:
 
 language: perl
 perl:
-    - "5.32"
+    - "5.36"
     - "5.30.2"
     - "5.26"
-    - "5.14.4"
 
 before_install:
       # quoting preserves newlines in the script and then avoid error if the
diff --git a/Changes b/Changes
@@ -1,5 +1,19 @@
 Release history for Zonemaster component Zonemaster-LDNS
 
+4.0.0 2024-03-18 (public release version)
+
+ [Breaking change]
+ - Fix crashing induced by Zonemaster::LDNS::RR::NSEC3::salt()
+   method (#177)
+ - Fix other methods in Zonemaster::LDNS::RR::NSEC3 and
+   ::NSEC3PARAM (#178)
+
+ [Fixes]
+ - Ignore incomplete RRs (#136)
+ - Validate inputs to Zonemaster::LDNS::RR::NSEC3::covers()
+   (#176)
+
+
 3.2.0 2023-06-21 (public fix version)
  [Feature]
  - Expand DNAME support (#170)
diff --git a/Makefile.PL b/Makefile.PL
@@ -124,6 +124,7 @@ configure_requires 'Module::Install::XSUtil';
 test_requires 'JSON::PP';
 test_requires 'Test::Fatal';
 test_requires 'Test::More' => 1.302015;
+test_requires 'MIME::Base32';
 
 use_ppport 3.19;
 cc_include_paths 'include';
diff --git a/README.md b/README.md
@@ -59,6 +59,7 @@ Compile-time dependencies (only when installing from source):
  * `automake` (if [Internal ldns] is enabled)
 
 Test-time dependencies:
+ * `MIME::Base32`
  * `Test::Fatal`
 
 There is a small part in the code that may not be compatible with non-Unix
diff --git a/lib/Zonemaster/LDNS.pm b/lib/Zonemaster/LDNS.pm
@@ -2,7 +2,7 @@ package Zonemaster::LDNS;
 
 use 5.014;
 
-our $VERSION = '3.2.0';
+our $VERSION = '4.0.0';
 
 use parent 'Exporter';
 our @EXPORT_OK = qw[to_idn has_idn ldns_version load_zonefile];
diff --git a/lib/Zonemaster/LDNS/Packet.pm b/lib/Zonemaster/LDNS/Packet.pm
@@ -32,7 +32,9 @@ sub answer {
     my @records = $self->answer_unfiltered;
 
     for ( my $i = $#records ; $i >= 0 ; --$i ) {
-        if ( $records[$i]->type() eq 'DNSKEY' && $records[$i]->keysize() == -1 ) {
+        if ( !$records[$i]->check_rd_count()
+            || ( $records[$i]->type() eq 'DNSKEY' && $records[$i]->keysize() == -1 ) )
+        {
             splice @records, $i, 1;
         }
     }
@@ -45,6 +47,12 @@ sub authority {
 
     my @records = $self->authority_unfiltered;
 
+    for ( my $i = $#records ; $i >= 0 ; --$i ) {
+        if ( !$records[$i]->check_rd_count() ) {
+            splice @records, $i, 1;
+        }
+    }
+
     return @records;
 }
 
@@ -53,6 +61,12 @@ sub additional {
 
     my @records = $self->additional_unfiltered;
 
+    for ( my $i = $#records ; $i >= 0 ; --$i ) {
+        if ( !$records[$i]->check_rd_count() ) {
+            splice @records, $i, 1;
+        }
+    }
+
     return @records;
 }
 
diff --git a/lib/Zonemaster/LDNS/RR.pm b/lib/Zonemaster/LDNS/RR.pm
@@ -60,6 +60,7 @@ use Zonemaster::LDNS::RR::RKEY;
 use Zonemaster::LDNS::RR::RP;
 use Zonemaster::LDNS::RR::RRSIG;
 use Zonemaster::LDNS::RR::RT;
+use Zonemaster::LDNS::RR::SIG;
 use Zonemaster::LDNS::RR::SINK;
 use Zonemaster::LDNS::RR::SOA;
 use Zonemaster::LDNS::RR::SPF;
diff --git a/lib/Zonemaster/LDNS/RR/NSEC3.pm b/lib/Zonemaster/LDNS/RR/NSEC3.pm
@@ -37,11 +37,11 @@ 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()
 
-Returns the next owner field.
+Returns the next hashed owner name field, in binary form. To convert the return value to the human-readable presentation format, use L<MIME::Base32/encode_base32hex>.
 
 =item typelist()
 
diff --git a/lib/Zonemaster/LDNS/RR/NSEC3PARAM.pm b/lib/Zonemaster/LDNS/RR/NSEC3PARAM.pm
@@ -33,6 +33,6 @@ 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.
 
 =back
diff --git a/src/LDNS.xs b/src/LDNS.xs
@@ -1727,6 +1727,46 @@ rr_rdf(obj,n)
     OUTPUT:
         RETVAL
 
+bool
+rr_check_rd_count(obj)
+    Zonemaster::LDNS::RR obj;
+    CODE:
+        ldns_rr_type rr_type = ldns_rr_get_type(obj);
+        ldns_rr_descriptor *desc = ldns_rr_descript(rr_type);
+        size_t rd_min = ldns_rr_descriptor_minimum(desc);
+        size_t rd_max = ldns_rr_descriptor_maximum(desc);
+        size_t rd_count = ldns_rr_rd_count(obj);
+
+        // Workaround for when the last field is variable length with length
+        // zero, and ldns represents this by omitting the last field from
+        // the field list.
+        if (rd_min > 0 && rd_min == rd_max)
+        {
+            switch (ldns_rr_descriptor_field_type(desc,rd_min-1))
+            {
+            // This list is taken from ldns_wire2rdf()
+            case LDNS_RDF_TYPE_APL:
+            case LDNS_RDF_TYPE_B64:
+            case LDNS_RDF_TYPE_HEX:
+            case LDNS_RDF_TYPE_NSEC:
+            case LDNS_RDF_TYPE_UNKNOWN:
+            case LDNS_RDF_TYPE_SERVICE:
+            case LDNS_RDF_TYPE_LOC:
+            case LDNS_RDF_TYPE_WKS:
+            case LDNS_RDF_TYPE_NSAP:
+            case LDNS_RDF_TYPE_ATMA:
+            case LDNS_RDF_TYPE_IPSECKEY:
+            case LDNS_RDF_TYPE_LONG_STR:
+            case LDNS_RDF_TYPE_AMTRELAY:
+            case LDNS_RDF_TYPE_NONE:
+                rd_min -= 1;
+            }
+        }
+
+        RETVAL = rd_min <= rd_count && rd_count <= rd_max;
+    OUTPUT:
+        RETVAL
+
 void
 rr_DESTROY(obj)
     Zonemaster::LDNS::RR obj;
@@ -2275,20 +2315,36 @@ 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)
     Zonemaster::LDNS::RR::NSEC3 obj;
+    INIT:
+        ldns_rdf *buf = NULL;
+        size_t size;
     CODE:
-        ldns_rdf *buf = ldns_nsec3_next_owner(obj);
-        RETVAL = newSVpvn((char *)ldns_rdf_data(buf), ldns_rdf_size(buf));
+        buf = ldns_nsec3_next_owner(obj);
+        if (!buf) {
+            XSRETURN_UNDEF;
+        }
+        size = ldns_rdf_size(buf);
+        if (size < 1) {
+            XSRETURN_UNDEF;
+        }
+
+        /* ldns_rdf_data(buf) points to the hashed next owner name preceded by a
+         * length byte, which we don't want. */
+        RETVAL = newSVpvn((char *)(ldns_rdf_data(buf) + 1), size - 1);
     OUTPUT:
         RETVAL
 
@@ -2337,22 +2393,45 @@ bool
 rr_nsec3_covers(obj,name)
     Zonemaster::LDNS::RR::NSEC3 obj;
     const char *name;
+    INIT:
+        /* Sanity test on owner name */
+        if (ldns_dname_label_count(ldns_rr_owner(obj)) == 0)
+            XSRETURN_UNDEF;
+
+        /* Sanity test on hashed next owner field */
+        ldns_rdf *next_owner = ldns_nsec3_next_owner(obj);
+        if (!next_owner || ldns_rdf_size(next_owner) <= 1)
+            XSRETURN_UNDEF;
+
     CODE:
     {
         ldns_rr *clone;
         ldns_rdf *dname;
         ldns_rdf *hashed;
         ldns_rdf *chopped;
 
-        clone = ldns_rr_clone(obj);
         dname = ldns_rdf_new_frm_str(LDNS_RDF_TYPE_DNAME, name);
+        if (!dname)
+            XSRETURN_UNDEF;
+
         ldns_dname2canonical(dname);
+
+        chopped = ldns_dname_left_chop(dname);
+        if (!chopped) {
+            ldns_rdf_deep_free(dname);
+            XSRETURN_UNDEF;
+        }
+
+        clone = ldns_rr_clone(obj);
         ldns_rr2canonical(clone);
         hashed = ldns_nsec3_hash_name_frm_nsec3(clone, dname);
-        chopped = ldns_dname_left_chop(dname);
+
         ldns_rdf_deep_free(dname);
+
         ldns_dname_cat(hashed,chopped);
+
         RETVAL = ldns_nsec_covers_name(clone,hashed);
+
         ldns_rdf_deep_free(hashed);
         ldns_rdf_deep_free(chopped);
         ldns_rr_free(clone);
@@ -2390,12 +2469,17 @@ rr_nsec3param_iterations(obj)
 SV *
 rr_nsec3param_salt(obj)
     Zonemaster::LDNS::RR::NSEC3PARAM obj;
-    PPCODE:
-        ldns_rdf *rdf = ldns_rr_rdf(obj,3);
-        if(ldns_rdf_size(rdf) > 0)
+    CODE:
+    {
+        ldns_rdf *rdf = ldns_rr_rdf(obj, 3);
+        size_t size = ldns_rdf_size(rdf);
+        if (size > 0)
         {
-            mPUSHs(newSVpvn((char *)ldns_rdf_data(rdf), ldns_rdf_size(rdf)));
+            RETVAL = newSVpvn((char *)(ldns_rdf_data(rdf) + 1), size - 1);
         }
+    }
+    OUTPUT:
+        RETVAL
 
 MODULE = Zonemaster::LDNS        PACKAGE = Zonemaster::LDNS::RR::PTR              PREFIX=rr_ptr_
 
diff --git a/t/dnssec.t b/t/dnssec.t
@@ -59,10 +59,31 @@ my $nsec = Zonemaster::LDNS::RR->new('xx.se.			7200	IN	NSEC	xx0r.se. NS RRSIG NS
 isa_ok($nsec, 'Zonemaster::LDNS::RR::NSEC');
 ok($nsec->covers('xx-example.se'), 'Covers xx-example.se');
 
+ok(!$nsec->covers('.'), 'Does not cover the root domain');
+
 my $nsec3 = Zonemaster::LDNS::RR->new('NR2E513KM693MBTNVHH56ENF54F886T0.com. 86400 IN NSEC3 1 1 0 - NR2FUHQVR56LH70L6F971J3L6N1RH2TU NS DS RRSIG');
 isa_ok($nsec3, 'Zonemaster::LDNS::RR::NSEC3');
 ok($nsec3->covers('xx-example.com'), 'Covers xx-example.com');
 
+is($nsec3->covers('.'), undef, 'Does not cover the root domain');
+
+subtest 'malformed NSEC3 do not cover anything' => sub {
+    # Malformed resource record lacking a next hashed owner name field in its
+    # RDATA. The only way to synthesize such a datum is to use the RFC 3597
+    # syntax.
+    my $example = Zonemaster::LDNS::RR->new(
+        q{example. 0 IN NSEC3 \# 15 01 00 0001 01 AB 00 0006 400000000002}
+    );
+    is( $example->covers("example"), undef );
+
+    # NSEC3 resource record whose owner name is the root name. This should
+    # normally not happen.
+    $example = Zonemaster::LDNS::RR->new(
+        q{.        0 IN NSEC3 1 0 1 ab 01234567 A RRSIG}
+    );
+    is( $example->covers("example"), undef );
+};
+
 SKIP: {
     skip 'no network', 3 unless $ENV{TEST_WITH_NETWORK};
 
diff --git a/t/packet.t b/t/packet.t
@@ -1,8 +1,10 @@
-use Test::More;
-use Test::Fatal;
-
 use strict;
 use warnings;
+use Test::More;
+
+use MIME::Base64;
+use Test::Differences;
+use Test::Fatal;
 
 use_ok('Zonemaster::LDNS');
 
@@ -59,4 +61,34 @@ subtest "croak when stringifying packet with malformed CAA" => sub {
     like( exception { $will_croak->() }, qr/^Failed to convert packet to string/ );
 };
 
+subtest "Answer section" => sub {
+    # Parse a packet with a single incomplete MX record
+    my $data = decode_base64( "EjSFgAABAAIAAAAAB2V4YW1wbGUCc2UAAA8AAcAMAA8AAQABUYAAAgAKwAwAAQABAAFRgAAEwAACAQ==");
+    my $p = Zonemaster::LDNS::Packet->new_from_wireformat( $data );
+
+    my $rr_count = scalar $p->answer;
+
+    is $rr_count, 1, "keep complete RRs but ignore incomplete ones";
+};
+
+subtest "Authority section" => sub {
+    # Parse a packet with a single incomplete MX record
+    my $data = decode_base64( "EjSFgAABAAAAAgAAB2V4YW1wbGUCc2UAAA8AAcAMAA8AAQABUYAAAgAKwAwAAQABAAFRgAAEwAACAQ==" );
+    my $p = Zonemaster::LDNS::Packet->new_from_wireformat( $data );
+
+    my $rr_count = scalar $p->authority;
+
+    is $rr_count, 1, "keep complete RRs but ignore incomplete ones";
+};
+
+subtest "Additional section" => sub {
+    # Parse a packet with a single incomplete MX record
+    my $data = decode_base64( "EjSFgAABAAAAAAACB2V4YW1wbGUCc2UAAA8AAcAMAA8AAQABUYAAAgAKwAwAAQABAAFRgAAEwAACAQ==" );
+    my $p = Zonemaster::LDNS::Packet->new_from_wireformat( $data );
+
+    my $rr_count = scalar $p->additional;
+
+    is $rr_count, 1, "keep complete RRs but ignore incomplete ones";
+};
+
 done_testing();
diff --git a/t/rr.t b/t/rr.t
