Notes, , security, regex, codigest

A catastrophic-backtracking regex in a terminal hook

Context

codigest can hook a shell (cdg term install) so that the output of every command is kept for later. Before anything is stored, the output goes through a redaction pass that masks things that look like secrets: password=..., private key blocks, and, since a change on a feature branch, keys written in camelCase or PascalCase such as SessionToken or AccountKey. That last rule is the subject here. It never reached a release: the installed Python build (0.12.2) and the Rust build did not contain it. It was caught on the branch and fixed before the branch was merged (codigest#26).

What I found

Symptom

A benchmark of the hook’s overhead sat on one cdg term ingest for 10 minutes at 100 % of a core. The ingest itself is normally a fraction of a second. On the feature branch, an ingest whose captured output was a short run of letters returned in 0.45 s at 20 letters and was still running after 60 s at 200. The released Python build returned in 0.3 s and the Rust build in 0.02 s at every length.

Cause

The rule looked for a key name preceded by any number of words, so that awsSecretAccessKey or AWSSessionToken is caught by its last word. The leading part was

(?:[A-Za-z][a-z0-9]*)*?

a lazily repeated group, each repetition one letter followed by any number of lowercase letters or digits. On a run of \(n\) lowercase letters, every letter can either start a new repetition or continue the current one, so the run can be split into words in \(2^{n-1}\) ways. When no key name follows, the match fails, and a backtracking engine has to try every split before it gives up. Timing that pattern alone: 0 ms at 14 letters, 2 ms at 16, 7 ms at 18, 26 ms at 20, about four times longer for every two more letters. That is the signature of exponential backtracking, and the redaction pass hands it every line of output.

The Rust port of the same rule was not affected. Rust’s regex crate does not backtrack: it guarantees matching time linear in the input, and gives up features like backreferences for it. Python’s re is a backtracking engine, so the same pattern text has different worst-case behaviour in the two builds.

Fix

Make the split unambiguous. An optional first word in lowercase, then words that each start with a capital letter:

(?:[a-z][a-z0-9]*)?(?:[A-Z][a-z0-9]*)*?

A run of lowercase letters can now only be the first word, and every later word boundary is fixed by a capital letter, so the number of splits left to try grows with the length of the run instead of doubling with every letter. This matches what a camelCase or PascalCase name is anyway; the old group accepted splits no identifier would use.

The fix (df09426) came with tests written before it: a timing test that runs the redaction on a long run of letters in a subprocess and fails if it takes a second or more, and ten camelCase cases, seven that must be masked and three look-alikes that must not (maxtokens, passkey, myTokenizer), whose expected results were pinned from the rule’s behaviour before the change. With the patched pattern, every input up to 20,000 characters in the families tried returned in under a second, and the full suite passed locally: 222 tests. The fix went into main with codigest#22.

What I take from it

A redaction rule runs on input nobody chose: whatever a command happened to print. That makes its worst case the case that matters. Two habits would have caught this before the benchmark did:

  • for every regex that scans untrusted text, ask whether a run of the same character class can be matched in more than one way by a repeated group (here, [A-Za-z][a-z0-9]* repeated, where the first class contains the second);
  • give the redaction pass a timing test on long, unremarkable input, not only correctness tests on short lines that do contain secrets.

Next

The cases are shared: the Rust tests read the same fixture file as the Python ones. The timing test lives on the Python side, where the engine can backtrack. This note does not cover the other rules in redact.py; checking each of them for the same shape is the obvious next step.

Edit this note on GitHub