dns_msg: skip RDLENGTH_LENGTH field when skipping record #20857

benpicco · 2024-09-09T10:08:16Z

Contribution description

dns_msg skips unknown records, but the length record length field was only added when the record was not skipped - this caused the next record to be started 2 bytes short.

Testing procedure

enable DNS with DNS64 server

USEMODULE += sock_dns
USEMODULE += auto_init_sock_dns
CFLAGS += -DCONFIG_AUTO_INIT_SOCK_DNS_SERVER_ADDR=\"2001:67c:2b0::6\"
CFLAGS += -DCONFIG_DNS_MSG_LEN=256 # response msg is 202 bytes

We can resolve an IPv4 host behind NAT64 now 🎉

2024-09-09 12:00:59,181 # > ping global.azure-devices-provisioning.net
2024-09-09 12:00:59,341 # 12 bytes from 2001:67c:2b0:db32::1432:418d: icmp_seq=0 ttl=105 time=84.760 ms
2024-09-09 12:01:00,336 # 12 bytes from 2001:67c:2b0:db32::1432:418d: icmp_seq=1 ttl=105 time=80.033 ms
2024-09-09 12:01:01,337 # 12 bytes from 2001:67c:2b0:db32::1432:418d: icmp_seq=2 ttl=105 time=79.999 ms
2024-09-09 12:01:01,337 # 
2024-09-09 12:01:01,338 # --- global.azure-devices-provisioning.net PING statistics ---
2024-09-09 12:01:01,339 # 3 packets transmitted, 3 packets received, 0% packet loss
2024-09-09 12:01:01,340 # round-trip min/avg/max = 79.999/81.597/84.760 ms

Issues/PRs references

fixes #20355

riot-ci · 2024-09-09T10:27:27Z

Murdock results

✔️ PASSED

bc8ce1c unittests: add test for dns_msg

Success	Failures	Total	Runtime
10197	0	10197	21m:49s

Artifacts

Documentation preview

miri64 · 2024-09-09T14:59:57Z

Can you add your test vector to https://github.com/RIOT-OS/RIOT/blob/master/tests/net/gnrc_sock_dns/tests-with-config/01-run.py to prevent future regressions, please? I think just adding the hexdump in a scapy DNS object should be enough, but, as we learned during the summit: It always pays to learn a little scapy for testing network stuff ;-).

benpicco · 2024-09-10T17:20:27Z

Ugh I think that test is somewhat broken. All requests after the first test_success result in a timeout.
The test still succeeds, because it expects all but the first test to fail, but if you do

--- a/tests/net/gnrc_sock_dns/tests-with-config/01-run.py
+++ b/tests/net/gnrc_sock_dns/tests-with-config/01-run.py
@@ -296,6 +296,7 @@ def testfunc(child):
 
         run(test_success)
         run(test_timeout)
+        run(test_success)
         run(test_too_short_response)
         run(test_qdcount_too_large1)
         run(test_qdcount_too_large2)

you'll find that the test isn't testing anything at all. (also on master)

miri64 · 2024-09-11T07:13:15Z

Ugh I think that test is somewhat broken. All requests after the first test_success result in a timeout.
The test still succeeds, because it expects all but the first test to fail, but if you do […] you'll find that the test isn't testing anything at all. (also on master)

Yeah, I was surprised that there is no dedicated test for dns_msg either. But I guess that has historical reasons, since it used to be part of sock_dns. Anyway, would be nice to have some test to prevent regression.

benpicco · 2024-09-11T12:18:47Z

I created unittests/tests-dns_msg to track regressions.
The test fails on master but succeeds with this PR.

It currently only contains positive tests, but could/should be extended with malicious data in the future to better stress the parser.

But IMHO that's out of the scope of a bugfix PR.

fixes RIOT-OS#20355

benpicco · 2024-09-12T09:00:48Z

rebased to to master because of #20860

miri64

IMHO better to use example values in the tests (actual values have the tendency to get stuck, e.g.), other than that, I would ACK.

miri64 · 2024-09-13T09:43:35Z

sys/net/application_layer/dns/msg.c

 /* handle DNS Message Compression */
- if (*bufpos >= 192) {
+ if (*bufpos & 0xc0) {


Not sure what this change has to do with debug output, but I agree it is more clearer what is checked for. :-)

Yea I first read this as bufpos >= 192 and was confused as to why compression would start once the answer reaches a certain size…

miri64 · 2024-09-13T09:55:33Z

tests/unittests/tests-dns_msg/tests-dns_msg.c

+ 0x00, 0x00, 0x00, 0x00, 0x06, 0x67, 0x6f, 0x6f,
+ 0x67, 0x6c, 0x65, 0x03, 0x63, 0x6f, 0x6d, 0x00,
+ 0x00, 0x1c, 0x00, 0x01, 0xc0, 0x0c, 0x00, 0x1c,
+ 0x00, 0x01, 0x00, 0x00, 0x01, 0x2c, 0x00, 0x10,
+ 0x2a, 0x00, 0x14, 0x50, 0x40, 0x05, 0x08, 0x0b,
+ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x20, 0x0e,
+ };
+ const uint8_t addr[] = {
+ 0x2a, 0x00, 0x14, 0x50, 0x40, 0x05, 0x08, 0x0b,
+ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x20, 0x0e,


Is there a reason why you need to use and name specific company names and existing addresses in the tests? I think example.org-based names and 2001:db8::/32-prefixed addresses should also work and have less potential of hassle (may it be legal, technical or otherwise) later on.

I don't know how to generate synthetic DNS response messages. Also having real-world test data in there is a good way to verify our implementation against what's out there (and previously triggered the bug) - I don't think there is copyright on DNS response messages.

I don't know how to generate synthetic DNS response messages.

scapy? Alternatively, DNS is a well known format, so you should be able to switch out the bytes manully. Here's how I would do it with the scapy shell for dns_msg.

>>> dns = DNS(bytes([ 0x00, 0x00, 0x81, 0x80, 0x00, 0x01, 0x00, 0x01, ...: 0x00, 0x00, 0x00, 0x00, 0x06, 0x67, 0x6f, 0x6f, ...: 0x67, 0x6c, 0x65, 0x03, 0x63, 0x6f, 0x6d, 0x00, ...: 0x00, 0x1c, 0x00, 0x01, 0xc0, 0x0c, 0x00, 0x1c, ...: 0x00, 0x01, 0x00, 0x00, 0x01, 0x2c, 0x00, 0x10, ...: 0x2a, 0x00, 0x14, 0x50, 0x40, 0x05, 0x08, 0x0b, ...: 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x20, 0x0e,])) >>> >>> dns.qd <DNSQR qname='google.com.' qtype=AAAA qclass=IN |> >>> dns.qd[0] <DNSQR qname='google.com.' qtype=AAAA qclass=IN |> >>> dns.qd[0].qname b'google.com.' >>> dns.qd[0].qname = 'example.org' >>> dns.an[0] <DNSRR rrname='google.com.' type=AAAA rclass=IN ttl=300 rdlen=16 rdata=2a00:1450:4005:80b::200e |> >>> dns.an[0].rrname = 'example.org' >>> dns.an[0].rdata = '2001:db8:4005:80b::200e' >>> dns.an <DNSRR rrname='example.org.' type=AAAA rclass=IN ttl=300 rdlen=16 rdata=2001:db8:4005:80b::200e |> >>> bytes(dns) b'\x00\x00\x81\x80\x00\x01\x00\x01\x00\x00\x00\x00\x07example\x03org\x00\x00\x1c\x00\x01\x07example\x03org\x00\x00\x1c\x00\x01\x00\x00\x01,\x00\x10 \x01\r\xb8@\x05\x08\x0b\x00\x00\x00\x00\x00\x00 \x0e' >>> str([f"0x{b:02x}" for b in bytes(dns)]).replace("'", "").replace("[", "{").replace("]", "}") '{0x00, 0x00, 0x81, 0x80, 0x00, 0x01, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x07, 0x65, 0x78, 0x61, 0x6d, 0x70, 0x6c, 0x65, 0x03, 0x6f, 0x72, 0x67, 0x00, 0x00, 0x1c, 0x00, 0x01, 0x07, 0x65, 0x78, 0x61, 0x6d, 0x70, 0x6c, 0x65, 0x03, 0x6f, 0x72, 0x67, 0x00, 0x00, 0x1c, 0x00, 0x01, 0x00, 0x00, 0x01, 0x2c, 0x00, 0x10, 0x20, 0x01, 0x0d, 0xb8, 0x40, 0x05, 0x08, 0x0b, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x20, 0x0e}'

…but why?

Arghs, just realized that this did not compress the name... use

str([f"0x{b:02x}" for b in bytes(dns.compress())]).replace("'", "").replace("[", "{").replace("]", "}")

in the last line

This seems like a lot of busywork for no real gain.
Why not use real world DNS messages to test against?

…but why?

Because it is unnecessary meta-data.

It was unnecessary meta-data (two CNAME records) that made the parsing fail in the first place!

How? Your OP doesn't mention them. And even then, afair your original bug analysis it was the length of the CNAME record that caused the issue, not the value itself.

benpicco requested review from miri64 and PeterKietzmann as code owners September 9, 2024 10:08

github-actions bot added Area: network Area: Networking Area: sys Area: System labels Sep 9, 2024

benpicco added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 9, 2024

benpicco requested a review from kaspar030 September 9, 2024 11:23

benpicco mentioned this pull request Sep 9, 2024

[POC] Azure Integration in RIOT OS #20223

Open

benpicco force-pushed the dns_msg-fix_skip branch from 0744a79 to 352c37d Compare September 9, 2024 12:56

benpicco force-pushed the dns_msg-fix_skip branch from 352c37d to 3c8e44d Compare September 10, 2024 17:22

github-actions bot added the Area: tests Area: tests and testing framework label Sep 11, 2024

benpicco added 5 commits September 12, 2024 11:00

dns_msg: skip RDLENGTH_LENGTH field when skipping record

74356c9

fixes RIOT-OS#20355

dns_msg: add debug output

305b5db

dns_msg: rename addrlen -> rdlen

bc2ad62

sock_dns: add debug output

344d4b8

unittests: add test for dns_msg

bc8ce1c

benpicco force-pushed the dns_msg-fix_skip branch from 5f5b6c7 to bc8ce1c Compare September 12, 2024 09:00

miri64 requested changes Sep 13, 2024

View reviewed changes

benpicco requested a review from Teufelchen1 September 13, 2024 11:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dns_msg: skip RDLENGTH_LENGTH field when skipping record #20857

dns_msg: skip RDLENGTH_LENGTH field when skipping record #20857

benpicco commented Sep 9, 2024

riot-ci commented Sep 9, 2024 •

edited

Loading

miri64 commented Sep 9, 2024 •

edited

Loading

benpicco commented Sep 10, 2024 •

edited

Loading

miri64 commented Sep 11, 2024

benpicco commented Sep 11, 2024

benpicco commented Sep 12, 2024

miri64 left a comment

miri64 Sep 13, 2024

benpicco Sep 13, 2024

miri64 Sep 13, 2024

benpicco Sep 13, 2024 •

edited

Loading

miri64 Sep 13, 2024

benpicco Sep 13, 2024

miri64 Sep 13, 2024

benpicco Sep 13, 2024

miri64 Sep 13, 2024

benpicco Sep 13, 2024

miri64 Sep 13, 2024

dns_msg: skip RDLENGTH_LENGTH field when skipping record #20857

Are you sure you want to change the base?

dns_msg: skip RDLENGTH_LENGTH field when skipping record #20857

Conversation

benpicco commented Sep 9, 2024

Contribution description

Testing procedure

enable DNS with DNS64 server

Issues/PRs references

riot-ci commented Sep 9, 2024 • edited Loading

Murdock results

Artifacts

miri64 commented Sep 9, 2024 • edited Loading

benpicco commented Sep 10, 2024 • edited Loading

miri64 commented Sep 11, 2024

benpicco commented Sep 11, 2024

benpicco commented Sep 12, 2024

miri64 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benpicco Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riot-ci commented Sep 9, 2024 •

edited

Loading

miri64 commented Sep 9, 2024 •

edited

Loading

benpicco commented Sep 10, 2024 •

edited

Loading

benpicco Sep 13, 2024 •

edited

Loading