Ignore incomplete RRs - Githubissues

zonemaster / zonemaster-ldns

A Perl interface module for Zonemaster to NLnet Labs' ldns library

Other

8 stars 12 forks source link

Ignore incomplete RRs #136

Closed mattias-p closed 10 months ago

mattias-p commented 2 years ago

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 commented 2 years ago

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

ghost commented 2 years ago

How is this PR related to https://github.com/zonemaster/zonemaster-ldns/pull/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.

matsduf commented 2 years ago

Will this PR resolve #110?

marc-vanderwal commented 2 years ago

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 commented 10 months ago

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

mattias-p commented 10 months ago

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.