Skip to content

dns: fix off-by-four in EDNS0 OWNER Option#4955

Merged
gpotter2 merged 1 commit intosecdev:masterfrom
evverx:sleep-proxy-stuff
Mar 29, 2026
Merged

dns: fix off-by-four in EDNS0 OWNER Option#4955
gpotter2 merged 1 commit intosecdev:masterfrom
evverx:sleep-proxy-stuff

Conversation

@evverx
Copy link
Copy Markdown
Contributor

@evverx evverx commented Mar 29, 2026

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 |>

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 29, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.30%. Comparing base (04b99c3) to head (223efad).
⚠️ Report is 1 commits behind head on master.

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     
Files with missing lines Coverage Δ
scapy/layers/dns.py 83.61% <ø> (ø)

... and 9 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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

evverx commented Mar 29, 2026

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
https://github.com/apple-oss-distributions/mDNSResponder/blob/d4658af3f5f291311c6aee4210aa6d39bda82bbe/mDNSCore/DNSCommon.c#L3600

Copy link
Copy Markdown
Member

@gpotter2 gpotter2 left a comment

Choose a reason for hiding this comment

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

Thanks for the great PR ! as always

@gpotter2 gpotter2 merged commit 2b220b9 into secdev:master Mar 29, 2026
24 checks passed
evverx added a commit to evverx/scapy that referenced this pull request Mar 29, 2026
@evverx
Copy link
Copy Markdown
Contributor Author

evverx commented Mar 29, 2026

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.

@evverx
Copy link
Copy Markdown
Contributor Author

evverx commented Mar 29, 2026

Looks like the test certificate expires tomorrow

notAfter=Mar 30 07:38:59 2026 GMT

and the string passed to remainingDays isn't valid so when the PR was merged the local time was used in remainingDays and the result was less than 1. I "fixed" the test locally with

 = 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

@gpotter2
Copy link
Copy Markdown
Member

Haha that's unfortunate, good catch. I think we want to regenerate them and make them 1000years

@evverx
Copy link
Copy Markdown
Contributor Author

evverx commented Mar 29, 2026

Another option would probably be to patch localtime. I opened #4956 to make it pass as time goes by :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants