fix(DnsPinMiddlware): only pass punycode/ASCII hostname to dns_get_record
#56197
+109
−1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…cord `dns_get_record()` only supports ASCII/Punycode input. `idn_to_utf8()` converts Punycode/ASCII hostnames to their Unicode form. If we pass it raw Unicode (e.g., "bücher.com"), it returns it unchanged. If you give it Punycode (e.g., "xn--bcher-kva.com"), it returns the Unicode (e.g., "bücher.com"). Neither of which will work with dns_get_record(). Passing either raw Unicode or the output of idn_to_utf8() on a punycode string to dns_get_record() will result in DNS resolution failure for IDNs. Only ASCII/punycode should be passed to the resolver. Signed-off-by: Josh <josh.t.richards@gmail.com>
Add tests for DNS pinning middleware handling of Punycode and Unicode hostnames. Signed-off-by: Josh <josh.t.richards@gmail.com>
|
/backport to stable32 |
|
/backport to stable31 |
Signed-off-by: Josh <josh.t.richards@gmail.com>
Signed-off-by: Josh <josh.t.richards@gmail.com>
kesselb
approved these changes
Nov 4, 2025
| } | ||
|
|
||
| $targetIps = $this->dnsResolve(idn_to_utf8($hostName), 0); | ||
| $targetIps = $this->dnsResolve(idn_to_ascii($hostName, IDNA_DEFAULT, INTL_IDNA_VARIANT_UTS46, null), 0); |
There was a problem hiding this comment.
Suggested change
| $targetIps = $this->dnsResolve(idn_to_ascii($hostName, IDNA_DEFAULT, INTL_IDNA_VARIANT_UTS46, null), 0); | |
| $targetIps = $this->dnsResolve(idn_to_ascii($hostName, IDNA_DEFAULT, INTL_IDNA_VARIANT_UTS46), 0); |
| ->with( | ||
| $this->callback(function ($hostname) use ($punycodeHost) { | ||
| // Should never be raw Unicode here! | ||
| $this->assertEquals($punycodeHost, $hostname, "dnsGetRecord should be called with Punycode ASCII host"); |
There was a problem hiding this comment.
Suggested change
| $this->assertEquals($punycodeHost, $hostname, "dnsGetRecord should be called with Punycode ASCII host"); | |
| $this->assertEquals($punycodeHost, $hostname, 'dnsGetRecord should be called with Punycode ASCII host'); |
For a happy linter ;)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
tldr: The current use of
idn_to_utf8()in this context breaks DNS resolution for both Unicode IDNs and explicit punycode hostnames. It also doesn't seem to match the intention.Likely also fixes #53185
Details
dns_get_record()only supports ASCII/Punycode input.idn_to_utf8()converts Punycode/ASCII hostnames to their Unicode form.If we pass it raw Unicode (e.g., "bücher.com"), it returns it unchanged. If you give it Punycode (e.g., "xn--bcher-kva.com"), it returns the Unicode (e.g., "bücher.com").
Neither of which will work with
dns_get_record().Passing either raw Unicode or the output of
idn_to_utf8()on a punycode string todns_get_record()will result in DNS resolution failure for IDNs. Only ASCII/punycode should be passed to the resolver.Not caught by unit tests previously due to ASCII-only hostnames, mocked DNS, etc.
TODO
server/lib/private/Security/RemoteHostValidator.php
Line 38 in 9581230
Checklist
3. to review, feature component)stable32)