Ignore incomplete RRs #136

mattias-p · 2022-03-16T09:53:38Z

Purpose

This PR makes sure that we're not assuming all RRs come with all fields included. Out-of-the-box ldns returns such RRs even though they're incomplete. If we're unlucky we could be accessing such non-existing fields and that could lead to assertion errors or unexpected undef values.

Context

I found this when investigating #110.

Changes

Introduces Zonemaster::LDNS::RR::check_rd_count() that validates the rdata field count for a record.
Changes Zonemaster::LDNS::Packet::{answer,authority,additional}() to filter out incomplete RRs.
The full set of RRs is still available through Zonemaster::LDNS::Packet::{answer,authority,additional}_unfiltered().

And I've included @pnax's fix from #133 for the broken tests.

How to test this PR

Just run make test. New unit test are added to verify that incomplete RRs are filtered out. The old tests verify that complete RRs are not filtered out.

matsduf · 2022-03-25T12:43:59Z

t/rr.t

-            # Alg 8 will replace 5. Now (December 2017) both are used.
-            ok( $rr->algorithm == 5 or $rr->algorithm == 8 );
+            # Alg 8 has replaced 5. Now (February 2022) only alg 8 is used.
+            ok( $rr->algorithm == 8 );


Is this really related to the PR description? I think this should be in a separate PR with a relevant description.

Yeah, it's under the Changes section:

And I've included @pnax's fix for the broken tests.

Preferably it should be in a separate PR, and "@pnax's fix for the broken tests" is not much of documentation.

matsduf · 2022-03-25T12:45:48Z

t/rr.t

-                ok( $rr->keytag == 59747 or $rr->keytag == 59407 );
+                # .SE KSK should not change very often. 59407 has replaced 59747.
+                # Now (February 2022) only 59407 is used.
+                ok( $rr->keytag == 59407 );


See comment above. And more changes below.

lib/Zonemaster/LDNS/Packet.pm

matsduf · 2022-03-25T16:48:51Z

How is this PR related to #135? They both introduce the new defintion of Zonemaster::LDNS::Packet::answer method.

ghost · 2022-04-19T10:19:30Z

How is this PR related to #135? They both introduce the new defintion of Zonemaster::LDNS::Packet::answer method.

They both use the same commit (86d8fef and 86d8fef) so both PR can be built on top of the same root and there won't be any conflict on merge.

ghost

LGTM.

tgreenx

LGTM, a welcome change here as well

matsduf · 2022-05-06T11:55:55Z

Will this PR resolve #110?

marc-vanderwal · 2022-08-01T11:33:54Z

Hi,

I was conducting some additional tests on this pull requests because I was wondering whether it would solve the crashes I witnessed when fuzz testing Zonemaster. One of my runs ended with the following fatal error:

   5.69 CRITICAL  Erreur fatale dans le module Zonemaster::Engine::Test::Nameserver: Can't locate object method "check_rd_count" via package "Zonemaster::LDNS::RR::SIG" at /home/mvw/.perl5/lib/perl5/x86_64-linux-gnu-thread-multi/Zonemaster/LDNS/Packet.pm line 34, <DATA> line 1.

I found the solution to this small issue, and I think it should be added to this pull request in order to avoid a regression when a server returns SIG records.

mattias-p · 2023-11-30T17:49:57Z

I've rebased this onto current develop. Please re-review and I'll merge it.

SIG resource records were missing methods defined in LDNS.xs because of a missing use statement in Zonemaster::LDNS::RR.

matsduf

If I understand this PR, incomplete RRs will be silently ignored from processing. Since Zonemaster is testing tool I think it would make more sence if the information is signalled to Engine, that can output a message that this has happened, e.g. (ERROR, "MX record with invalid format has been ignored from response of X query for DOMAIN to server SERVER/IP"

If this PR prevents crashes I think it is OK if it is seen as a preliminary work-around, but that should then be documented in the code and an issue being created.

mattias-p · 2023-12-04T09:30:28Z

Correct. Broken RRs are silently dropped when using the API the way Engine does today. This prevents crashes.

While Zonemaster::LDNS does not actively signal that a broken RR has been found, Engine could use the current Zonemaster::LDNS API without too much trouble to find out if any RRs have been dropped and the RRtypes of the dropped RRs. Doing this might put a little extra pressure on the garbage collector so it would be interesting to have a look at the performance impact. If the impact is significant we could provide a more efficient API in Zonemaster::LDNS.

Edit: For context, the dropping mechanism was introduced in #135.

mattias-p changed the title ~~Ignore incomplete rr~~ Ignore incomplete RRs Mar 16, 2022

mattias-p requested review from hannaeko, matsduf, a user and tgreenx March 16, 2022 10:11

mattias-p mentioned this pull request Mar 16, 2022

Ignore DNSKEY RRs with incalculable key sizes #135

Merged

mattias-p force-pushed the ignore-incomplete-rr branch from 4aa059a to 2e32534 Compare March 16, 2022 19:40

mattias-p added this to the v2022.1 milestone Mar 21, 2022

matsduf reviewed Mar 25, 2022

View reviewed changes

ghost previously approved these changes Apr 19, 2022

View reviewed changes

tgreenx previously approved these changes May 5, 2022

View reviewed changes

matsduf previously approved these changes May 6, 2022

View reviewed changes

matsduf modified the milestones: v2022.1, v2022.2 May 11, 2022

marc-vanderwal dismissed stale reviews from matsduf, tgreenx, and ghost via 0ca82ce August 1, 2022 11:25

marc-vanderwal mentioned this pull request Aug 1, 2022

Segfault when parsing a bad CAA resource record #149

Closed

matsduf modified the milestones: v2022.2, v2023.1 Nov 8, 2022

matsduf modified the milestones: v2023.1, v2023.2 May 2, 2023

mattias-p force-pushed the ignore-incomplete-rr branch from 0ca82ce to 0541cca Compare November 30, 2023 17:45

mattias-p added 2 commits November 30, 2023 19:01

Ignore incomplete records

68e1a62

Work around for ldns omitting empty variable length fields

898e662

Add missing methods for Zonemaster::LDNS::RR::SIG

0541cca

SIG resource records were missing methods defined in LDNS.xs because of a missing use statement in Zonemaster::LDNS::RR.

matsduf reviewed Dec 1, 2023

View reviewed changes

matsduf approved these changes Dec 4, 2023

View reviewed changes

mattias-p merged commit 3b9154f into zonemaster:develop Dec 4, 2023
1 check passed

matsduf mentioned this pull request Dec 11, 2023

PRs for v2023.2 zonemaster/zonemaster#1232

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ignore incomplete RRs #136

Ignore incomplete RRs #136

mattias-p commented Mar 16, 2022 •

edited

Loading

matsduf Mar 25, 2022

mattias-p Mar 25, 2022

matsduf Mar 25, 2022

matsduf Mar 25, 2022

matsduf commented Mar 25, 2022

ghost commented Apr 19, 2022 •

edited by ghost

Loading

ghost left a comment

tgreenx left a comment

matsduf commented May 6, 2022

marc-vanderwal commented Aug 1, 2022

mattias-p commented Nov 30, 2023

matsduf left a comment •

edited

Loading

mattias-p commented Dec 4, 2023 •

edited

Loading

Ignore incomplete RRs #136

Ignore incomplete RRs #136

Conversation

mattias-p commented Mar 16, 2022 • edited Loading

Purpose

Context

Changes

How to test this PR

matsduf Mar 25, 2022

Choose a reason for hiding this comment

mattias-p Mar 25, 2022

Choose a reason for hiding this comment

matsduf Mar 25, 2022

Choose a reason for hiding this comment

matsduf Mar 25, 2022

Choose a reason for hiding this comment

matsduf commented Mar 25, 2022

ghost commented Apr 19, 2022 • edited by ghost Loading

ghost left a comment

Choose a reason for hiding this comment

tgreenx left a comment

Choose a reason for hiding this comment

matsduf commented May 6, 2022

marc-vanderwal commented Aug 1, 2022

mattias-p commented Nov 30, 2023

matsduf left a comment • edited Loading

Choose a reason for hiding this comment

mattias-p commented Dec 4, 2023 • edited Loading

mattias-p commented Mar 16, 2022 •

edited

Loading

ghost commented Apr 19, 2022 •

edited by ghost

Loading

matsduf left a comment •

edited

Loading

mattias-p commented Dec 4, 2023 •

edited

Loading