fix(security): sanitize file paths in log messages to prevent GitHub Actions workflow command injection#2925
Conversation
…Actions workflow command injection Several log sites echo file paths derived from filesystem walk of the scanned repository without applying output.SanitizeForWorkflowCommand. On Linux file names may contain \r/\n bytes and git tracks such files, so an attacker-controlled repository (typically via pull_request-triggered osv-scanner-action) could inject ::add-mask::, ::stop-commands::, ::error::, etc. into the runner's stdout. pkg/osvscanner/scan.go:158 already applies the correct guard for the directory-name log line and its source comment explains the threat model. This change extends the same guard to the remaining unsanitized sites: - internal/config/manager.go:57,61 (config LoadPath / configPath) - internal/config/config.go:164 (LoadPath on duplicate-ignores warning) - pkg/osvscanner/osvscanner.go:401 (configFile on unused-ignores warning) - internal/sourceanalysis/rust.go:55,62,73 (rust binary path in analysis errors) Refs: https://issuetracker.google.com/issues/533784193
|
While looking at this, I noticed the same threat model also reaches a set of package-identity log sinks that this PR doesn't cover — ones that read the package name / version / ecosystem from lockfile or SBOM contents rather than from a file path. There's no global guard (the
A package name can legally contain |
|
Thanks @adilburaksen — appreciate you tracing the same class into the package-identity path. You're right that a lockfile-content-controlled sink (imodels.Name / pkgString / license overrides) is strictly a wider primitive than the file-path sinks this PR covers, and the same output.SanitizeForWorkflowCommand guard is the correct fix. For clean review + attribution, would you mind opening a separate follow-up PR for those sinks and cross-referencing this one in the description? That way each PR stays scoped (mine: filesystem-walk paths; yours: extractor-emitted identifiers) and it's clearer for the reviewers and any future audit which sites shipped in which change. Happy to review + approve on my side once you open it. If a maintainer prefers everything in one commit, I'm equally happy to merge yours into this branch — whichever the review team indicates as easier. Also cc-ing the related VRP report: https://issuetracker.google.com/issues/533784193 — if you'd like to file a separate VRP for the extractor-side sinks, they meet the same threat model. |
|
Opened as #2928 (scoped to the extractor-side identifier sinks, cross-referenced here). Thanks for the quick review offer — happy to fold it into this branch instead if the maintainers prefer a single commit. |
|
Looks good to me. Clean scope, matches the pattern from #2925, and correctly identifies the wider primitive (lockfile-content-controlled vs filename-controlled). Test coverage is a nice touch. Reviewers: #2925 and #2928 together cover the full set of GHA workflow-command-parseable log sinks I could identify — file-path sinks (#2925) and extractor-emitted identifier sinks (#2928). Merging both would close the class. |
Problem
Several log sites in osv-scanner echo file paths derived from filesystem walk of the scanned repository without applying
output.SanitizeForWorkflowCommand. On Linux, file names may contain\r/\nbytes and git tracks such files, so an attacker-controlled repository (typically viapull_request-triggeredgoogle/osv-scanner-action) could inject::add-mask::,::stop-commands::,::error::, etc. into the runner's stdout.Prior art in the same repository
pkg/osvscanner/scan.go:158already applies the correct guard for the directory-name log line, and its source comment explains the exact threat model:Fix
Extend the same guard to the remaining unsanitized sites:
internal/config/manager.go:57,61— config LoadPath / configPath (derived from lockfile path viaManager.Get(loc)→normalizeConfigLoadPath)internal/config/config.go:164—LoadPathon duplicate-ignores warningpkg/osvscanner/osvscanner.go:401—configFileon unused-ignores warninginternal/sourceanalysis/rust.go:55,62,73— Rust binary path in analysis errorsRelated VRP report
https://issuetracker.google.com/issues/533784193
No behaviour change for benign paths
SanitizeForWorkflowCommandis an identity function on strings that contain no\ror\n, so no CI or user-visible output changes for typical file paths.