From: Pieter Lexis Date: Wed, 9 Dec 2020 16:03:53 +0000 (+0100) Subject: SVCB: Correctly parse and print unknown params X-Git-Tag: rec-4.5.0-alpha1~56^2~1 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=b1a048a9caa0d14f9e6665c45af004c041fe5c5e;p=thirdparty%2Fpdns.git 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 ebd8944e11..7b2fd40ba5 100644 --- 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& 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 640922c950..adad9e0214 100644 --- a/pdns/dnswriter.cc +++ b/pdns/dnswriter.cc @@ -439,7 +439,7 @@ void DNSPacketWriter::xfrSvcParamKeyVals(const std::set &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 5d4f528e8b..a4796f9e43 100644 --- a/pdns/misc.hh +++ b/pdns/misc.hh @@ -624,3 +624,4 @@ std::vector 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 73473ac33e..2a8e891845 100644 --- 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& val) { while (d_pos != d_end) { @@ -397,12 +402,7 @@ void RecordTextReader::xfrSvcParamKeyVals(set& 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& val) { for (auto const ¶m : val) { if (!d_string.empty()) @@ -769,7 +790,10 @@ void RecordTextWriter::xfrSvcParamKeyVals(const set& 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 afc1dcb25a..cd0ae80832 100644 --- a/pdns/rcpgenerator.hh +++ b/pdns/rcpgenerator.hh @@ -63,11 +63,13 @@ public: void xfrBlob(string& val, int len=-1); void xfrSvcParamKeyVals(set& 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 06fa638490..2f0c990b28 100644 --- 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 7a84262802..e7653f216c 100644 --- 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) {