sec-approval

# Security Approval Skill > Dependencies and Bugzilla API key setup live in > [`README.md`](./README.md). Point the user there if `bmo-to-md` or > `bmo-sec-approval --check-auth` fails. > **Scope:** `sec-approval` is required **only** for a vulnerability in the > **parent process that is triggerable from a content process** — typically a > bug keyworded `sec-high` + `csectype-sandbox-escape`. A recent parent-process > regression that has shipped only in Nightly is exempt when its regressing > check-in is known and Beta and ESR are marked `unaffected`. All other > security fixes can land without explicit sec-approval once their normal > review and testing requirements are met. Most media bugs fall outside the > gate because the affected code runs in the content, RDD, GMP, or Utility > process. Phase 1 still applies to every security bug; Phase 2 Step 1 decides > whether the questionnaire is needed. This skill has two phases: 1. **Compliance audit** — verify the patch follows the [Fixing Security Bugs](https://firefox-source-docs.mozilla.org/bug-mgmt/processes/fixing-security-bugs.html) guidelines. Violations are a hard gate: they **must** be resolved before proceeding. 2. **Sec-approval questionnaire** — determine whether sec-approval is required at all, and if so draft answers to the [Security Approval](https://firefox-source-docs.mozilla.org/bug-mgmt/processes/security-approval.html) questions. **Arguments:** $0 Parse the arguments: the first token that looks like a number is the **bug ID**, and any token that contains a `/` (path separator) is the **bug report path**. Both are optional and may appear in either order. --- ## Preliminary: Gather Patch Context ### Retrieve the bug Security bugs are private and **cannot** be fetched via the MCP tool `mcp__moz__get_bugzilla_bug` — it will always fail for sec-* bugs. Use the following priority order: 1. **Bug report path provided**: if the user supplied a path argument, read the summary file directly from that directory. The path points to a folder containing markdown files generated by `bmo-to-md` (look for `summary.md` or `bug-<id>.md`). 2. **Bug ID provided but no path**: use `bmo-to-md` to download the report: - Check that `bmo-to-md` is installed: run `bmo-to-md --help`. If not found, ask the user to install it: `cargo install bmo-to-md` (see <https://github.com/padenot/bmo-to-md>). - Check that a Bugzilla API key is configured (**never** read or print the key itself): ```bash python3 .claude/skills/sec-approval/bmo-sec-approval --check-auth ``` If it fails, ask the user to store their key in `~/.config/bugzilla/config.toml` (see Step 6, item 1 for instructions). - Download to a temp folder: `bmo-to-md -o /tmp/bug-<id> -a <bug_id>` - If `bmo-to-md` fails, **stop and report** to the user. 3. **Neither provided**: ask the user whether they have a local bug report (markdown files from `bmo-to-md`). If they provide a path, read it. If they provide a bug ID, go to step 2. If neither, proceed without bug context (the compliance audit can still run on the patch alone, but the questionnaire will need the user to supply missing information manually). 4. Read the summary to understand the vulnerability type, severity, and any existing comments. Note the `sec-*` keyword (sec-critical, sec-high, sec-moderate, sec-low) and the `status-firefox*` flags. ### Inspect the local changes Determine which VCS is in use and read the relevant commits: ```bash # Check if using jj or git jj --version 2>/dev/null && echo "jj" || echo "git" ``` **If jj:** ```bash jj log -T builtin_log_detailed -r 'trunk()..@' jj diff -r 'trunk()..@' ``` **If git:** ```bash git log --oneline origin/main..HEAD git diff origin/main..HEAD ``` Collect: - Commit messages (check-in comments) - All changed files - The full diff --- ## Phase 1: Compliance Audit — Fixing Security Bugs Audit the patch against **every** rule from the [Fixing Security Bugs](https://firefox-source-docs.mozilla.org/bug-mgmt/processes/fixing-security-bugs.html) guidelines. Work through each check below. For each one, report **PASS** or **FAIL** with specifics. If any check fails, present concrete fixes and ask the user to resolve them before moving to Phase 2. ### Check 1: Commit Messages Commit messages **must not** contain any of the following: - Nature of the vulnerability (overflow, use-after-free, XSS, CSP bypass, null deref, race condition, etc.) - Exploitation methods or triggering mechanisms - Security-related trigger words: "security", "exploitable", "vulnerable", "vulnerability", "CVE", "attacker", "exploit", "malicious", "crash" (when crash implies a security issue) - Security approver names - Affected Firefox versions or components in a security context - Any phrasing that makes the patched vulnerability obvious **If a commit message fails**: suggest a generic rewrite. Examples: - Bad: `Fix use-after-free in MediaDecoder::Shutdown` - Good: `Improve lifetime management in MediaDecoder` - Bad: `Fix heap buffer overflow when parsing VP9` - Good: `Add validation for buffer size in VP9 decoder` - Bad: `Prevent XSS in content process` - Good: `Strengthen input sanitization in content process` Omitting a detailed commit message entirely is acceptable — details should go in the private bug comment instead. ### Check 2: Code Comments Inline comments in the diff **must not**: - Reveal the nature of the vulnerability or exploitation vectors - Disclose that the change is security-related - Mention the bug as a security issue - Reference CVE numbers or sec-* keywords - Include stack traces, ASAN output, or crash signatures - Reference file paths or line numbers that pinpoint the flaw (e.g. `// See H265.cpp:651`) - Provide reasoning that reveals the attack vector (e.g. `// Without this check, an attacker could trigger X by doing Y`) **If comments fail**: suggest removing them or rewriting them as generic correctness/robustness comments. Security context belongs in the private bug, not in the source. ### Check 3: Security-Revealing Identifiers New or renamed variables, functions, classes, or constants in the diff **must not** reveal the vulnerability class or attack vector: - Bad: `fixUAFBuffer`, `preventOverflow`, `sanitizeXSSInput` - Good: `buffer`, `validateSize`, `sanitizeInput` - Bad: `kMaxAllocBeforeOOB`, `gNullDerefGuard` - Good: `kMaxAlloc`, `gGuard` **If identifiers fail**: suggest neutral renames that describe the *correctness* purpose, not the *security* purpose. The vulnerability context belongs in the private bug. ### Check 4: Test Cases Tests included in the patch **must not**: - Have filenames that hint at the vulnerability (e.g., `test_uaf_in_foo.html`, `test_overflow_parser.js`) - Contain comments or assertions that describe the security nature of the fix - Include exploit-like test content that demonstrates the attack vector **Test-landing policy.** Tests are no longer withheld by default. The current [Fixing Security Bugs](https://firefox-source-docs.mozilla.org/bug-mgmt/processes/fixing-security-bugs.html) guidance is that "the advent of AI tools has frequently made this precaution not worth the extra effort". Judge case by case: - **Default**: land the tests with the fix, provided they are sanitized per the rules above. - **Split the tests into a separate commit** only when the vulnerability is *unusually* hard to deduce from the patch, or *unusually* hard to trigger — i.e. when the test, not the diff, is what gives it away. - **If split and the bug shipped in a release**: the tests land at least 4 weeks after the release containing the fix goes live. Track it either by cloning a security-sensitive "land tests for bug XXXXXXX" task bug (rated `sec-other`), or by setting `in-testsuite` to `?` plus a whiteboard tag of the form `[reminder-test YYYY-MM-DD]` — Bugbot needinfos the assignee on that date; flip `in-testsuite` to `+` once the tests land. - **If the bug never shipped in a release** (development-branch-only regression): tests can land immediately on all affected branches. **If tests fail the sanitization rules**: suggest renaming or rewriting the test content. Deferring the tests is a judgment call, not an automatic requirement. ### Check 5: Try Server / CI Check whether the user has pushed (or plans to push) to Try: - **Best practice**: do not push to Try at all; test locally instead. - **If a Try push is necessary**: remind the user to: - Remove bug numbers from all commits in the Try push - Never push the bug's own vulnerability testcase; ideally push no tests - Never disclose the vulnerability nature or triggering methods in the Try push commit message or mozconfig - Fold the change in with unrelated work in the same area, so the push doesn't read as a security fix Ask the user about their Try push status. ### Check 6: Patch Obfuscation Review whether the fix can be plausibly framed as a non-security change (performance improvement, correctness fix, code cleanup). The goal is to reduce the identifiability of the security fix: - Can the fix be bundled with other unrelated work? - Does the diff look like a pure correctness or robustness improvement rather than a targeted security patch? This is advisory — report observations but do not block on it. The current process explicitly downgrades the value of obfuscation: AI tooling can analyze a fix, derive the root cause, and often build a proof of concept, so shipping the fix quickly is the primary protection. Obfuscation matters most for **Try pushes** (Check 5), where the patch is public before it lands. ### Compliance Verdict Present a summary table: | Check | Status | Details | | ----------------- | ------------- | ------- | | Commit messages | PASS/FAIL | ... | | Code comments | PASS/FAIL | ... | | Identifiers | PASS/FAIL | ... | | Test cases | PASS/FAIL | ... | | Try server | PASS/FAIL/N/A | ... | | Patch obfuscation | Advisory | ... | Then present the checklist for the user to confirm: - [ ] Commit message is not security-revealing - [ ] No security-revealing inline comments in the patch - [ ] No security-revealing identifiers in the patch - [ ] Tests are sanitized (split into a follow-up only if the test, not the diff, is what reveals the flaw) - [ ] Not pushed to Try with bug number / security tests - [ ] Bug is filed as restricted/sec-* on Bugzilla **If any check is FAIL**: present the specific violations with suggested fixes. Ask the user if they want help fixing them now. Re-audit after fixes. **This is a hard gate.** Ask the user to confirm all checklist items pass before proceeding. Do not proceed to Phase 2 until Checks 1–5 all pass and the user gives explicit approval to continue. --- ## Phase 2: Security Approval Questionnaire ### Step 1: Is sec-approval Required at All? The gate is narrow: **sec-approval is only required for a vulnerability in the parent process that is triggerable from a content process** — typically a bug keyworded `sec-high` **and** `csectype-sandbox-escape`. Previously all sec-high bugs needed approval; that rule is now inverted. Decide from the actual affected and triggering processes, not from severity or an IPC actor's name alone. `sec-high` by itself does not require approval, and `csectype-sandbox-escape` is supporting evidence rather than a substitute for checking the process boundary. **No sec-approval needed** if any of these hold: 1. Rating is **sec-low**, **sec-moderate**, **sec-other**, or **sec-want** 2. The flaw only affects the **content process**, regardless of a `sec-high` rating 3. The flaw only affects some other **non-parent** process (GPU, RDD, GMP, Utility, Socket, …), regardless of a `sec-high` rating 4. It is a cross-process bug whose **target** process is not the parent — the correct keyword for that case is `csectype-priv-escalation`, **not** `csectype-sandbox-escape`. Fix the keyword if you see it used wrongly. 5. It is a parent-process bug but a **recent unshipped regression** on mozilla-central: a specific regressing check-in is identified, the ESR and Beta status flags are marked `unaffected`, and the vulnerability only shipped in Nightly builds **Work out which process the flaw actually runs in** — for media code this is usually *not* the parent process: - Decoding (`dom/media/platforms/`, `dom/media/ipc/`) runs in the **RDD** or **Utility** process; EME/CDM code runs in the **GMP** process; the playback pipeline mostly runs in the **content** process. - Parent-process media code does exist: `MediaManager` / device enumeration, the `*ProcessHost` / `*ProcessParent` process-launch and IPC-bridging code, permission and pref plumbing, and code under `toolkit/` or `browser/`. - **Careful with IPDL naming**: a `…Parent` actor is the parent *side of the protocol*, which usually lives in the RDD/GPU/Utility process, not in the parent process — `RDDParent` and `RemoteDecoderManagerParent` run in the RDD process. Check where the actor is actually constructed. - The case that *does* need approval: the parent process mishandling data or a message that a content process controls. If the bug is unrated, rate it following the [Client Severity Guidelines](https://wiki.mozilla.org/Security_Severity_Ratings/Client) rather than defaulting to worst-case. State the determination to the user with the reason, e.g. "No sec-approval required: sec-high, but the flaw is in the RDD process (`csectype-priv-escalation`), not the parent process." If sec-approval is not required, stop after reporting the Phase 1 audit and this determination. Do not draft the questionnaire or offer to set the flag unless the user explicitly asks for a draft anyway. If the answer is genuinely unclear — an ambiguous process boundary, or an unrated bug that might reach the parent — the docs are explicit: request sec-approval anyway and move on. "Don't overthink it!" ### Step 2: Answer the Questionnaire Work through all questions systematically by examining the diff and commit messages gathered in the Preliminary step. #### Q1: Patch Visibility **Question**: "How easily could an exploit be constructed based on the patch?" (The source docs phrase this as "How easily can the security issue be deduced from the patch?" — cover both senses: how visible the flaw is in the diff, and how much work remains to weaponize it.) Analyze: - Does the diff clearly show a specific memory safety fix (e.g., bounds check, null check, free ordering fix)? - Are variable names, function names, or code structure self-explanatory about the vulnerability class? - Could a malicious actor trivially write an exploit by reading the diff? Rate as one of: - **Easy**: The trigger is obvious from the patch and reproducible with standard web APIs alone; no special timing or heap work needed. A security researcher would immediately understand the vulnerability class and location. - **Moderate**: The trigger is discoverable from the patch + public API knowledge, but reliable exploitation needs one additional factor (timing, heap shaping, or a non-default condition). - **Difficult**: The trigger is non-obvious from the patch and requires independent discovery; OR reliable exploitation needs two or more independent factors (e.g. non-obvious event vector + deterministic GC timing + heap shaping). For race conditions, also use Difficult when the patch only fixes one side of the race — the other racing operation and the API sequence needed to reproduce the window are not disclosed by the patch. Always open the answer with one of "Easy.", "Moderate.", or "Difficult.", followed by 2-3 sentences of justification. Base the answer on the patch itself — what the fix reveals about the vulnerable code path — and on whether an attacker could recreate the conditions using only publicly available web APIs. #### Q2: Comments and Tests as Bulls-Eyes **Question**: "Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?" By this point, Phase 1 should have already caught and resolved any bulls-eyes. Confirm that: - Commit messages are clean (verified in Check 1) - Code comments are clean (verified in Check 2) - Test files are sanitized, or split into a follow-up (verified in Check 4) Report the current state — ideally "No, the patch has been reviewed for information leaks." #### Q3: Affected Branches **Question**: "Which branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?" **Fetch the current release calendar** to determine which versions are on each channel: ``` WebFetch: https://whattrainisitnow.com/calendar/ ``` Extract the current Nightly, Beta, Release, and ESR version numbers. **Check the current Firefox version** from the local tree: ```bash cat config/milestone.txt ``` Check `status-firefox*` flags from the bug (if fetched). If not available, inspect the code history to find when the vulnerable code was introduced: **If git:** ```bash git log --oneline --follow -S "<key_symbol>" -- <affected-file> | head -10 ``` **If jj:** ```bash jj log -r 'ancestors(trunk())' -T builtin_log_oneline -p -s -- <affected-file> | head -50 ``` Cross-reference with the `status-firefoxNN: affected/unaffected/fixed` flags on the bug if available. **Verify reachability on each branch.** The vulnerable code may exist on a branch but be unreachable if it is gated behind a preference or feature flag that is disabled on that branch. For each affected ESR and release branch: 1. **Check the pref/flag value on the branch itself** — do not assume the current trunk value applies to older branches: ```bash