PATCH: Re: DNSBL and answer id check missing

[Diego d'Ambra]

Diego d'Ambra wrote:

To me it seems that plugin DNSBL is using Net::DNS bgsend/bgread, but is not checking the id of the reply received.

[...]



Changelog:
 * Added source port randomisation to DNS queries
 * Added id validation of DNS replies





The 0x20 stuff is (also) beyond my skills ;-)

Best regards,
Diego d'Ambra

Index: plugins/dnsbl
===================================================================
--- plugins/dnsbl       (revision 947)
+++ plugins/dnsbl       (working copy)
@@ -54,24 +54,31 @@
   my $res = new Net::DNS::Resolver;
   $res->tcp_timeout(30);
   $res->udp_timeout(30);
+  $res->srcport(1024+int(rand(64511)));
 
   my $sel = IO::Select->new();
 
   my $dom;
+  my %packet_ids;
   for my $dnsbl (keys %dnsbl_zones) {
     # fix to find A records, if the dnsbl_zones line has a second field 
20/1/04 ++msp
     $dom->{"$reversed_ip.$dnsbl"} = 1;
     if (defined($dnsbl_zones{$dnsbl})) {
       $self->log(LOGDEBUG, "Checking $reversed_ip.$dnsbl for A record in the 
background");
-      $sel->add($res->bgsend("$reversed_ip.$dnsbl"));
+      $packet = _dns_packet("$reversed_ip.$dnsbl");
+      $packet_ids{ _packet_id($packet) } = 1;
+      $sel->add( $res->bgsend($packet) );
     } else {
       $self->log(LOGDEBUG, "Checking $reversed_ip.$dnsbl for TXT record in the 
background");
-      $sel->add($res->bgsend("$reversed_ip.$dnsbl", "TXT"));
+      $packet = _dns_packet( "$reversed_ip.$dnsbl", "TXT" );
+      $packet_ids{ _packet_id($packet) } = 1;
+      $sel->add( $res->bgsend($packet) );
     }
   }
 
   $self->qp->connection->notes('dnsbl_sockets', $sel);
   $self->qp->connection->notes('dnsbl_domains', $dom);
+  $self->qp->connection->notes('dnsbl_ids', \%packet_ids);
 
   return DECLINED;
 }
@@ -92,6 +99,7 @@
 
   my $sel = $conn->notes('dnsbl_sockets') or return "";
   my $dom = $conn->notes('dnsbl_domains');
+  my $packet_ids = $conn->notes('dnsbl_ids');
   my $remote_ip = $self->qp->connection->remote_ip;
 
   my $result; 
@@ -113,6 +121,11 @@
 
     if ($query) {
       my $a_record = 0;
+
+      #check reply is a valid
+      my $id = _packet_id($query);
+      next unless ( exists( $packet_ids->{$id} ) );
+
       foreach my $rr ($query->answer) {
        my $name = $rr->name;
        $self->log(LOGDEBUG, "name $name");
@@ -206,6 +219,51 @@
   return DECLINED;
 }
 
+#_dns_packet: private function to create new Net::DNS::Packet object
+#arg0: str with host
+#arg1: str with type
+#ret0: ref to dns packet object
+sub _dns_packet
+{
+    my $host = shift;    #arg0
+    my $type = shift;    #arg1
+    
+    #set default
+    $type = 'A' if ( !defined($type) );
+    
+    #create new dns packet
+    my $packet = Net::DNS::Packet->new( $host, $type );
+    
+    #return packet object
+    return ($packet);
+}
+    
+#_packet_id: private function that calculate id to match dns replies.
+#arg0: ref to dns packet object
+#ret0: str with id
+sub _packet_id
+{
+    my $packet = shift;    #arg0
+
+    #extract data from packet
+    my $header    = $packet->header;
+    my $id        = $header->id;
+    my @questions = $packet->question;
+    my $ques      = $questions[0];
+
+    #calculate id
+    if ( defined $ques )
+    {
+        return join '/', $id, $ques->qname, $ques->qtype, $ques->qclass;
+    }
+    else
+    {
+    
+        #just return (safe) id to support broken DNS servers that replies with 
on question section
+        return $id . "NO_QUESTION_IN_PACKET";
+    }
+}
+
 1;
 
 =head1 Usage

Attached suggested patch. Large part of it inspired by how Spamassassin does it. I did notice a small (but important) change from the version I tested and latest DNSBL taken from svn - $dom - it actually saves the request and later matches the reply against it. So as I say in OP, maybe a storm for nothing. Anyway, I do think id checking is the best solution (if port and id are random).

smime.p7s
Description: S/MIME Cryptographic Signature