dns: fix off-by-four in EDNS0 OWNER Option#4955
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4955 +/- ##
=======================================
Coverage 80.29% 80.30%
=======================================
Files 379 379
Lines 93107 93107
=======================================
+ Hits 74763 74768 +5
+ Misses 18344 18339 -5
🚀 New features to boost your workflow:
|
The lengths taken from https://datatracker.ietf.org/doc/html/draft-cheshire-edns0-owner-option-01 are off by four because they include the length of the two-byte EDNS0 option code and the length of the two-byte EDNS0 option length but according to https://datatracker.ietf.org/doc/html/rfc6891#section-6.1.2 the length should include the size of data only so all the lengths should be decreased by 4 to parse the OWNER option containing the wakeup MAC correctly. Without this patch the wakeup MAC gets cut off: ``` >>> EDNS0OWN(b'\x00\x04\x00\x0e\x00\x9b\x11"3DUffUD3"\x11') <EDNS0OWN optcode=Owner optlen=14 v=0 s=155 primary_mac=11:22:33:44:55:66 |<Padding load=b'fUD3"\x11' |>> ``` With this patch it's included as expected ``` <EDNS0OWN optcode=Owner optlen=14 v=0 s=155 primary_mac=11:22:33:44:55:66 wakeup_mac=66:55:44:33:22:11 |> ```
|
mDNSResponder subtracts 4 too in https://github.com/apple-oss-distributions/mDNSResponder/blob/d4658af3f5f291311c6aee4210aa6d39bda82bbe/mDNSCore/mDNSEmbeddedAPI.h#L907-L910 #define DNSOpt_OwnerData_ID_Space (4 + 2 + 6)
#define DNSOpt_OwnerData_ID_Wake_Space (4 + 2 + 6 + 6)
#define DNSOpt_OwnerData_ID_Wake_PW4_Space (4 + 2 + 6 + 6 + 4)
#define DNSOpt_OwnerData_ID_Wake_PW6_Space (4 + 2 + 6 + 6 + 6)
...
#define ValidOwnerLength(X) ( (X) == DNSOpt_OwnerData_ID_Space - 4 || \
(X) == DNSOpt_OwnerData_ID_Wake_Space - 4 || \
(X) == DNSOpt_OwnerData_ID_Wake_PW4_Space - 4 || \
(X) == DNSOpt_OwnerData_ID_Wake_PW6_Space - 4 )to validate the optlen in |
gpotter2
left a comment
There was a problem hiding this comment.
Thanks for the great PR ! as always
This reverts commit 2b220b9.
|
I'm not sure why the tests suddenly started failing with the master branch. I reverted the commit in my fork just to make sure and they still failed. Looks like it has something to do with dates/times. |
|
Looks like the test certificate expires tomorrow notAfter=Mar 30 07:38:59 2026 GMTand the string passed to remainingDays isn't valid so when the PR was merged the local time was used in = Cert class : test remainingDays
assert abs(x.remainingDays("02/12/11")) > 5000
-assert abs(x.remainingDays("Feb 12 10:00:00 2011 Paris, Madrid")) > 1
+assert abs(x.remainingDays("Feb 12 10:00:00 2011 UTC")) > 1 |
|
Haha that's unfortunate, good catch. I think we want to regenerate them and make them 1000years |
|
Another option would probably be to patch localtime. I opened #4956 to make it pass as time goes by :-) |
The lengths taken from
https://datatracker.ietf.org/doc/html/draft-cheshire-edns0-owner-option-01 are off by four because they include the length of the two-byte EDNS0 option code and the length of the two-byte EDNS0 option length but according to https://datatracker.ietf.org/doc/html/rfc6891#section-6.1.2 the length should include the size of data only so all the lengths should be decreased by 4 to parse the OWNER option containing the wakeup MAC correctly.
Without this patch the wakeup MAC gets cut off:
With this patch it's included as expected