frankenbit/release-toolkit

Fork 0

security: verify sed-rewrite injection surface in bake_toolkit_ref (Sprint 5 security audit finding) #180

New issue

Closed

opened 2026-06-27 12:11:03 +02:00 by quartermaster · 1 comment

quartermaster commented

2026-06-27 12:11:03 +02:00

Owner

Audit finding

Sprint 5 security audit (#156) flagged a potential sed-injection surface in scripts/lib/build_bake.sh:50:

sed -i "s|'[^']*'  # release-toolkit-build-ref|'${new_ref}'  # release-toolkit-build-ref|" "$file"

If $new_ref contains a literal | (the sed delimiter), the sed expression splits into multiple commands, enabling sed-side injection. Agent's framing: tag_format config could be poisoned to produce a $new_ref containing |.

Verify-at-source needed (Surveyor discipline)

The agent's finding may have conflated the immediate new_ref source. Tracing:

bake_toolkit_ref "$f" "$NEW_TAG" is invoked from scripts/release-prep.sh (after section 8c)
NEW_TAG is computed by semver_bump (scripts/lib/semver.sh) — validated SemVer output
SemVer regex bounds: v[0-9]+\.[0-9]+\.[0-9]+(-[A-Za-z0-9.-]+)?

If NEW_TAG is genuinely SemVer-bounded, the | character can't appear → no immediate injection.

The agent's referenced tag_format config (scripts/lib/config.sh:307) may be an indirect path — verify whether tag_format influences NEW_TAG's value at any step in the cut flow.

Action items (in order of verification rigor)

Trace NEW_TAG's provenance from semver_bump to bake_toolkit_ref's call site. Are there any intermediate string-templating steps that could inject special characters?
Audit tag_format config handling: if tag_format ISN'T part of NEW_TAG's computation, document the irrelevance + close. If it IS, confirm the injection feasibility.
Defensive hardening (regardless): bound new_ref in bake_toolkit_ref itself via a [[ "$new_ref" =~ ^[A-Za-z0-9._-]+$ ]] check. Cheap defense-in-depth; no functional impact for legitimate refs (which are all SemVer-bounded).
Bats coverage: add mutation-verify test asserting bake_toolkit_ref "$f" "v0.13.0|MALICIOUS" is REJECTED (not silently sed-injected).

Composition

scripts/lib/build_bake.sh is the file in question (shipped in v0.13.0 #173)
Phase 2 (#172) drops the toolkit_ref input — the bake mechanism (and this attack surface) PERSISTS post-Phase-2
Sister tracker for path-traversal finding: see separate Sprint 5 security tracker (linked)

What this PR will NOT do

Will NOT change the bake mechanism's correctness (already proven by v0.13.0 cut)
Will NOT touch the tag_format config schema beyond audit findings
Will NOT add new ADR text unless the audit surfaces a substrate decision

Refs

Sprint 5 #156 security audit
v0.13.0 cut empirical close (the substrate being audited)
AGENTS.md section 2 "Build-bake" subsection

## Audit finding Sprint 5 security audit (#156) flagged a potential sed-injection surface in `scripts/lib/build_bake.sh:50`: ```bash sed -i "s|'[^']*' # release-toolkit-build-ref|'${new_ref}' # release-toolkit-build-ref|" "$file" ``` If `$new_ref` contains a literal `|` (the sed delimiter), the sed expression splits into multiple commands, enabling sed-side injection. Agent's framing: `tag_format` config could be poisoned to produce a `$new_ref` containing `|`. ## Verify-at-source needed (Surveyor discipline) The agent's finding may have conflated the immediate `new_ref` source. Tracing: - `bake_toolkit_ref "$f" "$NEW_TAG"` is invoked from `scripts/release-prep.sh` (after section 8c) - `NEW_TAG` is computed by `semver_bump` (`scripts/lib/semver.sh`) — validated SemVer output - SemVer regex bounds: `v[0-9]+\.[0-9]+\.[0-9]+(-[A-Za-z0-9.-]+)?` If `NEW_TAG` is genuinely SemVer-bounded, the `|` character can't appear → no immediate injection. The agent's referenced `tag_format` config (`scripts/lib/config.sh:307`) may be an indirect path — verify whether `tag_format` influences `NEW_TAG`'s value at any step in the cut flow. ## Action items (in order of verification rigor) 1. **Trace `NEW_TAG`'s provenance** from semver_bump to `bake_toolkit_ref`'s call site. Are there any intermediate string-templating steps that could inject special characters? 2. **Audit `tag_format` config handling**: if `tag_format` ISN'T part of `NEW_TAG`'s computation, document the irrelevance + close. If it IS, confirm the injection feasibility. 3. **Defensive hardening (regardless)**: bound `new_ref` in `bake_toolkit_ref` itself via a `[[ "$new_ref" =~ ^[A-Za-z0-9._-]+$ ]]` check. Cheap defense-in-depth; no functional impact for legitimate refs (which are all SemVer-bounded). 4. **Bats coverage**: add mutation-verify test asserting `bake_toolkit_ref "$f" "v0.13.0|MALICIOUS"` is REJECTED (not silently sed-injected). ## Composition - `scripts/lib/build_bake.sh` is the file in question (shipped in v0.13.0 #173) - Phase 2 (#172) drops the `toolkit_ref` input — the bake mechanism (and this attack surface) PERSISTS post-Phase-2 - Sister tracker for path-traversal finding: see separate Sprint 5 security tracker (linked) ## What this PR will NOT do - Will NOT change the bake mechanism's correctness (already proven by v0.13.0 cut) - Will NOT touch the `tag_format` config schema beyond audit findings - Will NOT add new ADR text unless the audit surfaces a substrate decision ## Refs - Sprint 5 #156 security audit - v0.13.0 cut empirical close (the substrate being audited) - AGENTS.md section 2 "Build-bake" subsection

quartermaster referenced this issue

2026-06-27 12:13:37 +02:00

docs: refresh README + integration.md to v0.13.0 (closes #182) #183

quartermaster commented

2026-06-27 12:17:36 +02:00

Author

Owner

Correction per Surveyor b316 verify-at-source

My PR-body framing dismissed the agent's finding as "NEW_TAG comes from semver_bump's validated regex, not tag_format." That reasoning is WRONG. Surveyor traced the actual data flow at source:

NEW_TAG = config_render_tag("$NEW_VERSION", config) (release-prep.sh:266)
config_render_tag = ${fmt//\{version\}/$version} — substitutes the validated version INTO the tag_format TEMPLATE
config.sh:307 literally says tag_format is unvalidated

So:

The {version} substitution input IS validated (semver) ✓
The tag_format TEMPLATE (literal chars around {version}) is UNVALIDATED and flows verbatim into NEW_TAG → bake_toolkit_ref's sed
A tag_format like 'v{version}|evil' → NEW_TAG='v0.13.0|evil' → sed delimiter collision / injection

The agent's data-flow finding is correct. The discipline failure is mine: substrate-of-record verify failed (verified my mental model rather than reading config_render_tag).

Reachability bound (the LOW-risk framing — also from Surveyor b316)

bake_toolkit_ref no-ops for external consumers via the [[ ! -f "$file" ]] file-guard (build_bake.sh:42). The injection is TOOLKIT-SELF-ONLY — only reachable where the maintainers control tag_format (the toolkit's own release-toolkit.yml).

External consumers' tag_format → bake is a no-op for them. So it's self-harm threat model = LOW risk, but REAL.

Updated mitigation

Sanitize new_ref in bake_toolkit_ref (not at the config layer) — reject/escape sed-meta chars, especially the | delimiter. One guard closes the class regardless of tag_format provenance.

Specifically:

bake_toolkit_ref() {
    local file="${1:?file required}"
    local new_ref="${2:?new_ref required}"
    [[ ! -f "$file" ]] && return 0
    # Defense-in-depth: reject sed-meta chars in new_ref. The bake mechanism
    # only consumes refs (semver-shape: vX.Y.Z[-rc.N]), and any `|`, `'`,
    # `\`, `&`, `$` in the value indicates either a misconfigured tag_format
    # template (the validated-version-into-unvalidated-template injection
    # surface per Surveyor b316) or an upstream bug.
    if [[ "$new_ref" =~ [\|\\\'\&\$] ]]; then
        log "build-bake: refusing to bake illegal new_ref=$new_ref (sed-meta chars rejected; see release-toolkit#180)"
        return 1
    fi
    sed -i "s|'[^']*'  # release-toolkit-build-ref|'${new_ref}'  # release-toolkit-build-ref|" "$file"
}

Plus bats coverage: a mutation-verify fixture that passes 'v0.13.0|MALICIOUS' and asserts the function returns non-zero + leaves the file unchanged.

Updated severity

LOW-RISK-BUT-REAL — not "validation-bounded" (it's NOT validated) but "reachability-bounded" (file-guard limits to toolkit-self). The defense-in-depth guard is the right fix because it closes the class regardless of the data-flow's upstream story.

Discipline lesson banked

The verify-at-source culture goes BOTH WAYS: my agent dispatched the audit, the agent found the issue, I second-guessed the agent based on my mental model of the data flow. Surveyor verified at source and the agent was right. The next agent-flag I'm tempted to dismiss as "agent overreach" needs the same trace-at-source rigor I apply to substrate claims I want to believe.

This is also the calibration banked earlier in the session: "detector-output that confirms what I want to believe gets the SAME independent-probe rigor as detector-output that contradicts it." Add a sibling: "detector-output that I'm tempted to DISMISS gets the same rigor too."

## Correction per Surveyor b316 verify-at-source My PR-body framing dismissed the agent's finding as "NEW_TAG comes from semver_bump's validated regex, not tag_format." **That reasoning is WRONG**. Surveyor traced the actual data flow at source: - `NEW_TAG = config_render_tag("$NEW_VERSION", config)` (release-prep.sh:266) - `config_render_tag` = `${fmt//\{version\}/$version}` — substitutes the validated version INTO the tag_format TEMPLATE - `config.sh:307` literally says `tag_format` is unvalidated So: - The `{version}` substitution input IS validated (semver) ✓ - The `tag_format` TEMPLATE (literal chars around `{version}`) is UNVALIDATED and flows verbatim into NEW_TAG → bake_toolkit_ref's sed - A tag_format like `'v{version}|evil'` → `NEW_TAG='v0.13.0|evil'` → sed delimiter collision / injection **The agent's data-flow finding is correct.** The discipline failure is mine: substrate-of-record verify failed (verified my mental model rather than reading config_render_tag). ## Reachability bound (the LOW-risk framing — also from Surveyor b316) `bake_toolkit_ref` no-ops for external consumers via the `[[ ! -f "$file" ]]` file-guard (build_bake.sh:42). The injection is TOOLKIT-SELF-ONLY — only reachable where the maintainers control `tag_format` (the toolkit's own `release-toolkit.yml`). External consumers' `tag_format` → bake is a no-op for them. So it's **self-harm threat model = LOW risk**, but REAL. ## Updated mitigation Sanitize `new_ref` in `bake_toolkit_ref` (not at the config layer) — reject/escape sed-meta chars, especially the `|` delimiter. **One guard closes the class regardless of `tag_format` provenance**. Specifically: ```bash bake_toolkit_ref() { local file="${1:?file required}" local new_ref="${2:?new_ref required}" [[ ! -f "$file" ]] && return 0 # Defense-in-depth: reject sed-meta chars in new_ref. The bake mechanism # only consumes refs (semver-shape: vX.Y.Z[-rc.N]), and any `|`, `'`, # `\`, `&`, `$` in the value indicates either a misconfigured tag_format # template (the validated-version-into-unvalidated-template injection # surface per Surveyor b316) or an upstream bug. if [[ "$new_ref" =~ [\|\\\'\&\$] ]]; then log "build-bake: refusing to bake illegal new_ref=$new_ref (sed-meta chars rejected; see release-toolkit#180)" return 1 fi sed -i "s|'[^']*' # release-toolkit-build-ref|'${new_ref}' # release-toolkit-build-ref|" "$file" } ``` Plus bats coverage: a mutation-verify fixture that passes `'v0.13.0|MALICIOUS'` and asserts the function returns non-zero + leaves the file unchanged. ## Updated severity LOW-RISK-BUT-REAL — not "validation-bounded" (it's NOT validated) but "reachability-bounded" (file-guard limits to toolkit-self). The defense-in-depth guard is the right fix because it closes the class regardless of the data-flow's upstream story. ## Discipline lesson banked The verify-at-source culture goes BOTH WAYS: my agent dispatched the audit, the agent found the issue, I second-guessed the agent based on my mental model of the data flow. Surveyor verified at source and the agent was right. The next agent-flag I'm tempted to dismiss as "agent overreach" needs the same trace-at-source rigor I apply to substrate claims I want to believe. This is also the calibration banked earlier in the session: "detector-output that confirms what I want to believe gets the SAME independent-probe rigor as detector-output that contradicts it." Add a sibling: "detector-output that I'm tempted to DISMISS gets the same rigor too."

quartermaster self-assigned this

2026-06-27 14:22:42 +02:00

quartermaster referenced this issue from a commit

2026-06-27 14:27:13 +02:00

fix(build_bake): defense-in-depth ref-shape validation (closes #180)

~~quartermaster referenced this issue 2026-06-27 14:27:35 +02:00~~

chore(release): v0.13.2 #189

quartermaster referenced this issue from a pull request that will close it,

2026-06-27 14:32:47 +02:00

fix(config): reject path-traversal in version_files (closes #181) #190

quartermaster referenced this issue from a commit

2026-06-27 14:33:26 +02:00