fix(advisory): validate ids used in output paths (path traversal) - #8
Open
MarkAtwood wants to merge 1 commit into
Open
fix(advisory): validate ids used in output paths (path traversal)#8MarkAtwood wants to merge 1 commit into
MarkAtwood wants to merge 1 commit 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
Adds a TestPathIdValidation regression class.
Fixes #1
There was a problem hiding this comment.
Pull request overview
This PR mitigates an arbitrary-file-write (path traversal) risk in central/gen-advisory by validating untrusted identifiers (cveId from remotely-fetched CVE records and --advisory-id) before they are interpolated into output filenames and self URLs.
Changes:
- Add strict regex validation for CVE IDs and advisory IDs used in output paths.
- Enforce validation when parsing CVE records and when processing
--advisory-id. - Add CLI-level regression tests to ensure traversal attempts are rejected and do not write escaped files.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| central/gen-advisory | Adds ID validation helpers/regexes and enforces them for cveId and --advisory-id before building output paths. |
| central/test_gen_advisory.py | Adds end-to-end CLI regression tests covering traversal/absolute-path rejection and a valid-ID success case. |
Comments suppressed due to low confidence (1)
central/test_gen_advisory.py:768
- The "well formed" CVE id acceptance test only checks for the CSAF output file, but batch mode emits both CSAF and CycloneDX documents. Adding an assertion for the .cdx.json output ensures the happy-path actually produced the full expected output set.
r = self._run(['--cve-record', rec, '--out-dir', d])
self.assertEqual(r.returncode, 0, r.stderr)
self.assertTrue(
os.path.exists(os.path.join(d, 'CVE-2026-12345.csaf.json')))
💡 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
+746
to
+749
| # The escaped path (sibling of out/, i.e. d/out/ESCAPED.*) must | ||
| # not have been written. | ||
| escaped = os.path.join(d, 'out', 'ESCAPED.csaf.json') | ||
| self.assertFalse(os.path.exists(escaped), escaped) |
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 #1.
cveId(from remotely-fetched CVE records) and--advisory-idwere interpolated into output filenames without validation — an arbitrary-file-write vector (cveId "../ESCAPED"escaped--out-dir). Now constrained to^CVE-[0-9]{4}-[0-9]{4,}$/ a path-safe grammar. AddsTestPathIdValidation.feat/advisory-test-infra) — its regression test lives in the suite ported there. Merge #7 first; GitHub will retarget this to master afterward.