SVCB: Correctly parse and print unknown params

There were multiple issues. With this commit, we *always* store the
bytes that are represented by the option. To do this, we needed to
properly parse RFC 1035 character-strings. This is now done with a
conversion of the ABNF from draft-ietf-dnsop-svcb-https-02 to ragel.

The resulting function could be used as a starting point for a better
TXT storage format.

Fixes #9829

diff --git a/pdns/dnslabeltext.rl b/pdns/dnslabeltext.rl
index ebd8944e11039b42e10b778cbe7e6c54d2549faf..7b2fd40ba50d5433ada600f0453f572ccc217190 100644 (file)
--- a/pdns/dnslabeltext.rl
+++ b/pdns/dnslabeltext.rl
@@ -6,6 +6,7 @@
 #include "dnsname.hh"
 #include "namespaces.hh"
 #include "dnswriter.hh"
+#include "misc.hh"
 
 namespace {
 void appendSplit(vector<string>& ret, string& segment, char c)
@@ -169,6 +170,76 @@ DNSName::string_t segmentDNSNameRaw(const char* realinput, size_t inputlen)
         return ret;
 };
 
+// Reads an RFC 1035 character string from 'in', puts the resulting bytes in 'out'.
+// Returns the amount of bytes read from 'in'
+size_t parseRFC1035CharString(const std::string &in, std::string &val) {
+
+  val.clear();
+  val.reserve(in.size());
+  const char *p = in.c_str();
+  const char *pe = p + in.size() + 1;
+  int cs = 0;
+  uint8_t escaped_octet = 0;
+  // Keeps track of how many chars we read from the source string
+  size_t counter=0;
+
+/* This parses an RFC 1035 char-string.
+ * It was created from the ABNF in draft-ietf-dnsop-svcb-https-02 with
+ * https://github.com/zinid/abnfc and modified to put all the characters in the
+ * right place.
+ */
+%%{
+  machine dns_text_to_string;
+
+  action doEscapedNumber {
+    escaped_octet *= 10;
+    escaped_octet += fc-'0';
+    counter++;
+  }
+
+  action doneEscapedNumber {
+    val += escaped_octet;
+    escaped_octet = 0;
+  }
+
+  action addToVal {
+    val += fc;
+    counter++;
+  }
+
+  action incrementCounter {
+    counter++;
+  }
+
+  # generated rules, define required actions
+  DIGIT = 0x30..0x39;
+  DQUOTE = "\"";
+  HTAB = "\t";
+  SP = " ";
+  WSP = (SP | HTAB)@addToVal;
+  non_special = "!" | 0x23..0x27 | 0x2a..0x3a | 0x3c..0x5b | 0x5d..0x7e;
+  non_digit = 0x21..0x2f | 0x3a..0x7e;
+  dec_octet = ( ( "0" | "1" ) DIGIT{2} ) | ( "2" ( ( 0x30..0x34 DIGIT ) | ( "5" 0x30..0x35 ) ) );
+  escaped = '\\'@incrementCounter ( non_digit$addToVal | dec_octet$doEscapedNumber@doneEscapedNumber );
+  contiguous = ( non_special$addToVal | escaped )+;
+  quoted = DQUOTE@incrementCounter ( contiguous | ( '\\'? WSP ) )* DQUOTE@incrementCounter;
+  char_string = (contiguous | quoted);
+
+  # instantiate machine rules
+  main := char_string;
+  write data;
+  write init;
+}%%
+
+  // silence warnings
+  (void) dns_text_to_string_first_final;
+  (void) dns_text_to_string_error;
+  (void) dns_text_to_string_en_main;
+  %% write exec;
+
+  return counter;
+}
+
 
 
 #if 0

diff --git a/pdns/dnswriter.cc b/pdns/dnswriter.cc
index 640922c950d871be5781afd04d60f412526de45b..adad9e0214309dd61729754cf627531694ce95eb 100644 (file)
--- a/pdns/dnswriter.cc
+++ b/pdns/dnswriter.cc
@@ -439,7 +439,7 @@ void DNSPacketWriter::xfrSvcParamKeyVals(const std::set<SvcParam> &kvs)
       break;
     default:
       xfr16BitInt(param.getValue().size());
-      xfrUnquotedText(param.getValue(), false);
+      xfrBlob(param.getValue());
       break;
     }
   }

diff --git a/pdns/misc.hh b/pdns/misc.hh
index 5d4f528e8be85942293515df14f79ffb1dd0b878..a4796f9e432da24f29f362a1bdbe095672ac4735 100644 (file)
--- a/pdns/misc.hh
+++ b/pdns/misc.hh
@@ -624,3 +624,4 @@ std::vector<ComboAddress> getResolvers(const std::string& resolvConfPath);
 DNSName reverseNameFromIP(const ComboAddress& ip);
 
 std::string getCarbonHostName();
+size_t parseRFC1035CharString(const std::string &in, std::string &val); // from ragel

diff --git a/pdns/rcpgenerator.cc b/pdns/rcpgenerator.cc
index 73473ac33e4408ddbb41e302c91d986bafb49c69..2a8e89184525e59e12d73f338d7839c1e512b434 100644 (file)
--- a/pdns/rcpgenerator.cc
+++ b/pdns/rcpgenerator.cc
@@ -298,6 +298,11 @@ void RecordTextReader::xfrBlob(string& val, int)
   B64Decode(tmp, val);
 }
 
+void RecordTextReader::xfrRFC1035CharString(string &val) {
+  auto ctr = parseRFC1035CharString(d_string.substr(d_pos, d_end - d_pos), val);
+  d_pos += ctr;
+}
+
 void RecordTextReader::xfrSvcParamKeyVals(set<SvcParam>& val)
 {
   while (d_pos != d_end) {
@@ -397,12 +402,7 @@ void RecordTextReader::xfrSvcParamKeyVals(set<SvcParam>& val)
     }
     default: {
       string value;
-      if (d_string.at(d_pos) == '"') {
-        xfrText(value);
-      }
-      else {
-        xfrUnquotedText(value);
-      }
+      xfrRFC1035CharString(value);
       val.insert(SvcParam(key, value));
       break;
     }
@@ -720,6 +720,27 @@ void RecordTextWriter::xfrHexBlob(const string& val, bool)
   }
 }
 
+// FIXME copied from dnsparser.cc, see #6010 and #3503 if you want a proper solution
+static string txtEscape(const string &name)
+{
+  string ret;
+  char ebuf[5];
+
+  for(string::const_iterator i=name.begin();i!=name.end();++i) {
+    if((unsigned char) *i >= 127 || (unsigned char) *i < 32) {
+      snprintf(ebuf, sizeof(ebuf), "\\%03u", (unsigned char)*i);
+      ret += ebuf;
+    }
+    else if(*i=='"' || *i=='\\'){
+      ret += '\\';
+      ret += *i;
+    }
+    else
+      ret += *i;
+  }
+  return ret;
+}
+
 void RecordTextWriter::xfrSvcParamKeyVals(const set<SvcParam>& val) {
   for (auto const &param : val) {
     if (!d_string.empty())
@@ -769,7 +790,10 @@ void RecordTextWriter::xfrSvcParamKeyVals(const set<SvcParam>& val) {
       break;
     }
     default:
-      d_string.append(param.getValue());
+      auto str = d_string;
+      d_string.clear();
+      xfrText(param.getValue(), false, false);
+      d_string = str + '"' + txtEscape(d_string) + '"';
       break;
     }
   }

diff --git a/pdns/rcpgenerator.hh b/pdns/rcpgenerator.hh
index afc1dcb25a21008a46e39f0de1c1e6964e27cf30..cd0ae80832a61fee1636790a30384628b3f8c027 100644 (file)
--- a/pdns/rcpgenerator.hh
+++ b/pdns/rcpgenerator.hh
@@ -63,11 +63,13 @@ public:
   void xfrBlob(string& val, int len=-1);
 
   void xfrSvcParamKeyVals(set<SvcParam>& val);
+  void xfrRFC1035CharString(string &val);
 
   const string getRemaining() const {
     return d_string.substr(d_pos);
   }
 
+
   bool eof();
 private:
   string d_string;

diff --git a/pdns/test-dnsrecords_cc.cc b/pdns/test-dnsrecords_cc.cc
index 06fa638490fdcd9bfeca2f566e1c36334edf4101..2f0c990b28dbda44146ef60e3d8c97e95cda3a01 100644 (file)
--- a/pdns/test-dnsrecords_cc.cc
+++ b/pdns/test-dnsrecords_cc.cc
@@ -219,6 +219,11 @@ BOOST_AUTO_TEST_CASE(test_record_types) {
      (CASE_L(QType::SVCB, "1 foo.powerdns.org. echconfig=aGVsbG8=", "1 foo.powerdns.org. echconfig=\"aGVsbG8=\"", "\0\x01\3foo\x08powerdns\x03org\x00\x00\x05\x00\x05hello"))
      (CASE_S(QType::SVCB, "1 foo.powerdns.org. ipv6hint=2001:db8::1,2001:db8::53:1", "\0\x01\3foo\x08powerdns\x03org\x00\x00\x06\x00\x20\x20\x01\x0d\xb8\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x01\x20\x01\x0d\xb8\x00\x00\x00\x00\x00\x00\x00\x00\x00\x53\x00\x01"))
 
+     (CASE_S(QType::SVCB, "1 foo.powerdns.org. key666=\"hello\"", "\0\x01\3foo\x08powerdns\x03org\x00\x02\x9a\x00\x005hello"))
+     (CASE_L(QType::SVCB, "1 foo.powerdns.org. key666=hello\\210qoo", "1 foo.powerdns.org. key666=\"hello\\210qoo\"", "\0\x01\3foo\x08powerdns\x03org\x00\x02\x9a\x00\x009hello\xd2qoo"))
+     (CASE_S(QType::SVCB, "1 foo.powerdns.org. key666=\"hello\\210qoo\"", "\0\x01\3foo\x08powerdns\x03org\x00\x02\x9a\x00\x009hello\xd2qoo"))
+     (CASE_S(QType::SVCB, "1 foo.powerdns.org. key666=\"hello\\210qoo bar\"", "\0\x01\3foo\x08powerdns\x03org\x00\x02\x9a\x00\x0dhello\xd2qoo bar"))
+
      (CASE_S(QType::SVCB, "16 foo.powerdns.org. mandatory=alpn alpn=h2,h3 ipv4hint=192.0.2.1", "\0\x10\3foo\x08powerdns\x03org\x00\x00\x00\x00\x02\x00\x01\x00\x01\x00\x06\x02h2\x02h3\x00\x04\x00\x04\xc0\x00\x02\x01"))
      (CASE_L(QType::SVCB, "16 foo.powerdns.org. alpn=h2,h3 mandatory=alpn ipv4hint=192.0.2.1", "16 foo.powerdns.org. mandatory=alpn alpn=h2,h3 ipv4hint=192.0.2.1", "\0\x10\3foo\x08powerdns\x03org\x00\x00\x00\x00\x02\x00\x01\x00\x01\x00\x06\x02h2\x02h3\x00\x04\x00\x04\xc0\x00\x02\x01"))
 

diff --git a/pdns/test-rcpgenerator_cc.cc b/pdns/test-rcpgenerator_cc.cc
index 7a84262802e422a7cc3b61db6551525140c3bd76..e7653f216c8e1a7e9834c404c7fd21d3f597f3f8 100644 (file)
--- a/pdns/test-rcpgenerator_cc.cc
+++ b/pdns/test-rcpgenerator_cc.cc
@@ -274,7 +274,7 @@ BOOST_AUTO_TEST_CASE(test_xfrSvcParamKeyVals_generic) {
         string target;
         RecordTextWriter rtw(target);
         rtw.xfrSvcParamKeyVals(v);
-        BOOST_CHECK_EQUAL(target, source);
+        BOOST_CHECK_EQUAL(target, "key666=\"foobar\"");
 
         v.clear();
         RecordTextReader rtr2("key666=");
@@ -292,13 +292,37 @@ BOOST_AUTO_TEST_CASE(test_xfrSvcParamKeyVals_generic) {
         k = v.begin()->getKey();
         BOOST_CHECK(k == SvcParam::keyFromString("key666"));
         val = v.begin()->getValue();
-        BOOST_CHECK_EQUAL(val, "\"blablabla\"");
+        BOOST_CHECK_EQUAL(val, "blablabla");
 
         // Check the writer
         target.clear();
         RecordTextWriter rtw2(target);
         rtw2.xfrSvcParamKeyVals(v);
         BOOST_CHECK_EQUAL(target, source);
+
+        v.clear();
+        source = "key666=\"foo\\123 bar\"";
+        RecordTextReader rtr5(source);
+        rtr5.xfrSvcParamKeyVals(v);
+        BOOST_CHECK_EQUAL(v.size(), 1U);
+        k = v.begin()->getKey();
+        BOOST_CHECK(k == SvcParam::keyFromString("key666"));
+        val = v.begin()->getValue();
+        BOOST_CHECK_EQUAL(val, "foo{ bar");
+
+        // Check the writer
+        target.clear();
+        RecordTextWriter rtw3(target);
+        rtw3.xfrSvcParamKeyVals(v);
+        BOOST_CHECK_EQUAL("key666=\"foo{ bar\"", target);
+
+        v.clear();
+        RecordTextReader rtr6("key665= blabla");
+        BOOST_CHECK_THROW(rtr6.xfrSvcParamKeyVals(v), RecordTextException);
+
+        v.clear();
+        RecordTextReader rtr7("key665=bla bla");
+        BOOST_CHECK_THROW(rtr7.xfrSvcParamKeyVals(v), RecordTextException);
 }
 
 BOOST_AUTO_TEST_CASE(test_xfrSvcParamKeyVals_multiple) {
@@ -335,7 +359,7 @@ BOOST_AUTO_TEST_CASE(test_xfrSvcParamKeyVals_multiple) {
         string target;
         RecordTextWriter rtw(target);
         rtw.xfrSvcParamKeyVals(v);
-        BOOST_CHECK_EQUAL(target, "mandatory=alpn alpn=h2,h3 ipv4hint=192.0.2.1,192.0.2.2 echconfig=\"dG90YWxseSBib2d1cyBlY2hjb25maWcgdmFsdWU=\" ipv6hint=2001:db8::1 key666=foobar");
+        BOOST_CHECK_EQUAL(target, "mandatory=alpn alpn=h2,h3 ipv4hint=192.0.2.1,192.0.2.2 echconfig=\"dG90YWxseSBib2d1cyBlY2hjb25maWcgdmFsdWU=\" ipv6hint=2001:db8::1 key666=\"foobar\"");
 }
 
 BOOST_AUTO_TEST_CASE(test_xfrSvcParamKeyVals_echconfig) {