[Idn] Bound the Punycode decoder input length - #646
Merged
Conversation
punycodeDecode() inserts every decoded character into an array with array_splice(), making the decode quadratic in the payload length. The resulting label size is only checked after decoding, so a crafted multi-kilobyte "xn--" label reaching idn_to_utf8()/idn_to_ascii() spends minutes of CPU before being rejected: 5 KB took 36 ms, 80 KB took 12 s and 200 KB took 177 s on a single call. Valid ACE labels are limited to 63 bytes, so payloads beyond a generous 1024-byte bound are always invalid input. Reject them up front with ERROR_PUNYCODE without invoking the decoder, capping the worst case at about half a million elementary operations (~milliseconds) per label. This mirrors ext-intl, which never decodes absurdly long labels: native ICU rejects a 5 KB domain instantly. Labels above the DNS limit but below this bound still decode exactly as before (ToUnicode reports their decoded form even though they are invalid), keeping the UTS symfony#46 spec fixtures passing.
Contributor
Author
|
The single failing job (PHP 8.6 + APC) is pre-existing CI breakage on |
Member
|
Opened #648 as a companion for the encoder, which has the mirror problem: |
Member
|
Thank you @iliaal. |
nicolas-grekas
added a commit
that referenced
this pull request
Aug 24, 2026
…would refuse (nicolas-grekas) This PR was merged into the 1.x branch. Discussion ---------- [Intl-Idn] Don't Punycode-encode domains the intl extension would refuse | Q | A | ------------- | --- | Branch? | 1.x | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | - | License | MIT Follow-up to #646, which bounded the Punycode *decoder*. The *encoder* has the mirror problem and was not covered there. Rebased on top of it, so the two changes are disjoint. `Idn::punycodeEncode()` walks the whole code point array twice for every distinct code point it still has to emit, so encoding costs O(n*r) and becomes quadratic when a label is made of distinct code points. Measured on PHP 8.5 without ext-intl, one label of distinct CJK code points: | label | before | | --- | --- | | 1 KB | 27 ms | | 8 KB | 430 ms | | 16 KB | 1.6 s | | 32 KB | 6.1 s | | 64 KB | 25 s | The reason this is reachable is the one #646 already identified: the polyfill converts input the extension refuses outright. ext-intl fails before converting when the result would not fit its own output buffer, and answers the 64 KB case above in 0.8 ms. So `idn_to_ascii()` now returns `false` without filling `$idna_info` when the domain has more than 255 code points. The ASCII form is at least as long as the code point count, so beyond that no result can fit that buffer, and ext-intl already returns `false` there without reporting anything. Encoding is then bounded to 255 code points, and a 20k-code-point label goes from 25 s to about 1.4 ms. Reachability inside Symfony: `HttpClientTrait::parseUrl()` calls `idn_to_ascii()` on the host with no length bound, so an application passing a user-supplied URL to HttpClient pays this when the extension is missing. I checked for behaviour changes by running every input in `IdnaTestV2.txt`, 7691 of them, through both directions before and after and comparing the return value, the error bits and the result. Nothing changes except domains over 255 code points, which returned `false` before as well; the difference is that `$idna_info` is no longer populated for them, which is what ext-intl does. Counting is done with the expression `Mbstring::mb_strlen()` falls back to, inlined since this package does not depend on mbstring. Counting only the bytes that can start a sequence would be shorter, but it would leave the bound trivially evadable: continuation bytes would not be counted, so a label could be padded with them while staying under the limit. Measured with the short version in place, 200 distinct code points followed by continuation bytes still cost 575 ms at 60 KB, 1.0 s at 120 KB and 2.1 s at 240 KB, against 3 to 6 ms once malformed bytes count too. There is a test for that case. The added tests fail without the change and pass with it. Commits ------- 405a3ba [Intl-Idn] Don't Punycode-encode domains the intl extension would refuse
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
punycodeDecode()inserts every decoded character into an array witharray_splice(), making the decode quadratic in the payload length. The label size is only checked after decoding, so a crafted multi-kilobytexn--label reachingidn_to_utf8()/idn_to_ascii()spends minutes of CPU in a single call before being rejected:This is reachable from any application that validates attacker-controlled domains while ext-intl is not installed — the deployment this polyfill targets.
egulias/emailvalidator, for one, depends on this package for exactly that.Valid ACE labels are limited to 63 bytes, so the patch rejects payloads beyond a generous 1024-byte bound up front with
ERROR_PUNYCODE, capping worst-case work at roughly half a million elementary operations (~ms). This mirrors observable native behavior: native ICU never decodes absurdly long labels — it rejects a 5 KB domain instantly.Labels above the DNS limit but below the bound still decode exactly as before, so ToUnicode keeps reporting their decoded form even when invalid, and all UTS #46 spec fixtures pass unchanged (37602 tests green, run both with and without ext-intl).
After the patch: same 200 KB input completes in 125 ms (the residual is the linear mapping pass), and legitimate domains (
xn--bcher-kva.com,bücher.example) behave identically.