Skip to content

Fix out-of-bounds read in vCard QUOTED-PRINTABLE soft-line-break handling - #10270

Open
MiMoHo wants to merge 1 commit into
roundcube:masterfrom
MiMoHo:held-vcard-qp-oob
Open

Fix out-of-bounds read in vCard QUOTED-PRINTABLE soft-line-break handling#10270
MiMoHo wants to merge 1 commit into
roundcube:masterfrom
MiMoHo:held-vcard-qp-oob

Conversation

@MiMoHo

@MiMoHo MiMoHo commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Problem

Parsing a vCard whose final line uses a QUOTED-PRINTABLE soft line break (a line ending with =) triggers PHP "Undefined array key" warnings and a downstream preg_match(): Passing null deprecation. Under the test suite's failOnWarning/failOnNotice settings this is an outright failure; in production it emits warnings on untrusted input.

Root cause

program/lib/Roundcube/rcube_vcard.php has two QUOTED-PRINTABLE continuation loops that append the next line while the current line ends with =:

  • vcard_decode() at line 758
  • detect_encoding() at line 1052

Both were written as while (preg_match('/=$/', $lines[$i])) { $data .= "\n" . $lines[++$i]; }. When the last element of $lines ends with =, $lines[++$i] reads past the end of the array. The loop also re-checks $lines[$i] (now unset) on the next iteration, producing repeated warnings and passing null to preg_match.

Fix

Bound both loops so they never advance past the last element:

while (isset($lines[$i]) && preg_match('/=$/', $lines[$i]) && isset($lines[$i + 1])) {
    $data .= "\n" . $lines[++$i];
}

This keeps the existing best-effort decode behaviour (a dangling = on the last line is simply left as the end of the value) while eliminating the out-of-bounds access. The same guard is applied in both vcard_decode() and detect_encoding().

Testing

Added test_parse_qp_soft_break_on_last_line in tests/Framework/VCardTest.php, which decodes a vCard whose final QUOTED-PRINTABLE line ends with =, asserts no PHP warnings/errors are raised (via a temporary error handler), and checks the best-effort decoded value. The test fails on unmodified code (three PHP diagnostics) and passes with the fix. The full VCardTest file (15 tests, 40 assertions) passes, and phpstan level 4 reports no errors on the changed file.

@MiMoHo
MiMoHo force-pushed the held-vcard-qp-oob branch from 7edf066 to b10ad36 Compare July 16, 2026 08:14
…ling

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@MiMoHo
MiMoHo force-pushed the held-vcard-qp-oob branch from b10ad36 to d12dd87 Compare July 16, 2026 11:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant