From 84d1597b165314f516dc4f1e143b1385489fde8c Mon Sep 17 00:00:00 2001 From: Brigs Date: Thu, 6 Aug 2026 22:09:04 -0400 Subject: [PATCH] Add a CI guard for report injection and remote destinations Port of iLEAPP #1891, with this core's own baseline. html_columns cells are written to the report without html.escape, which makes them an injection sink, and any remote href/src makes opening a report a disclosure event. The escaping was fixed by hand in GHSA-45q2-q93c-cfv2 and nothing has watched it since. The drift is not hypothetical: the same waze format_url() was fixed in iLEAPP and left anchoring evidence URLs in ALEAPP until a corrected run of this very check found it. admin/scripts/check_html_safety.py, wired into python_lint.yml beside check_claim_language.py. Three AST rules -- unescaped-interpolation over markup-bearing f-strings, + concatenation, %, .format() and "
".join(); remote-destination over literal remote URLs and any href/src not provably report-relative; and unguarded-html-columns for a module that declares html_columns and calls no escaper. Scoped to modules that can reach a no-escape cell: a declared html_columns, a media column, or a named framework helper. Elsewhere the writer escapes the whole cell, so hand-built markup is a display bug rather than an injection. BASELINE carries what this core has today so the gate can go up without blocking anyone; new findings fail. Both lists also fail the run when an entry stops matching, so the debt list can only shrink. Entries are keyed by (path, rule, function) and survive line moves. Every entry says what it is and what would clear it. Validated: pylint --disable=C,R = 10.00; workflow YAML parses; the check exits 0 on this tree and exits 1 on injected unescaped-interpolation and remote-src probes, returning to 0 when they are reverted. Co-Authored-By: Claude Opus 5 --- .github/workflows/python_lint.yml | 8 + admin/scripts/check_html_safety.py | 567 +++++++++++++++++++++++++++++ 2 files changed, 575 insertions(+) create mode 100644 admin/scripts/check_html_safety.py diff --git a/.github/workflows/python_lint.yml b/.github/workflows/python_lint.yml index a03e292..88b99d8 100644 --- a/.github/workflows/python_lint.yml +++ b/.github/workflows/python_lint.yml @@ -42,6 +42,14 @@ jobs: if: steps.changed-files-py.outputs.any_changed == 'true' run: python admin/scripts/check_claim_language.py + # html_columns cells are written to the report without html.escape, so evidence + # placed there can inject markup, and any remote href/src makes opening a report + # beacon to a third party. Pre-existing findings are carried in the script's + # BASELINE and do not fail; new ones do. See the script's docstring. + - name: Guard report output against injection and remote destinations + if: steps.changed-files-py.outputs.any_changed == 'true' + run: python admin/scripts/check_html_safety.py + # Fails only on warnings this pull request introduces. dleapp.py and # dleappGUI.py carry pre-existing warnings that are structural rather than # fixable -- wildcard imports are how those modules are put together -- so diff --git a/admin/scripts/check_html_safety.py b/admin/scripts/check_html_safety.py new file mode 100644 index 0000000..94f5e3f --- /dev/null +++ b/admin/scripts/check_html_safety.py @@ -0,0 +1,567 @@ +"""Guard the HTML report against unescaped evidence and remote destinations. + +Columns listed in an artifact's `__artifacts_v2__` `html_columns` are written to the +report *without* `html.escape` (see `scripts/artifact_report.py`, the `html_no_escape` +branch). Anything an artifact places in such a cell is emitted verbatim, so the report +inherits two distinct risks from the evidence it parses: + +* **Injection.** Markup in a parsed value renders live in the examiner's report + (stored XSS, CWE-79; GHSA-45q2-q93c-cfv2). `scripts/html_safe.py` exists to stop + this: route the dynamic parts through `esc`/`safe_join`/`safe_source` and let only + tool-authored markup through unescaped. + +* **Disclosure.** A cell that names a remote host makes the examiner's browser reach + it. An `` needs no click at all: opening the report tells + the service operator that the account is under examination, when, and from which + IP address. Reports are read on analyst workstations and mailed to counsel, so a + report must never be a beacon. + +The standard these two risks produce is: + + Escape every evidence-derived value before it reaches a no-escape cell, and + point no `href` or `src` at anything outside the report folder. + +Report-relative destinations stay live -- `media/` thumbnails and the files +artifacts write beside the report are what make the report navigable, and they +resolve offline. Remote destinations do not, whatever the scheme: `http`/`https` +fetch from a third party, and `mailto:`/`tel:` hand the subject's own address or +number to a mail client or dialer, where one stray click in front of a suspect's +counsel is its own kind of incident. + +The failure mode this check exists to stop is a partial fix. When html_safe.py landed, +iLEAPP's `walStrings` anchor was escaped and ALEAPP's identical `walStrings` anchor was +not; it stayed unescaped for weeks because nothing was watching. Cross-core drift is +the normal outcome here, not the exception. + +THREE RULES +----------- +`unescaped-interpolation` + A string expression whose literal parts contain markup interpolates something + that is not a constant and not a call to an approved escaper. Covers f-strings, + `+` concatenation, `%`, `.format()`, and `"
".join(...)`, because HTML gets + built every one of those ways in this repo. + +`remote-destination` + A literal remote URL appears in a markup-bearing string, or an `href=`/`src=` + attribute is completed by an interpolation. A dynamic destination cannot be shown + to be report-relative by reading the source, so it fails unless it comes from + `safe_local_link()`. + +`unguarded-html-columns` + A module declares `html_columns` but never references an escaper. This catches the + case the other two rules cannot see: a value assembled with no markup anywhere near + it -- a bare URL, say -- that still lands in a no-escape cell. + +BASELINE vs ALLOWLIST +--------------------- +`BASELINE` holds violations that already existed when this check landed. They do not +fail the build, so the check can be enabled before the cleanup finishes, but they are +debt: delete each entry as its violation is fixed. + +`ALLOWLIST` holds reviewed exceptions that are correct as they stand and are expected +to stay -- markup built only from tool-owned constants, a media-only column. + +Both lists fail the run when an entry stops matching anything. A stale entry means the +code moved or was fixed, and the entry now shields whatever lands under the same name +next. Entries are keyed by (path, rule, enclosing function) so they survive line moves; +a module-level violation uses ``. + +Coverage holes are printed, never hidden. A module whose `__artifacts_v2__` is not a +static literal cannot have its `html_columns` read, so `unguarded-html-columns` cannot +run on it; those are listed as NOT CHECKED on every run. + +`scripts/artifact_report.py` is deliberately out of scope. It implements the +escape/no-escape branch itself, so it is the sink these rules protect, not a producer. + +Usage: + python admin/scripts/check_html_safety.py # CI mode, exits 1 on a violation + python admin/scripts/check_html_safety.py --list # every finding, baselined included + python admin/scripts/check_html_safety.py --verbose # coverage and list counts +""" + +import argparse +import ast +import glob +import os +import re +import sys + +# Markup that means the surrounding string is being built for the report rather than +# for a log line or a query. An opening tag, or an attribute that takes a URL. +# A complete tag, not a fragment: `]*>' + r'|\b(?:href|src)\s*=\s*["\']', + re.IGNORECASE) + +# Schemes that leave the report folder. `mailto`/`tel` do not fetch, but they hand the +# subject's contact details to a local app on a click, so they are destinations too. +REMOTE_SCHEME_PATTERN = re.compile( + r'\b(?:https?|ftps?|ws|wss|mailto|tel|data)\s*:', re.IGNORECASE) + +# A literal part ending here means the attribute's value is whatever comes next. +DESTINATION_ATTR_PATTERN = re.compile(r'\b(?:href|src)\s*=\s*["\']?\s*$', re.IGNORECASE) + +# The flag artifacts use to pick the HTML output arm of a two-output helper. +HTML_FLAG_PATTERN = re.compile(r'html_format|html_output|is_html|as_html') + +# Calls whose result is safe to drop into a no-escape cell. `esc`/`safe_*` come from +# scripts/html_safe.py; `escape` is the stdlib import artifacts predating it still use; +# `quote` is urllib's percent-encoder, used on report-relative media paths. +ESCAPER_NAMES = frozenset({ + 'esc', 'safe_url', 'safe_join', 'safe_source', 'safe_local_link', 'safe_local_path', + 'escape', 'quote', +}) + +# The subset of the above that yields a destination which resolves inside the report +# folder. Only these may complete an href= or src=. The media helpers belong here +# because they rewrite an extraction path to `media/` beside the report. +LOCAL_LINK_NAMES = frozenset({ + 'safe_local_path', 'safe_local_link', 'media_to_html', 'check_in_media', + 'html_media_tag', +}) + +# Pre-existing violations. Delete an entry when its violation is fixed; a stale entry +# fails the run. See the module docstring before adding one. +BASELINE = { + # media_to_html() assigns `source` four times before emitting it -- the raw match, + # a relative path, a copied path, then safe_local_path(). A name is treated as safe + # only when *every* assignment to it is, because this check does not order + # assignments. The function is in fact correct: the last write is safe_local_path() + # and nothing reads `source` before it. Rewriting it to bind the escaped value to + # its own name would clear this honestly. + ('scripts/ilapfuncs.py', 'remote-destination', 'media_to_html'), + ('scripts/ilapfuncs.py', 'unescaped-interpolation', 'media_to_html'), +} + +# Reviewed exceptions expected to stay. Every entry needs a comment saying why. +ALLOWLIST = set() + +# Framework helpers that build the contents of a report *cell* for artifacts to use. +# They have no __artifacts_v2__ of their own, so they are named rather than detected, +# and they are named per-function on purpose. +# +# ilapfuncs.py also writes the device-info page and the log page, which interpolate +# evidence-derived values into markup on a surface this check does not cover. That is +# a real surface and a larger one; guarding it is its own piece of work, and pulling +# it in here would bury the cell findings this check exists to hold the line on. +FRAMEWORK_FUNCTIONS = { + 'scripts/ilapfuncs.py': frozenset({ + 'html_media_tag', 'media_to_html', 'get_data_list_with_media', + }), +} + +STANDARD_NOTE = ( + 'Values in an html_columns cell are written to the report unescaped.\n' + 'Escape evidence-derived text with esc()/safe_join()/safe_source() from\n' + 'scripts/html_safe.py, and keep every href/src inside the report folder --\n' + 'no http, https, ftp, mailto or tel destination reaches the report.\n' + 'If the finding is markup built only from tool-owned constants, add it to\n' + 'ALLOWLIST in admin/scripts/check_html_safety.py with a comment saying why.' +) + + +def enclosing_functions(tree): + """Map each node to the name of the function that contains it. + + Keys are `id()` values, so the tree must stay alive while the map is used. + Nodes outside any function map to ``. + """ + owner = {} + + def walk(node, name): + for child in ast.iter_child_nodes(node): + child_name = name + if isinstance(child, (ast.FunctionDef, ast.AsyncFunctionDef)): + child_name = child.name + owner[id(child)] = child_name + walk(child, child_name) + + owner[id(tree)] = '' + walk(tree, '') + return owner + + +def flatten_concat(node): + """Return the operands of a `+` chain in source order, or None if not one.""" + if not isinstance(node, ast.BinOp) or not isinstance(node.op, ast.Add): + return None + left = flatten_concat(node.left) or [node.left] + right = flatten_concat(node.right) or [node.right] + return left + right + + +def string_parts(node): + """Return this node's string parts as [(literal, dynamic_expr_or_None), ...]. + + Returns None when the node does not build a string. Each entry is either a + literal fragment or a dynamic expression; the caller reads them in source order, + so a literal ending in `href="` is known to be completed by the next entry. + """ + if isinstance(node, ast.JoinedStr): + parts = [] + for value in node.values: + if isinstance(value, ast.Constant) and isinstance(value.value, str): + parts.append((value.value, None)) + elif isinstance(value, ast.FormattedValue): + parts.append((None, value.value)) + return parts + + operands = flatten_concat(node) + if operands and any(isinstance(op, ast.Constant) and isinstance(op.value, str) + for op in operands): + return [(op.value, None) if isinstance(op, ast.Constant) + and isinstance(op.value, str) else (None, op) for op in operands] + + # "%s" % value + if (isinstance(node, ast.BinOp) and isinstance(node.op, ast.Mod) + and isinstance(node.left, ast.Constant) and isinstance(node.left.value, str)): + args = (node.right.elts if isinstance(node.right, ast.Tuple) else [node.right]) + return [(node.left.value, None)] + [(None, arg) for arg in args] + + if isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute): + target = node.func.value + is_literal_target = (isinstance(target, ast.Constant) + and isinstance(target.value, str)) + # "{}".format(value) + if node.func.attr == 'format' and is_literal_target: + return ([(target.value, None)] + + [(None, arg) for arg in node.args] + + [(None, kw.value) for kw in node.keywords]) + # "
".join(values) -- the separator is the markup, the values are dynamic. + if node.func.attr == 'join' and is_literal_target and node.args: + return [(target.value, None), (None, node.args[0])] + + return None + + +def call_name(node): + """Return the callee name for a Call node, or None.""" + if not isinstance(node, ast.Call): + return None + if isinstance(node.func, ast.Name): + return node.func.id + if isinstance(node.func, ast.Attribute): + return node.func.attr + return None + + +def _resolve(node, assignments, names, seen): + """True when `node` is safe under the callee set `names`. + + `assignments` maps a local variable to every expression assigned to it in the + enclosing function, so the common `thumb = media_to_html(...)` then + `f'src="{thumb}"'` shape resolves instead of failing for lack of context. A name + is safe only when *every* assignment to it is, which keeps a conditional + reassignment from laundering a raw value. + """ + if isinstance(node, ast.Constant): + return True + if isinstance(node, ast.IfExp): + # `esc(v) if html_format else v` is the repo's standard two-output idiom: the + # escaped arm builds the HTML row, the raw arm builds the TSV/LAVA row, and + # only the first reaches a no-escape cell. Judge the HTML arm alone when the + # test names that flag; otherwise both arms have to be safe. + if HTML_FLAG_PATTERN.search(ast.dump(node.test)): + return _resolve(node.body, assignments, names, seen) + return (_resolve(node.body, assignments, names, seen) + and _resolve(node.orelse, assignments, names, seen)) + if isinstance(node, ast.BoolOp): + return all(_resolve(v, assignments, names, seen) for v in node.values) + # "
".join(esc(v) for v in values) -- the element is what reaches the cell. + if isinstance(node, (ast.GeneratorExp, ast.ListComp, ast.SetComp)): + return _resolve(node.elt, assignments, names, seen) + if isinstance(node, ast.JoinedStr): + return all(_resolve(v.value, assignments, names, seen) + for v in node.values if isinstance(v, ast.FormattedValue)) + if isinstance(node, ast.Call): + if call_name(node) in names: + return True + # str.join() is transparent: safety is decided by what is being joined. + if (isinstance(node.func, ast.Attribute) and node.func.attr == 'join' + and node.args): + return _resolve(node.args[0], assignments, names, seen) + return False + if isinstance(node, ast.Name): + if node.id in seen: # a self-referential accumulator (x = x + ...) + return True + values = assignments.get(node.id) + if not values: + return False + return all(_resolve(v, assignments, names, seen | {node.id}) for v in values) + # An empty container is safe on its own; what matters is what gets put in it, + # which local_assignments() records from .append()/.extend(). + if isinstance(node, (ast.List, ast.Tuple, ast.Set)): + return all(_resolve(elt, assignments, names, seen) for elt in node.elts) + return False + + +def is_escaped(node, assignments): + """True when this interpolated expression is safe in a no-escape cell.""" + return _resolve(node, assignments, ESCAPER_NAMES, frozenset()) + + +def is_local_link(node, assignments): + """True when this expression yields a destination inside the report folder.""" + return _resolve(node, assignments, LOCAL_LINK_NAMES, frozenset()) + + +def local_assignments(tree): + """Map each function to {variable: [assigned expressions]} within it. + + Augmented assignment (`aggregate += f'...'`) records the right-hand side, so an + accumulator built only from escaped fragments still resolves as escaped. + """ + def own_nodes(scope): + """Walk a function body without descending into nested functions. + + A nested helper has its own bindings. html_media_tag()'s inner + relative_paths() binds `filename` to an unescaped value, and letting that + leak into the outer table made the outer function's own escaped `filename` + look unsafe. + """ + for child in ast.iter_child_nodes(scope): + if isinstance(child, (ast.FunctionDef, ast.AsyncFunctionDef)): + continue + yield child + yield from own_nodes(child) + + per_function = {} + for node in ast.walk(tree): + if not isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef, ast.Module)): + continue + key = node.name if hasattr(node, 'name') else '' + table = per_function.setdefault(key, {}) + for child in own_nodes(node): + if isinstance(child, ast.Assign): + for target in child.targets: + if isinstance(target, ast.Name): + table.setdefault(target.id, []).append(child.value) + elif isinstance(child, ast.AugAssign) and isinstance(child.target, ast.Name): + table.setdefault(child.target.id, []).append(child.value) + # `parts.append(esc(v))` is how most list-then-join builders fill up, so + # the appended expression counts as a value the name can hold. + elif (isinstance(child, ast.Call) and isinstance(child.func, ast.Attribute) + and child.func.attr in ('append', 'extend', 'add') + and isinstance(child.func.value, ast.Name) and child.args): + table.setdefault(child.func.value.id, []).append(child.args[0]) + return per_function + + +def scan_string_builders(tree, owner, assignments_by_function): + """Return (rule, function, line, detail) for every markup string built unsafely.""" + findings = [] + for node in ast.walk(tree): + parts = string_parts(node) + if not parts: + continue + literal_text = ''.join(text for text, _ in parts if text) + if not MARKUP_PATTERN.search(literal_text): + continue + + function = owner.get(id(node), '') + assignments = assignments_by_function.get(function, {}) + line = getattr(node, 'lineno', 0) + snippet = ' '.join(literal_text.split())[:120] + + if REMOTE_SCHEME_PATTERN.search(literal_text): + findings.append(('remote-destination', function, line, + f'literal remote destination in markup: {snippet}')) + + previous_literal = '' + for text, dynamic in parts: + if dynamic is None: + previous_literal = text or '' + continue + completes_destination = DESTINATION_ATTR_PATTERN.search(previous_literal) + if completes_destination and not is_local_link(dynamic, assignments): + findings.append(('remote-destination', function, line, + f'href/src completed by an expression that is not a ' + f'report-relative destination: {snippet}')) + elif not is_escaped(dynamic, assignments): + findings.append(('unescaped-interpolation', function, line, + f'unescaped value interpolated into markup: {snippet}')) + previous_literal = '' + return findings + + +def find_artifacts_dict(tree): + """Return the AST node assigned to `__artifacts_v2__`, or None.""" + for node in ast.walk(tree): + if not isinstance(node, ast.Assign): + continue + for target in node.targets: + if isinstance(target, ast.Name) and target.id == '__artifacts_v2__': + return node.value + return None + + +def declares_html_columns(tree): + """Return (declares, skip_reason). `declares` is None when it cannot be read.""" + node = find_artifacts_dict(tree) + if node is None: + return False, None + try: + artifacts = ast.literal_eval(node) + except (ValueError, TypeError, SyntaxError, MemoryError, RecursionError) as ex: + return None, f'__artifacts_v2__ is not a literal: {ex}' + if not isinstance(artifacts, dict): + return None, '__artifacts_v2__ is not a dict' + return any(isinstance(entry, dict) and entry.get('html_columns') + for entry in artifacts.values()), None + + +def uses_media_helper(tree): + """True when the module builds a media cell, which the framework never escapes.""" + return any(call_name(node) in LOCAL_LINK_NAMES for node in ast.walk(tree) + if isinstance(node, ast.Call)) + + +def references_escaper(tree): + """True when the module calls any approved escaper anywhere.""" + return any(call_name(node) in ESCAPER_NAMES for node in ast.walk(tree) + if isinstance(node, ast.Call)) + + +def scan_file(path, rel_path): + """Return (findings, skip_reason) for one module.""" + try: + with open(path, 'r', encoding='utf-8') as handle: + source = handle.read() + except OSError as ex: + return [], f'could not read file: {ex}' + try: + tree = ast.parse(source, filename=path) + except SyntaxError as ex: + return [], f'could not parse module: {ex}' + + declares, skip_reason = declares_html_columns(tree) + + # Markup only matters where it can reach a cell the report writes unescaped: a + # declared html_columns, a media column (the framework adds those to + # html_no_escape itself), or a framework helper that builds markup for others. + # Elsewhere the writer escapes the whole cell, so a hand-built tag renders as + # visible text -- a display bug, not an injection, and not this check's business. + in_scope_functions = FRAMEWORK_FUNCTIONS.get(rel_path) + is_sink = bool(declares) or uses_media_helper(tree) or in_scope_functions + + findings = [] + if is_sink: + found = scan_string_builders(tree, enclosing_functions(tree), + local_assignments(tree)) + if in_scope_functions is not None: + found = [f for f in found if f[1] in in_scope_functions] + findings.extend(found) + if declares and not references_escaper(tree): + findings.append(('unguarded-html-columns', '', 0, + 'module declares html_columns but calls no escaper')) + return findings, skip_reason + + +def repo_root(): + """Return the repository root, derived from this script's location.""" + return os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) + + +def scan_paths(root): + """Return the modules in scope: every artifact, plus the framework helpers.""" + paths = sorted(glob.glob(os.path.join(root, 'scripts', 'artifacts', '*.py'))) + helper = os.path.join(root, 'scripts', 'ilapfuncs.py') + if os.path.exists(helper): + paths.append(helper) + return paths + + +def main(): + """Scan for unescaped markup and remote destinations in report output.""" + parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + parser.add_argument('--list', action='store_true', dest='list_all', + help='print every finding, including baselined and allowlisted') + parser.add_argument('--verbose', action='store_true', + help='also report coverage and list counts') + parser.add_argument('--root', default=None, + help='scan this repository instead of the one this script ' + 'lives in; BASELINE/ALLOWLIST still apply, so a sibling ' + 'core reports its own findings as violations') + args = parser.parse_args() + + root = args.root or repo_root() + paths = scan_paths(root) + if not paths: + print(f'No modules found under {os.path.join(root, "scripts")}', file=sys.stderr) + return 2 + + violations = [] + known = [] + skipped = [] + fired = set() + for path in paths: + rel_path = os.path.relpath(path, root).replace(os.sep, '/') + findings, skip_reason = scan_file(path, rel_path) + if skip_reason: + skipped.append((rel_path, skip_reason)) + for rule, function, line, detail in findings: + key = (rel_path, rule, function) + fired.add(key) + entry = (rel_path, rule, function, line, detail) + if key in BASELINE or key in ALLOWLIST: + known.append(entry) + else: + violations.append(entry) + + stale_baseline = sorted(BASELINE - fired) + stale_allowlist = sorted(ALLOWLIST - fired) + + if skipped: + print(f'NOT CHECKED -- {len(skipped)} module(s) have no statically readable ' + f'__artifacts_v2__, so html_columns could not be read:') + for rel_path, reason in skipped: + print(f' {rel_path}: {reason}') + print() + + if args.verbose: + print(f'Scanned {len(paths)} module(s).') + print(f'BASELINE holds {len(BASELINE)} entr(ies); ALLOWLIST holds ' + f'{len(ALLOWLIST)}; {len(known)} finding(s) matched one this run.') + print() + + if args.list_all and known: + print(f'Known findings ({len(known)}):') + for rel_path, rule, function, line, detail in known: + print(f' {rel_path}:{line}: [{rule}] in {function}(): {detail}') + print() + + for label, stale in (('BASELINE', stale_baseline), + ('ALLOWLIST', stale_allowlist)): + if stale: + print(f'Stale {label} entr(ies) ({len(stale)}) -- these no longer match ' + f'anything and should be deleted:') + for rel_path, rule, function in stale: + print(f' {rel_path}: [{rule}] in {function}()') + print() + + if violations: + print(f'Unsafe report output ({len(violations)}):') + for rel_path, rule, function, line, detail in violations: + print(f' {rel_path}:{line}: [{rule}] in {function}(): {detail}') + print() + print(STANDARD_NOTE) + return 1 + + if stale_baseline or stale_allowlist: + print('Remove the stale entr(ies) above from ' + 'admin/scripts/check_html_safety.py.') + return 1 + + summary = (f'Checked {len(paths)} module(s): no new unsafe report output ' + f'({len(BASELINE)} baselined, {len(ALLOWLIST)} allowlisted).') + if skipped: + summary += f' {len(skipped)} module(s) NOT fully checked, listed above.' + print(summary) + return 0 + + +if __name__ == '__main__': + sys.exit(main())