Repository navigation
fix: keep what sanitizers clear in function summaries - #138
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #134, the sanitizer half. The validator half, for project validators, follows in its own PR.
Problem
A function summary recorded which parameters reach which external calls and the return value, but not what the calls on the way did to the data. When a caller passed request data to a helper that escapes it, or to a view that redirects to
reverse(...), the taint engine mapped the full taint through the summary and reported the sink anyway. Only a sanitizer called directly in the flow's own function counted.Change
ExternalCall.argument_cleared,keyword_cleared);FunctionSummary.return_cleared).When two paths join, only what both cleared is kept, so a path around the sanitizer still reaches the sink. Summaries compose: a sanitizer two calls deep still protects.
Clearingsays what each call clears: a sanitizer clears its kinds, and aTemplateRenderof an escaped template clearsHTML. It is derived byModelTable.clearing(escaped), the single place that rule lives. The taint engine uses it for direct calls, replacing its two separate checks. The engine provides it to the summaries (ClearingAnalysis), which stay free of the taint package. Without it, a summary clears nothing, which is conservative.nonlocalwrites still clear nothing, which is conservative. This is documented.Snapshot changes, reviewed (healthchecks only, 8 findings removed)
hc/accounts/views.py:170(login),:281×2 (check_token) open-redirect_check_2fa, which redirects toreverse(route, query=...);nextonly lands in the query string.hc/accounts/views.py:637×2 (change_email_verify) open-redirectcheck_token(...)leads to the samereverse, one more call deep.hc/api/views.py:778(badge),:811(check_badge) xssHttpResponse(get_badge_svg(...)): the helper rendersbadge.svg, which printstag/statusonly in text and quoted attributes, so it is established as escaped.hc/integrations/po/views.py:59(add) open-redirectsettings.PUSHOVER_SUBSCRIPTION_URL + "?" + urlencode(...). The POST data only enters throughabsolute_reverse→reverse, in the query.Verification
pytest: 1073 passed.ruff check src testsandmypyare clean.pytest -m regressionunder Python 3.12: 19 passed. Onlyhealthchecks.jsonchanged, by the 8 findings above.Dephad nocleared. The two guard tests, a path around the sanitizer and a sanitizer clearing only its own kinds, were green before and after.