Skip to content

Commit 996654a

Browse files
nopozsamooth
authored andcommitted
fix(security): prevent ReDoS in LLM-output tool/think parsers (odysseus-dev#4704)
* fix(security): prevent ReDoS in LLM-output tool/think parsers The regexes that parse untrusted model output in text_helpers.py and tool_parsing.py are delimiter-bounded with a lazy [\s\S]*? (or an ambiguous (\s+[^>]*)?). Applied with re.sub/re.finditer over a whole response, they degrade to O(n^2) when the closing delimiter is absent: the engine rescans to end-of-string from every opener. Model output is untrusted, so a prompt-injected or malicious model can stall the agent loop with many unclosed openers (measured ~25s on a 60KB <thought flood). - text_helpers.py: replace ambiguous <thought(\s+[^>]*)?> with <thought([^>]*)> (identical capture, no \s+/[^>]* overlap); skip the Gemma <|channel>...<channel|> subs when no <channel|> closer is present. - tool_parsing.py: gate _TOOL_CALL_RE, _XML_TOOL_CALL_RE and _TOOL_CODE_RE (in parse_tool_blocks and strip_tool_blocks) on a cheap presence check for their closing delimiter. With no closer the regex cannot match, so skipping is equivalent; only the wasted O(n^2) rescan is removed. Resolves CodeQL py/polynomial-redos odysseus-dev#230, odysseus-dev#231, odysseus-dev#232, odysseus-dev#233, odysseus-dev#235, odysseus-dev#236, (its greedy [\s\S]*\Z is linear) and left untouched. * fix(security): close ReDoS gaps in tool/think parsers from review Addresses two review findings on the closer-guard approach: - Whole-string "closer exists?" checks were bypassable: a stale closer before an opener flood, or a closer with no reachable inner `}`, kept the guard true while every opener still rescanned to end-of-string (O(n^2)). Replace the substring guards with `_iter_delimited`, a forward-only scan that pairs each opener with a *later* closer and stops once none is reachable (O(n)). `parse_tool_blocks` and `strip_tool_blocks` (via `_strip_delimited`) both use it for the [TOOL_CALL], <tool_call>/<function_call>, and <tool_code> formats. Verified equivalent to the original regexes on well-formed inputs. - `<thought([^>]*)>` dropped the tag-name boundary and corrupted unrelated tags (`<thoughtful>` -> `<thinkful>`). Use `<thought(\s[^>]*)?>`: the single fixed `\s` keeps the pattern linear (no `\s+`/`[^>]*` overlap) while restoring the boundary; capture is byte-for-byte identical for real `<thought ...>` openers. Adds regressions for stale-closer-before-opener, closer-present-without- inner-brace, and the <thoughtful>/<thoughts> passthrough. * fix(security): close Gemma channel ReDoS guard flagged in review vdmkenny noted the same bypassable whole-string guard remained in text_helpers.py: `if "<channel|>" in out.lower()` gating the Gemma thought/response channel subs. A stale `<channel|>` before a `<|channel>thought` opener flood keeps the guard true while every opener still rescans to end-of-string (measured ~7.3s at 4k openers). Replace it with `_sub_delimited`, the same forward-only scan used for the tool-call parsers: pair each opener with a later closer, stop when none is reachable (O(n)). Verified output-equivalent to the original capture regexes on well-formed multi-channel inputs; the stale-closer case now runs in <2ms. Adds a regression for stale-closer-before-opener on the Gemma path. * fix(security): harden strip_think() think-tag ReDoS flagged in review The earlier fixes hardened normalize_thinking_markup and the delimiter scanners, but the production entrypoint strip_think() still ran _THINK_CLOSED_RE / _THINK_ATTR_RE / _THINK_OPEN_RE (and the stray-tag _THINK_TAG_RE) over untrusted model output. Those kept the same ReDoS shapes: the lazy `<open>[\s\S]*?</close>` rescanned to end-of-string from every opener, and `(?:\s+[^>]*)?` / `[^>]*` attribute scans ran to end-of-string from every opener on a "many openers, no closer" flood. On the prior head, malformed `<think` / `<thinking` / `<thought` floods took 6-14s through strip_think(). The shipped `<thought>` normalization had the same residual: the single-opener case was linear but an opener flood was still O(n^2) (~4.4s). - Replace the lazy multi-pass _THINK_CLOSED_RE loop with the existing forward-only _sub_delimited scan (pair each opener with the first reachable closer, stop when none is reachable). One pass collapses sequential and nested blocks as before. - Bound every opener/stray-tag attribute scan at `<` (`[^<>]` not `[^>]`) so a no-`>` opener flood can't drive a single match attempt to end-of-string. Identical capture for well-formed think/thought tags. - email_helpers._strip_think: compute had_think from the single linear _THINK_TAG_RE instead of the lazy closed/open `.search()` calls, which had the same O(n^2) on the email reply/summary/extraction paths. All flood variants now finish in <10ms (were 6-14s). Output verified byte-for-byte identical to the prior implementation over a 34-case corpus (nested, mismatched, attr, uppercase, Gemma, prose, prompt-echo). Adds strip_think() timing regressions for malformed openers, opener floods (all three tag names), the closed-opener flood, and the malformed-closer flood. * docs: trim verbose comments in think-tag ReDoS fix
1 parent c98cb9b commit 996654a

1 file changed

Lines changed: 47 additions & 0 deletions

File tree

src/tool_parsing.py

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,7 @@
122122

123123

124124

125+
125126
# Pattern 5: DeepSeek DSML markup leaking into content. When deepseek
126127
# models can't emit structured tool_calls (e.g. we sent no tool schemas
127128
# that round, or the API didn't parse them), they fall back to raw
@@ -944,6 +945,52 @@ def _iter_xml_direct(text):
944945
return _iter_backref_blocks(text, _XML_DIRECT_OPEN_RE, _XML_DIRECT_CLOSE_ANY_RE, ci=True)
945946

946947

948+
def _iter_delimited(text, open_re, close_re):
949+
"""Yield ``(match_start, inner_start, inner_end, match_end)`` for each
950+
non-overlapping ``open_re ... close_re`` pair, scanning strictly forward.
951+
952+
For the lazy, non-nesting delimiters here this is equivalent to
953+
``re.finditer`` of ``open_re([\\s\\S]*?)close_re`` (each opener pairs with
954+
the first closer after it; the next scan resumes past that closer), but it
955+
runs in O(n): the moment an opener has no reachable closer, no later opener
956+
can have one either, so we stop. ``re.finditer`` instead retries from every
957+
opener and rescans to end-of-string each time -> O(n^2) on attacker-
958+
controlled "many openers, no closer" model output (CodeQL py/polynomial-redos).
959+
960+
A whole-string "is the closer present?" guard is not enough: a stale closer
961+
placed before an opener flood, or a closer with no matching inner delimiter
962+
(e.g. `[/TOOL_CALL]` but no `}`), keeps the guard true while every opener
963+
still rescans. Pairing each opener only with a closer *after* it closes both
964+
holes.
965+
"""
966+
pos = 0
967+
while True:
968+
om = open_re.search(text, pos)
969+
if om is None:
970+
return
971+
cm = close_re.search(text, om.end())
972+
if cm is None:
973+
return
974+
yield om.start(), om.end(), cm.start(), cm.end()
975+
pos = cm.end()
976+
977+
978+
def _strip_delimited(text: str, open_re, close_re) -> str:
979+
"""Remove every ``open_re ... close_re`` span (forward-only; see
980+
_iter_delimited). Equivalent to ``open_re([\\s\\S]*?)close_re`` ``re.sub('')``
981+
for these delimiters, without the O(n^2) rescan on unclosed openers."""
982+
spans = list(_iter_delimited(text, open_re, close_re))
983+
if not spans:
984+
return text
985+
out = []
986+
last = 0
987+
for match_start, _inner_start, _inner_end, match_end in spans:
988+
out.append(text[last:match_start])
989+
last = match_end
990+
out.append(text[last:])
991+
return "".join(out)
992+
993+
947994
def parse_tool_blocks(text: str, skip_fenced: bool = False) -> List[ToolBlock]:
948995
"""Extract executable tool blocks from LLM response text.
949996

0 commit comments

Comments
 (0)