Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(m)DNS: improve (m)dnsd defaults and behavior #4390

Merged
merged 4 commits into from
May 26, 2024
Merged

Conversation

gpotter2
Copy link
Member

  • fix various bugs in dnsd / mdnsd
  • make it closer to real life (aa, ttl, id, opt)
  • add missing mDNS specific bits

fixes #4388

Copy link

codecov bot commented May 15, 2024

Codecov Report

Attention: Patch coverage is 71.69811% with 30 lines in your changes are missing coverage. Please review.

Project coverage is 81.56%. Comparing base (fa94fe3) to head (420665c).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4390      +/-   ##
==========================================
- Coverage   82.20%   81.56%   -0.64%     
==========================================
  Files         352      352              
  Lines       83665    83953     +288     
==========================================
- Hits        68775    68476     -299     
- Misses      14890    15477     +587     
Files Coverage Δ
scapy/arch/__init__.py 63.76% <66.66%> (-8.96%) ⬇️
scapy/layers/dns.py 87.63% <71.84%> (-0.63%) ⬇️

... and 26 files with indirect coverage changes

@gpotter2 gpotter2 force-pushed the mdnsd branch 2 times, most recently from a5737ee to cba2c13 Compare May 15, 2024 00:31
test/answering_machines.uts Outdated Show resolved Hide resolved
Set to False to disable, None to mirror the interface's IP.
Defaults to None, unless 'match' is used, then it defaults to
False.
:param joker6: default IPv6 for unresolved domains (Default: False)
set to False to disable, None to mirror the interface's IPv6.
Copy link
Contributor

Choose a reason for hiding this comment

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

(It's unrelated to this PR but joker6=None doesn't seem to be fully compatible with mDNS. It doesn't extract link-local addresses so no replies are sent on machines with no global addresses). According to https://datatracker.ietf.org/doc/html/rfc6762#section-6.2 all the valid addresses should be sent:

When a Multicast DNS responder sends a Multicast DNS response message
containing its own address records, it MUST include all addresses
that are valid on the interface on which it is sending the message,
and MUST NOT include addresses that are not valid on that interface
(such as addresses that may be configured on the host's other
interfaces)

@gpotter2
Copy link
Member Author

I've tweaked things quite a bit, trying to fix #4385 (comment). I've expanded the docstrings to add some usage examples.

The behavior on machines with multiple interfaces is very buggy.. that's because Scapy handles very poorly multicast link-layer addresses. This requires a rework but that's out of scope.

In the meantime, this code is usable on a machine with multiple interfaces using

conf.route.add(net="224.0.0.0/8", gw="<the gateway>", metric=1)

guedou
guedou previously approved these changes May 20, 2024
scapy/layers/dns.py Outdated Show resolved Hide resolved
@evverx
Copy link
Contributor

evverx commented May 22, 2024

It seems I can still reproduce #4385 (comment) when mdnsd receives A and AAAA queries from avahi. It could be I screwed something up though so I'll try to double-check.

@gpotter2
Copy link
Member Author

gpotter2 commented May 22, 2024

I'm personally able to have avahi-resolve work, but only on a non-loopback interface (with 2 machines).
It seems to never be receiving any packet back. (maybe an issue of using lo on L2 with raw sockets?)

Check the source MAC though. I still need to ip route add 224.0.0.0/4 dev x.x.x.x metric 1. (this however is a Scapy issue..)

In all cases I'm really interested if you have feedback on this one :D

rrname=rq.qname,
nextname=rq.qname,
typebitmaps=RRlist2bitmap([rq.qtype]),
))
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a nice touch! According to https://datatracker.ietf.org/doc/html/rfc6762#section-6.1 it should go to the additional record section

the
responder MAY also include an NSEC record in the Additional Record
Section indicating the nonexistence of other rrtypes for that name
and rrclass

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree but Windows put it in the answers, so I copied that behavior

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. In practice it should work either way. Though I think I saw libraries that can't handle additional SRV and TXT records described in https://datatracker.ietf.org/doc/html/rfc6763#section-12 when they are put in the answer section.

@evverx
Copy link
Contributor

evverx commented May 23, 2024

Turns out I screwed it up. I tested it with #4385. With this PR applied avahi-resolve -n works. Sorry!

@evverx
Copy link
Contributor

evverx commented May 23, 2024

I've just tested it with the loopback interface using

mdnsd(iface='lo', joker='192.168.56.100')

and it works too as far as I can see. allow-interfaces= and deny-interfaces= in avahi-daemon.conf should allow the loopback interface though.

@gpotter2
Copy link
Member Author

gpotter2 commented May 25, 2024

When it's fine by you @evverx, I'll proceed with merging this :) I'm unsure if I've addressed all your comments / remarks.

@evverx
Copy link
Contributor

evverx commented May 26, 2024

I tested it with avahi, mDNSResponder and systemd-resolved and it works so personally I think it should be good to go :-)

@gpotter2 gpotter2 merged commit a795ad6 into secdev:master May 26, 2024
21 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFE: mDNS unicast-response and cache-flush bits
3 participants