Skip to content

dns_msg: skip RDLENGTH_LENGTH field when skipping record#20857

Merged
benpicco merged 5 commits intoRIOT-OS:masterfrom
benpicco:dns_msg-fix_skip
Sep 27, 2024
Merged

dns_msg: skip RDLENGTH_LENGTH field when skipping record#20857
benpicco merged 5 commits intoRIOT-OS:masterfrom
benpicco:dns_msg-fix_skip

Conversation

@benpicco
Copy link
Copy Markdown
Contributor

@benpicco benpicco commented Sep 9, 2024

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

@github-actions github-actions bot added Area: network Area: Networking Area: sys Area: System labels Sep 9, 2024
@benpicco 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
@riot-ci
Copy link
Copy Markdown

riot-ci commented Sep 9, 2024 •
edited
Loading

Murdock results

✔️ PASSED

0463b27 unittests: add test for dns_msg

Success Failures Total Runtime
10197 0 10197 16m:24s

Artifacts

@benpicco benpicco requested a review from kaspar030 September 9, 2024 11:23
@benpicco benpicco mentioned this pull request Sep 9, 2024
Open
@miri64
Copy link
Copy Markdown
Member

miri64 commented Sep 9, 2024 •
edited
Loading

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
Copy link
Copy Markdown
Contributor Author

benpicco commented Sep 10, 2024 •
edited
Loading

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
Copy link
Copy Markdown
Member

miri64 commented Sep 11, 2024

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
Copy link
Copy Markdown
Contributor Author

benpicco commented Sep 11, 2024

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.

@github-actions github-actions bot added the Area: tests Area: tests and testing framework label Sep 11, 2024
@benpicco
Copy link
Copy Markdown
Contributor Author

benpicco commented Sep 12, 2024

rebased to to master because of #20860

miri64
miri64 previously requested changes Sep 13, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@benpicco benpicco requested review from bergzand and chrysn September 27, 2024 14:08
@miri64 miri64 dismissed their stale review September 27, 2024 15:21

Addressed it myself.

@benpicco
Copy link
Copy Markdown
Contributor Author

benpicco commented Sep 27, 2024

Thank you for addressing and documenting the test vectors!

@benpicco @miri64
unittests: add test for dns_msg
0463b27 
Co-Authored-By: Martine Lenders <[email protected]>
@miri64
Copy link
Copy Markdown
Member

miri64 commented Sep 27, 2024

Fixed a spelling mistake in the name 😅

@benpicco benpicco added this pull request to the merge queue Sep 27, 2024
Merged via the queue into RIOT-OS:master with commit 9bdb697 Sep 27, 2024
@benpicco benpicco deleted the dns_msg-fix_skip branch September 28, 2024 08:34
@benpicco benpicco added this to the Release 2024.10 milestone Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@miri64 miri64 miri64 approved these changes

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann

@kaspar030 kaspar030 Awaiting requested review from kaspar030

@Teufelchen1 Teufelchen1 Awaiting requested review from Teufelchen1 Teufelchen1 is a code owner

@chrysn chrysn Awaiting requested review from chrysn

@bergzand bergzand Awaiting requested review from bergzand

Assignees

No one assigned

Labels

Area: network Area: Networking Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

dns_msg_parse_reply() fails for response from DNS64 service

3 participants

@benpicco @riot-ci @miri64