Fix control characters passing unescaped into RFC 2047 encoded-words - #37
Open
sebastka wants to merge 1 commit into
Open
Fix control characters passing unescaped into RFC 2047 encoded-words#37sebastka wants to merge 1 commit into
sebastka wants to merge 1 commit into
Conversation
The "Q" character class escaped printable punctuation and every 8-bit byte but neither \x00-\x1F nor \x7F, so C0 controls and DEL were written into the header verbatim. RFC 2047 defines encoded-text as printable ASCII other than "?" and SPACE, so any of them makes the encoded-word invalid, and a raw CR or LF in a header is a header injection vector. A literal LF failed in a second way with ext/mbstring. encodeMB() uses "\n" as its own chunk separator and expands it into a fold when assembling the result, so an LF in the value was indistinguishable from a chunk boundary: the value was split into two encoded-words and the byte silently disappeared from the decoded header. Add the two missing ranges, which lets \x7B-\x7E, \x7F and \x80-\xFF collapse into \x7B-\xFF. The class was duplicated verbatim in encodeQP() and encodeMB(), the second copy carrying a "see encodeQP()" comment, so it moves into Mail_mimePart::QP_ESCAPE_REGEXP as 4216044 (pear#34) did for MAX_CHARSET_LENGTH. tests/headers_with_mbstring.phpt pinned the defect as expected output and is regenerated. Case [31] encodes a Japanese subject to ISO-2022-JP, whose charset-switching escapes are ESC, and one of the JIS X 0208 bytes involved is \x0D: the expectations held ten raw ESC bytes and a bare carriage return inside a Subject header, invisible unless viewed with cat -v. Unlike pear#33, pear#34 and pear#35 this reproduces with ext/mbstring present, so tests/rfc2047_control_chars.phpt needs no --INI-- section. It sweeps all 32 C0 control characters plus DEL across both encodings, checking that the encoded-text holds only printable ASCII and that the byte survives a round trip. It reports 34 failures against the previous code.
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
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.
Hello again,
This is the last RFC 2047 violation found by the LLM: C0 controls and DEL were not being encoded.
The "Q" encoder does not escape control characters, so they end up in the header verbatim. RFC 2047 restricts
encoded-textto printable ASCII, and a raw CR or LF in a header is a header injection vector.Unlike the previous three findings, this one reproduces on a default build with ext/mbstring present.
How to reproduce
poc.php
Before
After
Root cause
The "Q" character class covers printable punctuation and every 8-bit byte, but not
\x00-\x1For\x7F. It was duplicated verbatim inencodeQP()and inencodeMB(), the second copy carrying a// see encodeQP()comment to point at the first:RFC 2047 §2 defines
encoded-textas1*<Any printable ASCII character other than "?" or SPACE>, and §4.2(3) permits leaving a byte unescaped only when it is printable ASCII other than=,?and_. A C0 control or DEL is neither, so any of them makes the encoded-word invalid.The LF case has a second failure mode.
encodeMB()uses"\n"as its own chunk separator and expands it into a fold at the end:A literal LF in the value is therefore indistinguishable from a chunk boundary:
"aä\nX-Injected: yes"becomes two encoded-words and the newline is silently dropped from the decoded value. The output looks perfectly legal, which is why the poc checks the round trip separately.Nothing in
Mail/mime.phpstrips CR or LF before this point, so on a build without ext/mbstring the same input emits a raw LF straight into the header.Fix
Add the two missing ranges;
\x7B-\x7E,\x7Fand\x80-\xFFthen collapse into\x7B-\xFF. Since the class was duplicated, it moves into a constant so the two encoders cannot drift apart, the same treatmentMAX_CHARSET_LENGTHgot in #34:The docblock is worth the space: without it the absence of
\x20reads as a third oversight rather than a deliberate choice, and a future reader could "fix" SPACE into the class and break the_substitution.Scope
tests/headers_with_mbstring.phptneeds its expectations regenerated, and the diff is worth a look, because case[31]was pinning this defect as correct output. That case encodes a Japanese subject to ISO-2022-JP, whose charset-switching escapes are ESC (\x1B) — and one of the JIS X 0208 bytes for those characters is\x0D. Neither was escaped, so the expected output contained 10 raw ESC bytes and a bare CR inside aSubjectheader, invisible unless viewed withcat -v:That is the header injection case arising from an ordinary Japanese subject rather than a crafted one. After the fix:
test_Bug_21205.phptandtest_Bug_20226.phptalso cover ISO-2022-JP but are untouched: they encode with base64, where ESC is base64-encoded regardless.test_Bug_20273.phpt("encodeHeader() and TAB character") also passes unchanged. Its value is a pure ASCIIReferencesheader, so it never reachesencodeQP(): the TAB is consumed byexplodeQuotedString()'s separator handling.Test
tests/rfc2047_control_chars.phptsweeps all 32 C0 control characters plus DEL, across both encodings, asserting that the encoded-text holds only printable ASCII and that the byte survives a round trip. It reports 34 failures against the current code and none with the fix.No
--INI--section this time: the defect reproduces with ext/mbstring present, so the test exercises it on every CI job as-is.AI use disclosure
I used Anthropic's Claude LLM with Opus 5 to find this bug. It also suggested a fix, which I have reviewed and tested.