From e12e91b90d0c7c56a1619cee922e2d3a9b7e00a8 Mon Sep 17 00:00:00 2001 From: Remi Gacogne Date: Thu, 5 Sep 2024 15:11:21 +0200 Subject: [PATCH 1/2] '{&dns}' is as valid as '{?dns}' in a SVCB's dohpath See for example section 1.2. "Levels and Expression Types" of rfc6570. (cherry picked from commit e74052ea712dffc44565d81536f368db4790f232) --- lib/dns/rdata/in_1/svcb_64.c | 6 ++++-- tests/dns/rdata_test.c | 2 ++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/dns/rdata/in_1/svcb_64.c b/lib/dns/rdata/in_1/svcb_64.c index a99177a14f..5145230f33 100644 --- a/lib/dns/rdata/in_1/svcb_64.c +++ b/lib/dns/rdata/in_1/svcb_64.c @@ -157,7 +157,7 @@ svcb_validate(uint16_t key, isc_region_t *region) { /* * Minimum valid dohpath is "/{?dns}" as * it MUST be relative (leading "/") and - * MUST contain "{?dns}". + * MUST contain "{?dns}" or "{&dns}". */ if (region->length < 7) { return DNS_R_FORMERR; @@ -172,8 +172,10 @@ svcb_validate(uint16_t key, isc_region_t *region) { { return DNS_R_FORMERR; } - /* MUST contain "{?dns}" */ + /* MUST contain "{?dns}" or "{&dns}" */ if (strnstr((char *)region->base, "{?dns}", + region->length) == NULL && + strnstr((char *)region->base, "{&dns}", region->length) == NULL) { return DNS_R_FORMERR; diff --git a/tests/dns/rdata_test.c b/tests/dns/rdata_test.c index d7858c801d..b8a98188d5 100644 --- a/tests/dns/rdata_test.c +++ b/tests/dns/rdata_test.c @@ -2613,6 +2613,8 @@ ISC_RUN_TEST_IMPL(https_svcb) { "1 example.net. key7=\"/{?dns}\""), TEXT_VALID_LOOPCHG(1, "1 example.net. dohpath=/some/path{?dns}", "1 example.net. key7=\"/some/path{?dns}\""), + TEXT_VALID_LOOPCHG(1, "1 example.net. dohpath=/some/path?key=value{&dns}", + "1 example.net. key7=\"/some/path?key=value{&dns}\""), TEXT_INVALID("1 example.com. dohpath=no-slash"), TEXT_INVALID("1 example.com. dohpath=/{?notdns}"), TEXT_INVALID("1 example.com. dohpath=/notvariable"), From 2d55935c6e5a28f55e59ce8546eb017d99562748 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Mon, 9 Sep 2024 15:59:30 +1000 Subject: [PATCH 2/2] Parse the URI template and check for a dns variable The 'dns' variable in dohpath can be in various forms ({?dns}, {dns}, {&dns} etc.). To check for a valid dohpath it ends up being simpler to just parse the URI template rather than looking for all the various forms if substring. (cherry picked from commit af54ef9f5d703ed61c140f3e32e6fab5d88923a1) --- lib/dns/rdata.c | 171 +++++++++++++++++++++++++++++++++++ lib/dns/rdata/in_1/svcb_64.c | 25 +---- tests/dns/rdata_test.c | 45 ++++++++- 3 files changed, 212 insertions(+), 29 deletions(-) diff --git a/lib/dns/rdata.c b/lib/dns/rdata.c index d986279ebe..f30893b9d4 100644 --- a/lib/dns/rdata.c +++ b/lib/dns/rdata.c @@ -25,6 +25,7 @@ #include #include #include +#include #include #include @@ -602,6 +603,176 @@ typemap_test(isc_region_t *sr, bool allow_empty) { static const char hexdigits[] = "0123456789abcdef"; static const char decdigits[] = "0123456789"; +/* + * A relative URI template that has a "dns" variable. + */ +static bool +validate_dohpath(isc_region_t *region) { + const unsigned char *p; + const unsigned char *v = NULL; + const unsigned char *n = NULL; + unsigned char c; + bool dns = false; + bool wasop = false; + enum { + path, + variable, + percent1, + percent2, + variable_percent1, + variable_percent2, + prefix, + explode + } state = path; + + if (region->length == 0 || *region->base != '/' || + !isc_utf8_valid(region->base, region->length)) + { + return false; + } + + /* + * RFC 6570 URI Template check + "dns" variable. + */ + p = region->base; + while (p < region->base + region->length) { + switch (state) { + case path: + switch (*p++) { + case '{': /*}*/ + state = variable; + wasop = false; + v = p; + break; + case '%': + state = percent1; + break; + default: + break; + } + break; + case variable: + c = *p++; + switch (c) { + case '+': + case '#': + case '.': + case '/': + case ';': + case '?': + case '&': + /* Operators. */ + if (p != v + 1 || wasop) { + return false; + } + wasop = true; + v = p; + break; + case '=': + case '!': + case '@': + case '|': + /* Reserved operators. */ + return false; + case '*': + case ':': + case '}': + case ',': + /* Found the end of the variable name. */ + if (p == (v + 1)) { + return false; + } + /* 'p' has been incremented so 4 not 3 */ + if ((p - v) == 4 && memcmp(v, "dns", 3) == 0) { + dns = true; + } + switch (c) { + case ':': + state = prefix; + n = p; + break; + case /*{*/ '}': + state = path; + break; + case '*': + state = explode; + break; + case ',': + wasop = false; + v = p; + break; + } + break; + case '%': + /* Percent encoded variable name. */ + state = variable_percent1; + break; + default: + /* Valid variable name character? */ + if (c != '_' && !isalnum(c)) { + return false; + } + break; + } + break; + case explode: + switch (*p++) { + case ',': + state = variable; + wasop = false; + v = p; + break; + case /*}*/ '}': + state = path; + break; + default: + return false; + } + break; + /* Check % encoding */ + case percent1: + case percent2: + case variable_percent1: + case variable_percent2: + /* bad percent encoding? */ + if (!isxdigit(*p++)) { + return false; + } + if (state == percent1) { + state = percent2; + } else if (state == percent2) { + state = path; + } else if (state == variable_percent1) { + state = variable_percent2; + } else { + state = variable; + } + break; + case prefix: + c = *p++; + if (!isdigit(c)) { + /* valid number range [1..9999] */ + if ((p == n + 1) || (p - n) > 5 || *n == '0') { + return false; + } + switch (c) { + case ',': + state = variable; + wasop = false; + break; + case /*{*/ '}': + state = path; + break; + default: + return false; + } + } + break; + } + } + return state == path && dns; +} + #include "code.h" #define META 0x0001 diff --git a/lib/dns/rdata/in_1/svcb_64.c b/lib/dns/rdata/in_1/svcb_64.c index 5145230f33..3891ab7664 100644 --- a/lib/dns/rdata/in_1/svcb_64.c +++ b/lib/dns/rdata/in_1/svcb_64.c @@ -154,30 +154,7 @@ svcb_validate(uint16_t key, isc_region_t *region) { case sbpr_base64: break; case sbpr_dohpath: - /* - * Minimum valid dohpath is "/{?dns}" as - * it MUST be relative (leading "/") and - * MUST contain "{?dns}" or "{&dns}". - */ - if (region->length < 7) { - return DNS_R_FORMERR; - } - /* MUST be relative */ - if (region->base[0] != '/') { - return DNS_R_FORMERR; - } - /* MUST be UTF8 */ - if (!isc_utf8_valid(region->base, - region->length)) - { - return DNS_R_FORMERR; - } - /* MUST contain "{?dns}" or "{&dns}" */ - if (strnstr((char *)region->base, "{?dns}", - region->length) == NULL && - strnstr((char *)region->base, "{&dns}", - region->length) == NULL) - { + if (!validate_dohpath(region)) { return DNS_R_FORMERR; } break; diff --git a/tests/dns/rdata_test.c b/tests/dns/rdata_test.c index b8a98188d5..921f3f0591 100644 --- a/tests/dns/rdata_test.c +++ b/tests/dns/rdata_test.c @@ -2609,15 +2609,50 @@ ISC_RUN_TEST_IMPL(https_svcb) { TEXT_INVALID("1 foo.example.com. ( mandatory=key123,key123 " "key123=abc)"), /* dohpath tests */ + TEXT_VALID_LOOPCHG(1, "1 example.net. dohpath=/{dns}", + "1 example.net. key7=\"/{dns}\""), + TEXT_VALID_LOOPCHG(1, "1 example.net. dohpath=/{+dns}", + "1 example.net. key7=\"/{+dns}\""), + TEXT_VALID_LOOPCHG(1, "1 example.net. dohpath=/{#dns}", + "1 example.net. key7=\"/{#dns}\""), + TEXT_VALID_LOOPCHG(1, "1 example.net. dohpath=/{.dns}", + "1 example.net. key7=\"/{.dns}\""), + TEXT_VALID_LOOPCHG(1, "1 example.net. dohpath=\"/{;dns}\"", + "1 example.net. key7=\"/{;dns}\""), TEXT_VALID_LOOPCHG(1, "1 example.net. dohpath=/{?dns}", "1 example.net. key7=\"/{?dns}\""), TEXT_VALID_LOOPCHG(1, "1 example.net. dohpath=/some/path{?dns}", "1 example.net. key7=\"/some/path{?dns}\""), - TEXT_VALID_LOOPCHG(1, "1 example.net. dohpath=/some/path?key=value{&dns}", - "1 example.net. key7=\"/some/path?key=value{&dns}\""), - TEXT_INVALID("1 example.com. dohpath=no-slash"), - TEXT_INVALID("1 example.com. dohpath=/{?notdns}"), - TEXT_INVALID("1 example.com. dohpath=/notvariable"), + TEXT_VALID_LOOPCHG(1, "1 example.net. dohpath=/{dns:9999}", + "1 example.net. key7=\"/{dns:9999}\""), + TEXT_VALID_LOOPCHG(1, "1 example.net. dohpath=/{dns*}", + "1 example.net. key7=\"/{dns*}\""), + TEXT_VALID_LOOPCHG( + 1, "1 example.net. dohpath=/some/path?key=value{&dns}", + "1 example.net. key7=\"/some/path?key=value{&dns}\""), + TEXT_VALID_LOOPCHG(1, + "1 example.net. " + "dohpath=/some/path?key=value{&dns,x*}", + "1 example.net. " + "key7=\"/some/path?key=value{&dns,x*}\""), + TEXT_INVALID("1 example.com. dohpath=not-relative"), + TEXT_INVALID("1 example.com. dohpath=/{?no_dns_variable}"), + TEXT_INVALID("1 example.com. dohpath=/novariable"), + TEXT_INVALID("1 example.com. dohpath=/{?dnsx}"), + /* index too big > 9999 */ + TEXT_INVALID("1 example.com. dohpath=/{?dns:10000}"), + /* index not postive */ + TEXT_INVALID("1 example.com. dohpath=/{?dns:0}"), + /* index leading zero */ + TEXT_INVALID("1 example.com. dohpath=/{?dns:01}"), + /* two operators */ + TEXT_INVALID("1 example.com. dohpath=/{??dns}"), + /* invalid % encoding */ + TEXT_INVALID("1 example.com. dohpath=/%a{?dns}"), + /* invalid % encoding */ + TEXT_INVALID("1 example.com. dohpath=/{?dns,%a}"), + /* incomplete macro */ + TEXT_INVALID("1 example.com. dohpath=/{?dns" /*}*/), TEXT_SENTINEL() };