auth: Prevent a race during the processing of SVC auto-hints

author Remi Gacogne <remi.gacogne@powerdns.com>

Thu, 16 Mar 2023 13:18:49 +0000 (14:18 +0100)

committer Remi Gacogne <remi.gacogne@powerdns.com>

Fri, 17 Mar 2023 14:44:23 +0000 (15:44 +0100)
author Remi Gacogne <remi.gacogne@powerdns.com>
Thu, 16 Mar 2023 13:18:49 +0000 (14:18 +0100)
committer Remi Gacogne <remi.gacogne@powerdns.com>
Fri, 17 Mar 2023 14:44:23 +0000 (15:44 +0100)
diff --git a/pdns/dnsrecords.cc b/pdns/dnsrecords.cc

index 475f12cfa50e997132f33267cb42905110dfd488..e2212e36cd697734a7688f49b279fa7fe59497e1 100644 (file)
--- a/pdns/dnsrecords.cc
+++ b/pdns/dnsrecords.cc
@@ -341,7 +341,7 @@ boilerplate_conv(SVCB,
                   conv.xfr16BitInt(d_priority);
                   conv.xfrName(d_target, false, true);
                   if (d_priority != 0) {
-                   conv.xfrSvcParamKeyVals(*d_params.lock());
+                   conv.xfrSvcParamKeyVals(d_params);
                   }
                   )
  
@@ -349,7 +349,7 @@ boilerplate_conv(HTTPS,
                   conv.xfr16BitInt(d_priority);
                   conv.xfrName(d_target, false, true);
                   if (d_priority != 0) {
-                   conv.xfrSvcParamKeyVals(*d_params.lock());
+                   conv.xfrSvcParamKeyVals(d_params);
                   }
                   )
  
@@ -747,29 +747,28 @@ string APLRecordContent::getZoneRepresentation(bool /* noDot */) const {
  /* APL end */
  
  /* SVCB start */
-bool SVCBBaseRecordContent::autoHint(const SvcParam::SvcParamKey &key) {
-  auto params = d_params.lock();
-  auto p = getParamIt(key, *params);
-  if (p == params->end()) {
+bool SVCBBaseRecordContent::autoHint(const SvcParam::SvcParamKey &key) const {
+  auto p = getParamIt(key);
+  if (p == d_params.end()) {
      return false;
    }
    return p->getAutoHint();
  }
  
  void SVCBBaseRecordContent::setHints(const SvcParam::SvcParamKey &key, const std::vector<ComboAddress> &addresses) {
+  auto p = getParamIt(key);
+  if (p == d_params.end()) {
+    return;
+  }
+
    std::vector<ComboAddress> h;
    h.reserve(h.size() + addresses.size());
    h.insert(h.end(), addresses.begin(), addresses.end());
  
-  auto params = d_params.lock();
-  auto p = getParamIt(key, *params);
-  if (p == params->end()) {
-    return;
-  }
    try {
      auto newParam = SvcParam(key, std::move(h));
-    params->erase(p);
-    params->insert(newParam);
+    d_params.erase(p);
+    d_params.insert(newParam);
    } catch (...) {
      // XXX maybe we should SERVFAIL instead?
      return;
@@ -777,40 +776,47 @@ void SVCBBaseRecordContent::setHints(const SvcParam::SvcParamKey &key, const std
  }
  
  void SVCBBaseRecordContent::removeParam(const SvcParam::SvcParamKey &key) {
-  auto params = d_params.lock();
-  auto p = getParamIt(key, *params);
-  if (p == params->end()) {
+  auto p = getParamIt(key);
+  if (p == d_params.end()) {
      return;
    }
-  params->erase(p);
+  d_params.erase(p);
  }
  
-bool SVCBBaseRecordContent::hasParams() {
-  return !d_params.lock()->empty();
+bool SVCBBaseRecordContent::hasParams() const {
+  return !d_params.empty();
  }
  
-bool SVCBBaseRecordContent::hasParam(const SvcParam::SvcParamKey &key) {
-  auto params = d_params.lock();
-  return getParamIt(key, *params) != params->end();
+bool SVCBBaseRecordContent::hasParam(const SvcParam::SvcParamKey &key) const {
+  return getParamIt(key) != d_params.end();
  }
  
-SvcParam SVCBBaseRecordContent::getParam(const SvcParam::SvcParamKey &key) {
-  auto params = d_params.lock();
-  auto p = getParamIt(key, *params);
-  if (p == params->end()) {
+SvcParam SVCBBaseRecordContent::getParam(const SvcParam::SvcParamKey &key) const {
+  auto p = getParamIt(key);
+  if (p == d_params.end()) {
      throw std::out_of_range("No param with key " + SvcParam::keyToString(key));
    }
    return *p;
  }
  
-set<SvcParam>::const_iterator SVCBBaseRecordContent::getParamIt(const SvcParam::SvcParamKey &key, std::set<SvcParam>& params) {
-  auto p = std::find_if(params.begin(), params.end(),
+set<SvcParam>::const_iterator SVCBBaseRecordContent::getParamIt(const SvcParam::SvcParamKey &key) const {
+  auto p = std::find_if(d_params.begin(), d_params.end(),
        [&key](const SvcParam &param) {
          return param.getKey() == key;
        });
    return p;
  }
  
+std::shared_ptr<SVCBBaseRecordContent> SVCBRecordContent::clone() const
+{
+  return std::shared_ptr<SVCBBaseRecordContent>(std::make_shared<SVCBRecordContent>(*this));
+}
+
+std::shared_ptr<SVCBBaseRecordContent> HTTPSRecordContent::clone() const
+{
+  return std::shared_ptr<SVCBBaseRecordContent>(std::make_shared<HTTPSRecordContent>(*this));
+}
+
  /* SVCB end */
  
  boilerplate_conv(TKEY,
diff --git a/pdns/dnsrecords.hh b/pdns/dnsrecords.hh

index 245d8e8d4e40a3ab02aea1e807fc9dc49d299790..1d3b45c5a48791d13d1c2568e9e23f161f99711e 100644 (file)
--- a/pdns/dnsrecords.hh
+++ b/pdns/dnsrecords.hh
@@ -517,37 +517,40 @@ class SVCBBaseRecordContent : public DNSRecordContent
      const DNSName& getTarget() const {return d_target;}
      uint16_t getPriority() const {return d_priority;}
      // Returns true if a value for |key| was set to 'auto'
-    bool autoHint(const SvcParam::SvcParamKey &key);
+    bool autoHint(const SvcParam::SvcParamKey &key) const;
      // Sets the |addresses| to the existing hints for |key|
      void setHints(const SvcParam::SvcParamKey &key, const std::vector<ComboAddress> &addresses);
      // Removes the parameter for |key| from d_params
      void removeParam(const SvcParam::SvcParamKey &key);
      // Whether or not there are any parameter
-    bool hasParams();
+    bool hasParams() const;
      // Whether or not the param of |key| exists
-    bool hasParam(const SvcParam::SvcParamKey &key);
+    bool hasParam(const SvcParam::SvcParamKey &key) const;
      // Get the parameter with |key|, will throw out_of_range if param isn't there
-    SvcParam getParam(const SvcParam::SvcParamKey &key);
+    SvcParam getParam(const SvcParam::SvcParamKey &key) const;
+    virtual std::shared_ptr<SVCBBaseRecordContent> clone() const = 0;
  
    protected:
-    LockGuarded<set<SvcParam>> d_params;
+    std::set<SvcParam> d_params;
      DNSName d_target;
      uint16_t d_priority;
  
      // Get the iterator to parameter with |key|, return value can be d_params::end
-    static set<SvcParam>::const_iterator getParamIt(const SvcParam::SvcParamKey &key, set<SvcParam>& params);
+  std::set<SvcParam>::const_iterator getParamIt(const SvcParam::SvcParamKey &key) const;
  };
  
  class SVCBRecordContent : public SVCBBaseRecordContent
  {
  public:
    includeboilerplate(SVCB)
+  std::shared_ptr<SVCBBaseRecordContent> clone() const override;
  };
  
  class HTTPSRecordContent : public SVCBBaseRecordContent
  {
  public:
    includeboilerplate(HTTPS)
+  std::shared_ptr<SVCBBaseRecordContent> clone() const override;
  };
  
  class RRSIGRecordContent : public DNSRecordContent
diff --git a/pdns/packethandler.cc b/pdns/packethandler.cc

index a1e3c71afe04a757dddd861f40c94398c16cc3d1..81d157dcb620b53dcb437194217bcee13ea9ba38 100644 (file)
--- a/pdns/packethandler.cc
+++ b/pdns/packethandler.cc
@@ -549,6 +549,12 @@ void PacketHandler::doAdditionalProcessing(DNSPacket& p, std::unique_ptr<DNSPack
      DNSName target = rrc->getTarget().isRoot() ? rec->dr.d_name : rrc->getTarget();
  
      if (rrc->hasParam(SvcParam::ipv4hint) && rrc->autoHint(SvcParam::ipv4hint)) {
+      auto newRRC = rrc->clone();
+      if (!newRRC) {
+        continue;
+      }
+      rrc = newRRC;
+      rec->dr.d_content = newRRC;
        if (s_SVCAutohints) {
          auto hints = getIPAddressFor(target, QType::A);
          if (hints.size() == 0) {
@@ -562,6 +568,12 @@ void PacketHandler::doAdditionalProcessing(DNSPacket& p, std::unique_ptr<DNSPack
      }
  
      if (rrc->hasParam(SvcParam::ipv6hint) && rrc->autoHint(SvcParam::ipv6hint)) {
+      auto newRRC = rrc->clone();
+      if (!newRRC) {
+        continue;
+      }
+      rrc = newRRC;
+      rec->dr.d_content = newRRC;
        if (s_SVCAutohints) {
          auto hints = getIPAddressFor(target, QType::AAAA);
          if (hints.size() == 0) {
author	Remi Gacogne <remi.gacogne@powerdns.com>
	Thu, 16 Mar 2023 13:18:49 +0000 (14:18 +0100)
committer	Remi Gacogne <remi.gacogne@powerdns.com>
	Fri, 17 Mar 2023 14:44:23 +0000 (15:44 +0100)
pdns/dnsrecords.cc		patch \| blob \| blame \| history
pdns/dnsrecords.hh		patch \| blob \| blame \| history
pdns/packethandler.cc		patch \| blob \| blame \| history