Skip to content

fix(Foundation,Net,XML,Redis): input limits, command injection and parser fixes from the ClickHouse fork review - #5609

Open
matejk wants to merge 16 commits into
mainfrom
5510-clickhouse-security-fixes
Open

matejk wants to merge 16 commits into
mainfrom
5510-clickhouse-security-fixes

Conversation

@matejk

@matejk matejk commented Oct 29, 2026

Copy link
Copy Markdown
Contributor

Part of #5510 (review of the Poco fork embedded in ClickHouse). This is the last group of security fixes from that review; one commit per fix, please merge without squashing.

Several readers trusted hostile input: Var::parse, BinaryReader::readCString(), Unicode::properties(), Logger::format() and the Redis reply reader could recurse without limit, loop, read out of bounds or allocate memory that a peer only declared. DialogSocket sent CR and LF in caller text unchanged (SMTP, POP3 and FTP command injection), and XMLWriter wrote ]]> inside CDATA unchanged.

  • Foundation (8): the input limits and bounds above; the AsyncChannel worker survives target exceptions; RotateAtTimeStrategy rejects out-of-range times; Path::home() uses getpwuid_r(); the const Message::operator[] throws as documented.
  • Net (5): DialogSocket rejects CR and LF; IPAddress rejects malformed IPv6 text; TCPServer pauses after an accept that fails for lack of resources; DNS reports EAI_SYSTEM with errno; MultipartReader throws instead of asserting on an empty boundary.
  • XML (2): XMLWriter splits ]]> in CDATA; XMLStreamParser passes callback exceptions to the caller and frees the parser.
  • Redis (1): bulk strings read in chunks, nesting limited to 128, truncated replies throw.
  • Breaking: listed in CHANGELOG under GH Review of the Poco fork embedded in ClickHouse: changes to take into main #5510.

Tests: every fix has a test that fails without it, except XMLStreamParser (it would need a global operator new replacement, against CODING_STYLE Rules 71 and 73; verified by hand under ASan/LSan). Run on macOS, Linux (release, ENABLE_TRACE, ASan/UBSan, TSan) and Windows; Redis also against a Redis 7 server.

Follow-ups: pre-existing issues found in the review, to be listed in #5510.

matejk added 16 commits October 29, 2026 08:48
An array closed by '}' made the parser loop forever and deep nesting exhausted the stack.
Without a limit, a stream without a terminator grows the string until the stream ends.
…or a missing parameter

It inserted an empty value into the const message instead.
…n hour or minute

A rotation time that is never reached made the constructor loop forever.
…() when HOME is unset

getpwuid() returns storage that concurrent lookups overwrite.
…to the caller

Unwinding through Expat kept its handler call depth above zero, so XML_ParserFree() leaked the parser.
@matejk matejk added this to the Release 2.0 milestone Oct 29, 2026
@matejk matejk added bug breaking A breaking change labels Oct 29, 2026
@matejk
matejk requested a review from aleks-f October 29, 2026 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking A breaking change bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant