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

Ignore Path MTU Discovery on UDP server socket #7410

Merged
merged 2 commits into from Apr 23, 2019
Merged

Conversation

@rgacogne
Copy link
Member

@rgacogne rgacogne commented Jan 23, 2019

Short description

It might help prevent Path MTU poisoning attacks.
Untested, don't merge it yet!

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)
#endif /* IP_PMTUDISC_OMIT */

/* IP_PMTUDISC_DONT disables Path MTU discovery */
SSetsockopt(sockfd, IPPROTO_IP, IP_MTU_DISCOVER, IP_PMTUDISC_DONT);

This comment has been minimized.

@rgacogne

rgacogne Jan 23, 2019
Author Member

We should check that IP_MTUDISC_DONT is defined. Note that unbound sets the don't fragment flag on systems that don't have IP_MTUDISC_DONT: https://github.com/NLnetLabs/unbound/blob/master/services/listen_dnsport.c#L571

This comment has been minimized.

@rgacogne

rgacogne Jan 23, 2019
Author Member

Added the missing check for IP_MTUDISC_DONT. I'm still undecided about setting DF=1.

@rgacogne
Copy link
Member Author

@rgacogne rgacogne commented Jan 23, 2019

As a datapoint, Knot decided against doing this: https://gitlab.labs.nic.cz/knot/knot-dns/issues/467

@mnordhoff
Copy link
Contributor

@mnordhoff mnordhoff commented Jan 23, 2019

Knot decided against it in 2016 when fragmentation attacks weren't in vogue, though.

@rgacogne rgacogne force-pushed the rgacogne:pmtu-dont branch from 60ff55c to c3cdd3f Feb 14, 2019
@rgacogne
Copy link
Member Author

@rgacogne rgacogne commented Feb 14, 2019

Rebased to fix a conflict.

@rgacogne
Copy link
Member Author

@rgacogne rgacogne commented Apr 4, 2019

As pointed out by @hdais NSD did bite the bullet as well recently: https://www.nlnetlabs.nl/bugs-script/show_bug.cgi?id=4235

@rgacogne rgacogne force-pushed the rgacogne:pmtu-dont branch from c3cdd3f to b707672 Apr 18, 2019
@rgacogne rgacogne added this to the rec-4.2.0-beta1 milestone Apr 18, 2019
@rgacogne
Copy link
Member Author

@rgacogne rgacogne commented Apr 18, 2019

Move this PR into the rec-4.2.0-beta1 milestone. Now is the time to speak if you are against this change ;-)

@rgacogne rgacogne merged commit aecec57 into PowerDNS:master Apr 23, 2019
2 checks passed
2 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rgacogne rgacogne deleted the rgacogne:pmtu-dont branch Apr 23, 2019
@jedisct1
Copy link

@jedisct1 jedisct1 commented Mar 26, 2020

Hi Remi,

That change seems to be what made DNSCrypt servers running dnsdist unreliable after they upgraded to version 1.4.0.

As you know since you implemented the protocol, DNSCrypt requires questions sent over UDP to be as large as responses, using padding.

Blocking fragmented questions prevents large responses from being received.

Ignoring fragments in responses from authoritative servers is fine, but even with unencrypted queries, I'm not sure that there is any value in dropping fragments on the server socket. A fragment attack would just allow the question to be modified, but a stub resolver would ignore a response for a different query.

@rgacogne
Copy link
Member Author

@rgacogne rgacogne commented Mar 26, 2020

Hi Frank,

Thanks a lot for reporting this! I have to admit I had completely forgotten that propriety of DNSCrypt when I submitted this change, sorry about that!
I'll re-evaluate whether it makes sense to keep setting setSocketIgnorePMTU() on our incoming sockets. At the very least I'll disable it for DNSCrypt binds.

@jedisct1
Copy link

@jedisct1 jedisct1 commented Mar 26, 2020

Thank you so much Remi!

/cc @welwood08

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants