fix: review findings — 3 Highs + advisory test coverage + CDX VEX per-product component - #4
Closed
MarkAtwood wants to merge 7 commits into
Closed
fix: review findings — 3 Highs + advisory test coverage + CDX VEX per-product component#4MarkAtwood wants to merge 7 commits into
MarkAtwood wants to merge 7 commits into
Conversation
cveId comes verbatim from remotely-fetched CVE records and was
interpolated into output filenames (<id>.csaf.json / <id>.cdx.json)
and the CSAF self URL without validation -- an untrusted-input ->
arbitrary-file-write vector (e.g. cveId "../ESCAPED" escaped --out-dir).
Constrain cveId to ^CVE-[0-9]{4}-[0-9]{4,}$ and --advisory-id to a
path-safe grammar before either is used to build a path.
Fixes #1
Caller-controlled ${{ inputs.* }} values were interpolated directly
into run: blocks; the runner substitutes them before the shell parses
the script, so shell metacharacters execute as code. As a reusable
workflow shipped to every product repo this distributes the footgun.
Bind each input to env: and reference "$VAR" in the script (env values
are not re-parsed by ${{ }}), add permissions: contents: read, and make
the host-path-leak grep distinguish exit 2 (no-file glob) from 1 (clean).
Fixes #2
$(dir $(lastword $(MAKEFILE_LIST))) cannot locate this fragment: Automake's include is textual, so at make time MAKEFILE_LIST is the top Makefile and SBOM_GEN resolved to <builddir>/gen-sbom, breaking `make sbom` unless every product overrode SBOM_GEN. Name the vendored directory explicitly (SBOM_VENDOR_DIR, default $(srcdir)/tools/sbom) and restore the WOLFSSL_DIR fallback for the wolfSSH-style route. Empty when neither exists so the recipe's `test -f "$(SBOM_GEN)"` fails with a clear error. Fixes #3
There was a problem hiding this comment.
Pull request overview
This PR addresses three high-severity internal review findings across the advisory generator, the reusable SBOM GitHub Actions workflow, and the autotools SBOM fragment—tightening input/path safety, hardening workflow execution, and fixing gen-sbom discovery under Automake.
Changes:
- Constrain CVE/advisory identifiers to safe grammars before using them in output filenames/URLs in
central/gen-advisory. - Harden
.github/workflows/sbom-reusable.ymlby bindingworkflow_callinputs throughenv:and improving the host-path-leak scan behavior. - Fix autotools
gen-sbomdiscovery inshare/sbom.amby switching from aMAKEFILE_LISTheuristic to an explicit vendored directory +WOLFSSL_DIRfallback.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
share/sbom.am |
Reworks autotools gen-sbom discovery to be reliable under Automake include, with WOLFSSL_DIR fallback. |
central/gen-advisory |
Validates untrusted CVE/advisory identifiers before interpolating into output paths/URLs. |
.github/workflows/sbom-reusable.yml |
Reduces script-injection risk by moving inputs into env: and hardens the host-path leak scan logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+57
to
+62
| def _validate_path_id(value, kind, pattern): | ||
| """Reject an id that is unsafe to interpolate into an output path.""" | ||
| if not pattern.match(value): | ||
| sys.exit(f"ERROR: refusing unsafe {kind} {value!r}: must match " | ||
| f"{pattern.pattern} (no path separators)") | ||
| return value |
Comment on lines
+48
to
+49
| permissions: | ||
| contents: read |
Comment on lines
+100
to
+102
| set +e | ||
| out="$(grep -REn '"/(home|Users|root)/' $OUTPUTS)" | ||
| rc=$? |
The consolidation dropped the live advisory data that gen-advisory reads by default (advisories/records/*.json + advisories/vex-overlay.json), so a bare `gen-advisory` / `make advisory` had no input. Vendor it from the wolfSSL source of truth. Also the fixture the ported test's canonical-tree case needs.
Port the 720-line stdlib gen-advisory gate (dropped in consolidation) into central/ beside gen-advisory, with its frozen testdata/ fixtures, and wire it into selftest.yml. Adds a TestPathIdValidation class that locks in the cveId/--advisory-id path-traversal fix: a record with cveId "../ESCAPED" or "/etc/pwned", or --advisory-id "../evil", is rejected and writes nothing outside --out-dir; a well-formed CVE id still generates. 66 tests (62 ported + 4 regression) pass.
Author
|
Added test coverage + the advisory data the test needs (2 more commits):
66 tests pass (62 ported + 4 new regression). This also closes the two consolidation gaps tracked internally as SBOM-gpex.14 (dropped test) and SBOM-gpex.15 (dropped overlay data). |
The CycloneDX VEX emitter collapsed every non-FIPS product onto the hardcoded wolfssl metadata.component, so a wolfSSH (or wolfMQTT, wolfTPM, ...) CVE emitted a VEX whose affects[] pointed at a component named "wolfssl" -- disagreeing with the CSAF output for the same record and mis-attributing the vulnerability. Mint a per-product component keyed on cdx_key (as the FIPS path already does) with the product's own name/cpe/purl, and reference it in affects[]; wolfssl products still use the umbrella metadata.component. Adds a regression test class covering a wolfSSH record.
MarkAtwood
changed the title
Author
|
Added a Medium fix in the same advisory subsystem (its regression test depends on the
69 advisory tests pass (66 + 3 new). |
load_overlay only json.load'd the overlay, so a hand-edited entry the schema forbids passed straight through -- above all a not_affected determination with no justification, which then degraded to a default CSAF flag and an omitted CycloneDX analysis.justification (the two VEX outputs silently disagreeing). Add a stdlib validator enforcing the overlay schema's structural invariants at load (required state, not_affected => justification for both the entry and its fips sub-object, enum membership, CVE-id keys, no unknown keys). The authoritative jsonschema pass still runs in CI; a drift test ties the validator's vocabulary to the schema file. 77 advisory tests pass (69 + 8 new).
Author
|
Another Medium in the advisory subsystem:
77 advisory tests pass (69 + 8 new). |
Author
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.
Fixes the three High-severity findings from the 2026-07-23 internal review of wolfGlass. One commit per issue.
Changes
fix(advisory)— Fixes Path traversal via unvalidated cveId in gen-advisory (arbitrary file write) #1.cveId(from remotely-fetched CVE records) and--advisory-idwere interpolated into output filenames without validation, an arbitrary-file-write vector. Now constrained to^CVE-[0-9]{4}-[0-9]{4,}$/ a path-safe grammar before use.fix(ci)— Fixes Script injection: untrusted inputs interpolated into run: in sbom-reusable.yml #2. Reusable workflow interpolated caller${{ inputs.* }}straight intorun:blocks (script injection, distributed to every product repo). Inputs bound toenv:and referenced as"$VAR"; addedpermissions: contents: read; hardened the host-path-leak grep against a no-file glob.fix(build)— Fixes Broken autotools gen-sbom discovery in sbom.am (make sbom fails) #3.sbom.amgen-sbom auto-discovery via$(MAKEFILE_LIST)is broken under Automake's textualinclude. Replaced with an explicitSBOM_VENDOR_DIRand restored theWOLFSSL_DIRfallback.Verification
cveId "../ESCAPED"now exits 1 and writes nothing outside--out-dir; a realCVE-2026-12345still emits both docs.sbom.am:SBOM_GENresolves to the vendored copy, falls back toWOLFSSL_DIR/scripts/gen-sbom, and is empty (→ clear recipe error) when neither exists.test_sbom.pygreen.Note: the
fix(advisory)change has no in-repo regression test becausetest_gen_advisory.pywas dropped during consolidation (tracked internally as SBOM-gpex.14); verified manually here.