Invalid signatures for TXT records with and without quotes

[ekohl]

We had a customer who created two TXT records for the same domain, one with quotes and one without:

+-----------+------+----------------------------------------------------------------------+
| name      | type | content                                                              |
+-----------+------+----------------------------------------------------------------------+
| domein.nl | TXT  | "v=spf1 include:spf.protection.outlook.com -all"                     |
| domein.nl | TXT  | v=spf1 include:spf.protection.outlook.com -all                       |
+-----------+------+----------------------------------------------------------------------+

This resulted in an invalid RRSIG according to dnsviz and the SIDN DNSSEC validation.

Removing one of the records (without quotes) solved the issue.

# rpm -qv pdns-server
pdns-server-3.3-2.el6.MIND.x86_64

[mind04]

confirmed with 2a18506e6ad

Apr 15 14:48:28 unbound[30308:0] info: validation failure <check.mind-dns.nl. TXT IN>: signature crypto failed from 80.69.80.183

[romuald]

Aren't the records invalid without the quotes? (might depend which backend you're using)

[Habbie]

reconfirmed with 7b75810edbc4b09ea1f3b7fbceb6b285999ad743

[Habbie]

The issue is that this comparison (https://github.com/PowerDNS/pdns/blob/master/pdns/dnspacket.cc#L177) uses whatever the human typed in instead of the wire format.

[pieterlexis]

I've made a patch so only one RR is served:

diff --git a/pdns/dnspacket.cc b/pdns/dnspacket.cc
index faffcc5..4b61f02 100644
--- a/pdns/dnspacket.cc
+++ b/pdns/dnspacket.cc
@@ -183,10 +183,11 @@ void DNSPacket::addRecord(const DNSResourceRecord &rr)
   // for AXFR, no such checking is performed!
   // cerr<<"addrecord, content=["<<rr.content<<"]"<<endl;
   if(d_compress)
-    for(vector<DNSResourceRecord>::const_iterator i=d_rrs.begin();i!=d_rrs.end();++i) 
-      if(rr.qname==i->qname && rr.qtype==i->qtype && rr.content==i->content) {
+    for(vector<DNSResourceRecord>::const_iterator i=d_rrs.begin();i!=d_rrs.end();++i) {
+      if(rr.qname==i->qname && rr.qtype==i->qtype && (rr.content==i->content || (rr.qtype == QType::TXT && (rr.content==i->content || "\""+rr.content+"\""==i->content || rr.content=="\""+i->content+"\"")))){
           return;
       }
+    }
   // cerr<<"added to d_rrs"<<endl;
   d_rrs.push_back(rr);
 }

We need a discussion on whether we should spew a warning into the log telling the operator to run pdnsutil check-zone (which correctly identifies this as a duplicate) and/or check the datastore.

Also, should we check AXFRs in the same way? At the moment AXFRs still send the double record (with a broken signature).

[hlindqvist]

To further complicate the situation, the two records in the original report are actually not equivalent if content is parsed based on master file format.

Ie,

"v=spf1 include:spf.protection.outlook.com -all"

(a single string)

vs

"v=spf1" "include:spf.protection.outlook.com" "-all"

(three separate strings, which will work fine for DNS but not so well for SPF)

[Habbie]

The last example is irrelevant - those really are different records and thus will cause no trouble with DNSSEC and other RFCs that iemand uniqueness within an RRset.

[hlindqvist]

I probably wasn't clear enough but I was trying to specifically point out that the original premise of this issue are two records that I don't believe should be equal (same as the "test4" example in https://github.com/PowerDNS/pdns/issues/3503)

[Habbie]

Ah yes, that's right, the records in the top comment here should not be equal indeed.

[Habbie]

Between 4.0 and 4.1 we changed how we pass records from backend to core. We should check if that has fixed this problem.