From e72044b6108c71654217d1f3684fe6e1060286c8 Mon Sep 17 00:00:00 2001 From: Miguel Jacq Date: Tue, 30 Jun 2026 12:11:48 +1000 Subject: [PATCH] Ensure markdown README gets sanitised --- enroll/ansible.py | 10 +++- enroll/cm.py | 61 ++++++++++++++++++++--- tests/test_markdown_sanitize.py | 87 +++++++++++++++++++++++++++++++++ 3 files changed, 150 insertions(+), 8 deletions(-) create mode 100644 tests/test_markdown_sanitize.py diff --git a/enroll/ansible.py b/enroll/ansible.py index 75e077a..76399f9 100644 --- a/enroll/ansible.py +++ b/enroll/ansible.py @@ -8,7 +8,13 @@ from dataclasses import dataclass from pathlib import Path from typing import Any, Dict, List, Optional, Set, Tuple -from .cm import CMModule, markdown_list, snapshot_excluded_lines, snapshot_note_lines +from .cm import ( + CMModule, + markdown_list, + sanitize_markdown_text, + snapshot_excluded_lines, + snapshot_note_lines, +) from .jinjaturtle import ( jinjify_managed_files as _jinjify_managed_files, resolve_jinjaturtle_mode, @@ -824,7 +830,7 @@ def _render_readme( fqdn: Optional[str] = None, ) -> str: host = state.get("host", {}) if isinstance(state.get("host"), dict) else {} - hostname = host.get("hostname") or "unknown" + hostname = sanitize_markdown_text(host.get("hostname") or "unknown") or "unknown" roles = state.get("roles", {}) if isinstance(state.get("roles"), dict) else {} excluded = snapshot_excluded_lines(roles) notes = snapshot_note_lines(roles) diff --git a/enroll/cm.py b/enroll/cm.py index 9dc4700..e9b0208 100644 --- a/enroll/cm.py +++ b/enroll/cm.py @@ -1,5 +1,6 @@ from __future__ import annotations +import re import shlex from dataclasses import dataclass, field from pathlib import Path @@ -713,8 +714,53 @@ def role_order_key(role: str) -> tuple[int, str]: return (priority.get(role, 50), role) +# Control characters (excluding ordinary tab) that must never reach generated +# documentation. A raw newline/carriage return in a harvested value would let it +# break out of a Markdown list item or code span and inject new document +# structure (a fake heading, a misleading link/command block); other C0/C1 +# control bytes can smuggle terminal escape sequences when the README is printed. +_MARKDOWN_CONTROL_RE = re.compile(r"[\x00-\x08\x0b-\x1f\x7f-\x9f]") + + +def sanitize_markdown_text(value: Any) -> str: + """Neutralise harvested text before it is spliced into generated Markdown. + + Generated docs (the Ansible ``README.md``) embed harvested, attacker- + influenceable values such as the host name and captured file paths. These + are not executed by Ansible, but a value containing a newline, carriage + return, backtick, or control byte could otherwise break out of its + surrounding list item / code span and inject misleading Markdown structure + (a forged heading, a deceptive ``[link](...)``/command block) or a terminal + escape sequence when the file is viewed. This collapses any whitespace run + (including newlines and tabs) to a single space, drops other control bytes, + and replaces backticks with a similar-looking single quote so a value can + never escape an inline code span. It is deliberately lossy: the README is a + human-readable summary, and faithful representation of hostile bytes there + is not a goal. + """ + + text = str(value) + # Collapse any run of whitespace (newlines, CR, tabs, spaces) to one space so + # a harvested value stays on a single Markdown line / inside one code span. + text = re.sub(r"\s+", " ", text) + # Drop remaining control characters that survived the whitespace collapse. + text = _MARKDOWN_CONTROL_RE.sub("", text) + # A backtick would close an inline code span and let following characters be + # interpreted as Markdown; swap it for a visually-similar acute accent. + text = text.replace("`", "\u00b4") + return text.strip() + + def markdown_list(items: Iterable[Any], *, empty: str = "None.") -> str: - values = [str(item) for item in items if str(item)] + """Render already-composed Markdown list lines. + + Callers that embed harvested values (``snapshot_note_lines``, + ``snapshot_excluded_lines``, ``path_reason_lines``) sanitise those values + with :func:`sanitize_markdown_text` before composing each line, so this + helper only joins lines it is given. It still drops empty entries. + """ + + values = [str(item) for item in items if str(item).strip()] return "\n".join(f"- {item}" for item in values) or f"- {empty}" @@ -723,10 +769,10 @@ def path_reason_lines( ) -> List[str]: lines: List[str] = [] for item in items or []: - path = str(item.get(source_key) or "") + path = sanitize_markdown_text(item.get(source_key) or "") if not path: continue - reason = str(item.get("reason") or "") + reason = sanitize_markdown_text(item.get("reason") or "") lines.append(f"{path} ({reason})" if reason else path) return lines @@ -744,17 +790,20 @@ def iter_role_snapshots(roles: Mapping[str, Any]) -> Iterator[Mapping[str, Any]] def snapshot_note_lines(roles: Mapping[str, Any]) -> List[str]: notes: List[str] = [] for snap in iter_role_snapshots(roles): - source = str( + source = sanitize_markdown_text( snap.get("role_name") or snap.get("unit") or snap.get("package") or "role" ) - notes.extend(f"`{source}`: {note}" for note in snap.get("notes", []) or []) + notes.extend( + f"`{source}`: {sanitize_markdown_text(note)}" + for note in snap.get("notes", []) or [] + ) return notes def snapshot_excluded_lines(roles: Mapping[str, Any]) -> List[str]: excluded: List[str] = [] for snap in iter_role_snapshots(roles): - source = str( + source = sanitize_markdown_text( snap.get("role_name") or snap.get("unit") or snap.get("package") or "role" ) for line in path_reason_lines(snap.get("excluded", []) or []): diff --git a/tests/test_markdown_sanitize.py b/tests/test_markdown_sanitize.py new file mode 100644 index 0000000..2d7c03f --- /dev/null +++ b/tests/test_markdown_sanitize.py @@ -0,0 +1,87 @@ +"""Security regression: harvested values embedded in the generated Ansible +``README.md`` must be neutralised so an attacker-influenceable host name, file +path, or note cannot inject Markdown structure (a forged heading, deceptive +link/command block) or a terminal escape sequence into the documentation. + +Harvested values are not executed by Ansible, but the README is the +human-readable summary an operator reads before applying a manifest, so a value +that breaks out of its surrounding list item / inline code span and forges +document structure is misleading and must be prevented. +""" + +from __future__ import annotations + +from enroll.cm import ( + markdown_list, + sanitize_markdown_text, + snapshot_excluded_lines, + snapshot_note_lines, +) + + +def test_sanitize_collapses_newlines_and_strips_controls(): + # A newline would let a value break out of its Markdown line; a backtick + # would close an inline code span; ESC/BEL are terminal escape vectors. + payload = "host`\n## Injected\n[x](http://evil)\n`y\x1b[31mRED\x07\ttab" + out = sanitize_markdown_text(payload) + + assert "\n" not in out and "\r" not in out + assert "`" not in out # backticks replaced so a code span cannot be closed + assert "\x1b" not in out and "\x07" not in out # control bytes stripped + # The visible text is preserved on a single line (heading marker is now inert + # because it no longer starts a line). + assert "## Injected" in out + assert "http://evil" in out + + +def test_sanitize_is_idempotent_and_handles_non_str(): + once = sanitize_markdown_text("a\nb") + assert sanitize_markdown_text(once) == once + assert sanitize_markdown_text(1234) == "1234" + assert sanitize_markdown_text(None) == "None" + + +def test_markdown_list_does_not_gain_extra_lines_from_payload(): + # Even if a pre-composed line still contained a newline, markdown_list must + # not emit more bullets than it was given. + out = markdown_list(["clean line", "two\nlines should be one bullet"]) + # Two input items -> at most two leading "- " bullets. + assert out.count("\n- ") <= 1 + + +def test_snapshot_note_lines_neutralises_injected_note(): + roles = { + "etc_custom": { + "role_name": "etc_custom", + "notes": ["benign`\n## PWNED\n- fake bullet\x1b[5m"], + } + } + lines = snapshot_note_lines(roles) + assert len(lines) == 1 + line = lines[0] + # The role name code span is intact; the payload is folded onto one line + # with backticks and control bytes removed. + assert line.startswith("`etc_custom`: ") + assert "\n" not in line + assert "\x1b" not in line + # Only the (structural) backticks around the role name remain -- the + # payload's backtick was neutralised. + assert line.count("`") == 2 + + +def test_snapshot_excluded_lines_neutralises_injected_path(): + roles = { + "etc_custom": { + "role_name": "etc_custom", + "excluded": [ + {"path": "/etc/`\n## fake-heading\nx", "reason": "denied_path"} + ], + } + } + lines = snapshot_excluded_lines(roles) + assert len(lines) == 1 + line = lines[0] + assert "\n" not in line + assert line.startswith("`etc_custom`: ") + assert line.count("`") == 2 # only the structural role-name span + assert "(denied_path)" in line