From a0bd1e67cd224ead5971140b1821a3014246f00d Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Tue, 30 Apr 2024 01:22:39 -0700 Subject: [PATCH 1/5] add a test method to print QP keys add a method qp_test_printkey() to print the name encoded in a QP key. --- tests/include/tests/qp.h | 6 ++++++ tests/libtest/qp.c | 12 ++++++++++++ 2 files changed, 18 insertions(+) diff --git a/tests/include/tests/qp.h b/tests/include/tests/qp.h index d0d7357315..34a84491d8 100644 --- a/tests/include/tests/qp.h +++ b/tests/include/tests/qp.h @@ -84,3 +84,9 @@ qp_test_dumptrie(dns_qpreadable_t qp); */ void qp_test_dumpdot(dns_qp_t *qp); + +/* + * Print the name encoded in a QP key. + */ +void +qp_test_printkey(const dns_qpkey_t key, size_t keylen); diff --git a/tests/libtest/qp.c b/tests/libtest/qp.c index d6a1d3055f..fb65962bbf 100644 --- a/tests/libtest/qp.c +++ b/tests/libtest/qp.c @@ -23,6 +23,7 @@ #include #include +#include #include #include #include @@ -338,4 +339,15 @@ qp_test_dumpdot(dns_qp_t *qp) { printf("}\n"); } +void +qp_test_printkey(const dns_qpkey_t key, size_t keylen) { + dns_fixedname_t fn; + dns_name_t *n = dns_fixedname_initname(&fn); + char txt[DNS_NAME_FORMATSIZE]; + + dns_qpkey_toname(key, keylen, n); + dns_name_format(n, txt, sizeof(txt)); + printf("%s%s\n", txt, dns_name_isabsolute(n) ? "." : ""); +} + /**********************************************************************/ From ada46eb9f584b59046734bfbe1a71fe77cfd14e9 Mon Sep 17 00:00:00 2001 From: Matthijs Mekking Date: Tue, 30 Apr 2024 17:36:47 +0200 Subject: [PATCH 2/5] Add a unit test case for converting \000\009 Sanity checking that this domain converts to the key I am expecting. Also fix some of the other names that had trailing 0x02 bits. --- tests/dns/qp_test.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/tests/dns/qp_test.c b/tests/dns/qp_test.c index efd1470360..dc053f8b90 100644 --- a/tests/dns/qp_test.c +++ b/tests/dns/qp_test.c @@ -56,9 +56,14 @@ ISC_RUN_TEST_IMPL(qpkey_name) { }, { .namestr = "\\000", - .key = { 0x03, 0x03, 0x02, 0x02 }, + .key = { 0x03, 0x03, 0x02 }, .len = 3, }, + { + .namestr = "\\000\\009", + .key = { 0x03, 0x03, 0x03, 0x0c, 0x02 }, + .len = 5, + }, { .namestr = "com", .key = { 0x16, 0x22, 0x20, 0x02 }, @@ -72,19 +77,19 @@ ISC_RUN_TEST_IMPL(qpkey_name) { { .namestr = "example.com.", .key = { 0x02, 0x16, 0x22, 0x20, 0x02, 0x18, 0x2b, 0x14, - 0x20, 0x23, 0x1f, 0x18, 0x02, 0x02 }, + 0x20, 0x23, 0x1f, 0x18, 0x02 }, .len = 13, }, { .namestr = "example.com", .key = { 0x16, 0x22, 0x20, 0x02, 0x18, 0x2b, 0x14, 0x20, - 0x23, 0x1f, 0x18, 0x02, 0x02 }, + 0x23, 0x1f, 0x18, 0x02 }, .len = 12, }, { .namestr = "EXAMPLE.COM", .key = { 0x16, 0x22, 0x20, 0x02, 0x18, 0x2b, 0x14, 0x20, - 0x23, 0x1f, 0x18, 0x02, 0x02 }, + 0x23, 0x1f, 0x18, 0x02 }, .len = 12, }, }; @@ -128,6 +133,9 @@ ISC_RUN_TEST_IMPL(qpkey_sort) { } testcases[] = { { .namestr = "." }, { .namestr = "\\000." }, + { .namestr = "\\000.\\000." }, + { .namestr = "\\000\\009." }, + { .namestr = "\\007." }, { .namestr = "example.com." }, { .namestr = "EXAMPLE.COM." }, { .namestr = "www.example.com." }, From a23ce2c53c9bb6ba24c721228e941a8e3e2f1873 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Tue, 30 Apr 2024 00:16:45 -0700 Subject: [PATCH 3/5] add another test case for an incorrect QP iterator position build a database tree with names containing control characters, search for another control character, and verify the iterator is positioned correctly. --- tests/dns/qp_test.c | 45 ++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 42 insertions(+), 3 deletions(-) diff --git a/tests/dns/qp_test.c b/tests/dns/qp_test.c index dc053f8b90..9cfab6509b 100644 --- a/tests/dns/qp_test.c +++ b/tests/dns/qp_test.c @@ -38,6 +38,8 @@ #include #include +bool verbose = false; + ISC_RUN_TEST_IMPL(qpkey_name) { struct { const char *namestr; @@ -106,6 +108,9 @@ ISC_RUN_TEST_IMPL(qpkey_name) { dns_test_namefromstring(testcases[i].namestr, &fn1); } len = dns_qpkey_fromname(key, in); + if (verbose) { + qp_test_printkey(key, len); + } assert_int_equal(testcases[i].len, len); assert_memory_equal(testcases[i].key, key, len); @@ -607,8 +612,20 @@ check_predecessors(dns_qp_t *qp, struct check_predecessors check[]) { for (int i = 0; check[i].query != NULL; i++) { dns_qpiter_t it; - namestr = NULL; dns_test_namefromstring(check[i].query, &fn1); + + /* + * normalize the expected predecessor name, in + * case it has escaped characters, so we can compare + * apples to apples. + */ + dns_fixedname_t fn3; + dns_name_t *expred = dns_fixedname_initname(&fn3); + char *predstr = NULL; + dns_test_namefromstring(check[i].predecessor, &fn3); + result = dns_name_tostring(expred, &predstr, mctx); + assert_int_equal(result, ISC_R_SUCCESS); + result = dns_qp_lookup(qp, name, NULL, &it, NULL, NULL, NULL); #if 0 fprintf(stderr, "%s: expected %s got %s\n", check[i].query, @@ -638,15 +655,16 @@ check_predecessors(dns_qp_t *qp, struct check_predecessors check[]) { result = dns_name_tostring(pred, &namestr, mctx); #if 0 fprintf(stderr, "... expected predecessor %s got %s\n", - check[i].predecessor, namestr); + predstr, namestr); #endif assert_int_equal(result, ISC_R_SUCCESS); - assert_string_equal(namestr, check[i].predecessor); + assert_string_equal(namestr, predstr); #if 0 fprintf(stderr, "%d: remaining names after %s:\n", i, namestr); #endif isc_mem_free(mctx, namestr); + isc_mem_free(mctx, predstr); int j = 0; while (dns_qpiter_next(&it, name, NULL, NULL) == ISC_R_SUCCESS) @@ -849,6 +867,27 @@ ISC_RUN_TEST_IMPL(fixiterator) { check_predecessors(qp, check3); dns_qp_destroy(&qp); + + const char insert4[][64] = { ".", "\\000.", "\\000.\\000.", + "\\000\\009.", "" }; + i = 0; + + dns_qp_create(mctx, &string_methods, NULL, &qp); + while (insert4[i][0] != '\0') { + insert_str(qp, insert4[i++]); + } + + static struct check_predecessors check4[] = { + { "\\007.", "\\000\\009.", DNS_R_PARTIALMATCH, 0 }, + { "\\009.", "\\000\\009.", DNS_R_PARTIALMATCH, 0 }, + { "\\045.", "\\000\\009.", DNS_R_PARTIALMATCH, 0 }, + { "\\044.", "\\000\\009.", DNS_R_PARTIALMATCH, 0 }, + { "\\000.", ".", ISC_R_SUCCESS, 3 }, + { NULL, NULL, 0, 0 }, + }; + + check_predecessors(qp, check4); + dns_qp_destroy(&qp); } ISC_TEST_LIST_START From f81bf6bafdc3084111e148d2fd6352f28d3b6506 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Tue, 30 Apr 2024 14:23:43 -0700 Subject: [PATCH 4/5] handle QP lookups involving escaped characters better in QP keys, characters that are not common in DNS names are encoded as two-octet sequences. this caused a glitch in iterator positioning when some lookups failed. consider the case where we're searching for "\009" (represented in a QP key as {0x03, 0x0c}) and a branch exists for "\000" (represented as {0x03, 0x03}). we match on the 0x03, and continue to search down. at the point where we find we have no match, we need to pop back up to the branch before the 0x03 - which may be multiple levels up the stack - before we position the iterator. --- lib/dns/qp.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/lib/dns/qp.c b/lib/dns/qp.c index 0329f424cb..e8ff8f66a7 100644 --- a/lib/dns/qp.c +++ b/lib/dns/qp.c @@ -2167,7 +2167,21 @@ fix_iterator(dns_qpreader_t *qp, dns_qpiter_t *iter, dns_qpkey_t search, } if (is_branch(n)) { - iter->stack[iter->sp--] = NULL; + /* + * Pop up until we reach a branch that + * differs earlier than the position we're + * looking at. Note that because of escaped + * characters, this might require popping + * more than once. + */ + dns_qpnode_t *last = iter->stack[iter->sp]; + while (iter->sp > 0 && + to < branch_key_offset(last)) + { + n = last; + iter->stack[iter->sp--] = NULL; + last = iter->stack[iter->sp]; + } greatest_leaf(qp, n, iter); } From 9bbba20fbff14ee42083d68044b94a8bcb8cf963 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Tue, 30 Apr 2024 15:22:03 -0700 Subject: [PATCH 5/5] CHANGES for [GL #4702] --- CHANGES | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGES b/CHANGES index 8ca7f9e5fd..5a4b6a0eed 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,8 @@ +6381. [bug] dns_qp_lookup() could position the iterator at the + wrong predecessor when searching for names with + uncommon characters, which are encoded as two-octet + sequences in QP trie keys. [GL #4702] + 6380. [func] Queries and responses now emit distinct dnstap entries for DoT and DoH. [GL #4523]