From 1e332f681bb16a12bf0c5075d058b7ea850f4288 Mon Sep 17 00:00:00 2001
From: Pieter Lexis <pieter.lexis@powerdns.com>
Date: Tue, 31 Oct 2017 22:57:46 +0100
Subject: [PATCH] recursor: Prevent DNSSEC downgrade attacks

RFC 4509 section 3: "Validator implementations SHOULD ignore DS RR
containing SHA-1 digests if DS RRs with SHA-256 digests are present in the
DS RRset."

As SHA348 is specified as well, the spirit of the this line is "use the
best algorithm".

This also means that if a delegation has DS records for multiple keys
(and algos) and only a subset have stronger digests, we will discard the
DS records for the weaker digests.
---
 pdns/syncres.cc | 20 ++++++++++++++++++++
 pdns/syncres.hh |  3 ++-
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/pdns/syncres.cc b/pdns/syncres.cc
index 504aa61ce..79db0a646 100644
--- a/pdns/syncres.cc
+++ b/pdns/syncres.cc
@@ -33,6 +33,7 @@
 #include "lua-recursor4.hh"
 #include "rec-lua-conf.hh"
 #include "syncres.hh"
+#include "dnsseckeeper.hh"
 #include "validate-recursor.hh"
 
 thread_local SyncRes::ThreadLocalStorage SyncRes::t_sstorage;
@@ -1542,12 +1543,21 @@ vState SyncRes::getDSRecords(const DNSName& zone, dsmap_t& ds, bool taOnly, unsi
 
   if (rcode == RCode::NoError || (rcode == RCode::NXDomain && !bogusOnNXD)) {
 
+    uint8_t bestDigestType = 0;
+
     if (state == Secure) {
       bool gotCNAME = false;
       for (const auto& record : dsrecords) {
         if (record.d_type == QType::DS) {
           const auto dscontent = getRR<DSRecordContent>(record);
           if (dscontent && isSupportedDS(*dscontent)) {
+            // Make GOST a lower prio than SHA256
+            if (dscontent->d_digesttype == DNSSECKeeper::GOST && bestDigestType == DNSSECKeeper::SHA256) {
+              continue;
+            }
+            if (dscontent->d_digesttype > bestDigestType || (bestDigestType == DNSSECKeeper::GOST && dscontent->d_digesttype == DNSSECKeeper::SHA256)) {
+              bestDigestType = dscontent->d_digesttype;
+            }
             ds.insert(*dscontent);
           }
         }
@@ -1556,6 +1566,16 @@ vState SyncRes::getDSRecords(const DNSName& zone, dsmap_t& ds, bool taOnly, unsi
         }
       }
 
+      /* RFC 4509 section 3: "Validator implementations SHOULD ignore DS RRs containing SHA-1
+       * digests if DS RRs with SHA-256 digests are present in the DS RRset."
+       * As SHA348 is specified as well, the spirit of the this line is "use the best algorithm".
+       */
+      for (const auto& dsrec : ds) {
+        if (dsrec.d_digesttype != bestDigestType) {
+          ds.erase(dsrec);
+        }
+      }
+
       if (rcode == RCode::NoError && ds.empty()) {
         if (foundCut) {
           if (gotCNAME || denialProvesNoDelegation(zone, dsrecords)) {
diff --git a/pdns/syncres.hh b/pdns/syncres.hh
index 59bd8e253..d93138043 100644
--- a/pdns/syncres.hh
+++ b/pdns/syncres.hh
@@ -329,6 +329,8 @@ public:
   typedef map<DNSName, DecayingEwmaCollection> nsspeeds_t;
   typedef map<ComboAddress, EDNSStatus> ednsstatus_t;
 
+  vState getDSRecords(const DNSName& zone, dsmap_t& ds, bool onlyTA, unsigned int depth, bool bogusOnNXD=true, bool* foundCut=nullptr);
+
   class AuthDomain
   {
   public:
@@ -754,7 +756,6 @@ private:
   void updateValidationState(vState& state, const vState stateUpdate);
   vState validateRecordsWithSigs(unsigned int depth, const DNSName& qname, const QType& qtype, const DNSName& name, const std::vector<DNSRecord>& records, const std::vector<std::shared_ptr<RRSIGRecordContent> >& signatures);
   vState validateDNSKeys(const DNSName& zone, const std::vector<DNSRecord>& dnskeys, const std::vector<std::shared_ptr<RRSIGRecordContent> >& signatures, unsigned int depth);
-  vState getDSRecords(const DNSName& zone, dsmap_t& ds, bool onlyTA, unsigned int depth, bool bogusOnNXD=true, bool* foundCut=nullptr);
   vState getDNSKeys(const DNSName& signer, skeyset_t& keys, unsigned int depth);
   dState getDenialValidationState(NegCache::NegCacheEntry& ne, const vState state, const dState expectedState, bool referralToUnsigned);
   void updateDenialValidationState(NegCache::NegCacheEntry& ne, vState& state, const dState denialState, const dState expectedState, bool allowOptOut);
-- 
2.40.0