Green tests, unsafe code

The change works and it should not ship

An assistant opens a pull request. It adds a filter box to the usage report, the description is clear, the diff is thirty lines, and every test in the suite is green. You try the feature by hand and it does exactly what the ticket asked for.

Approve it and you have shipped a way to run arbitrary Python from a query string.

This is the happy path lesson with the stakes turned all the way up. Green tests mean the behavior somebody asserted is present. They say nothing about the behavior nobody asserted, and that unasserted space is where a security hole lives. Nothing else in the loop objects either: the model wrote code that works, then wrote tests for the code it wrote, and the two agree with each other by construction. You are the only control left.

Follow the input to the sink

A sink is the operation that gives an input power: executing it as code, using it as a file path, splicing it into a query, handing it to a shell. Everything else in a program moves data around. A sink is where data starts making decisions.

So the audit is two questions, in this order, on any code that touches a request.

1. What does the user control?
2. What is the most powerful thing the code does with it?

The family of sinks is short enough to memorize. eval and exec run text as Python. A query built by string concatenation turns a name into a statement. A path join turns a filename into a file anywhere on the disk. A subprocess call built from a string turns an argument into a shell line.

# The same query, two shapes. The second one can only ever be a name.
cursor.execute("SELECT * FROM users WHERE name = '" + name + "'")
cursor.execute("SELECT * FROM users WHERE name = ?", (name,))

Here is the filter feature the pull request added. It is the shortest implementation that satisfies the ticket, which is exactly why it keeps getting generated.

def match(row, expression):
    # Safe because __builtins__ is empty, so nothing can be imported.
    return bool(eval(expression, {"__builtins__": {}}, dict(row)))

An allowlist beats a blocklist

The repair reflex is to filter the bad input out: strip double underscores, reject the word import, block a semicolon. That is a blocklist, and a blocklist loses on a schedule. You have to anticipate every hostile spelling. The person probing your endpoint has to find one you missed, once.

An allowlist inverts the arithmetic. Write down the grammar you meant to support, parse the input against that grammar, and refuse everything that does not fit. What gets refused is then everything you did not think of, which is the set that was going to hurt you.

# The shape of the parser, not the parser
# 1. find the operator the expression uses, and split the expression once there
# 2. check the left side against the row's real fields
# 3. parse the right side as a literal: an integer, or a quoted string
# 4. check that the literal's kind matches the field, then compare with a plain if chain

Notice what is absent from that outline. Nothing in it executes the input. The input stays data the whole way through, and the code does the deciding.

The same audit, a different sink

When a caller controls a path fragment, joining it onto a base directory and normalizing does not keep it under that base. Normalization is not a security check, it is a tidying step, and it will happily tidy its way out of the directory you meant.

import posixpath
print(posixpath.normpath("docs/" + "../internal/rotation-list.md"))
# internal/rotation-list.md

The base disappeared. So the check belongs on the normalized result rather than on the raw input: normalize first, then require the answer to sit under the directory you meant, and refuse it otherwise.

Pitfalls

• Trusting a gutted __builtins__. It removes __import__ and leaves every attribute of every literal.
• Blocklisting substrings. You are guessing at spellings. A parser does not have to guess.
• Wrapping the sink in try. That catches accidents. An escape does not raise, it returns a value.
• Repairing so hard the feature dies. The feature tests are there to hold that side of the trade.

Interview nuance: a security observation during a code review round is rare and gets credited far out of proportion to the seconds it costs. The form that lands is one sentence naming the input, the sink, and the fix: this endpoint evaluates user text, I would parse the grammar instead.

The real thing next: the bug-fix interview rounds drop you into a codebase like these with an interviewer in the loop.