x-pack/osquerybeat: prevent path traversal in .pkg CPIO extraction - #51446
Conversation
The macOS .pkg installer for osquery is extracted by expandPayload() in pkgutil/expand.go, which reads CPIO entries and joins each entry's FilePath directly onto the destination directory. The entry name comes unsanitized from the archive, so a crafted .pkg containing "../" components could write files outside the destination directory (CWE-22). This was inconsistent with the sibling tar and zip extractors, which already validate entry paths. Reject any entry whose name is not local using filepath.IsLocal before joining it with the destination directory. IsLocal also rejects absolute paths and (on Windows) reserved names, making it stricter than a manual prefix check while keeping the intent obvious. Add unit tests covering normal extraction and several path-traversal attempts to lock in the behavior. Assisted-By: Cursor
🤖 GitHub commentsJust comment with:
|
|
This pull request does not have a backport label.
To fixup this pull request, you need to add the backport labels for the needed
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthrough
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Pinging @elastic/sec-windows-platform (Team:Security-Windows Platform) |
Switch the cpio header helper to fmt.Fprintf(&buf, ...) to satisfy staticcheck QF1012, and add an absolute-path (/escape.txt) traversal case to round out the coverage. Assisted-By: Cursor
|
Tick the box to add this pull request to the merge queue (same as
|
|
@Mergifyio backport 9.5 9.4 9.3 8.19 |
✅ Backports have been createdDetails
|
…51446) (#52130) * x-pack/osquerybeat: prevent path traversal in .pkg CPIO extraction The macOS .pkg installer for osquery is extracted by expandPayload() in pkgutil/expand.go, which reads CPIO entries and joins each entry's FilePath directly onto the destination directory. The entry name comes unsanitized from the archive, so a crafted .pkg containing "../" components could write files outside the destination directory (CWE-22). This was inconsistent with the sibling tar and zip extractors, which already validate entry paths. Reject any entry whose name is not local using filepath.IsLocal before joining it with the destination directory. IsLocal also rejects absolute paths and (on Windows) reserved names, making it stricter than a manual prefix check while keeping the intent obvious. Add unit tests covering normal extraction and several path-traversal attempts to lock in the behavior. Assisted-By: Cursor * osquerybeat: address review feedback on pkg traversal test Switch the cpio header helper to fmt.Fprintf(&buf, ...) to satisfy staticcheck QF1012, and add an absolute-path (/escape.txt) traversal case to round out the coverage. Assisted-By: Cursor (cherry picked from commit 5041330) Co-authored-by: Marc Guasch <marc-gr@users.noreply.github.com>
…51446) (#52129) * x-pack/osquerybeat: prevent path traversal in .pkg CPIO extraction The macOS .pkg installer for osquery is extracted by expandPayload() in pkgutil/expand.go, which reads CPIO entries and joins each entry's FilePath directly onto the destination directory. The entry name comes unsanitized from the archive, so a crafted .pkg containing "../" components could write files outside the destination directory (CWE-22). This was inconsistent with the sibling tar and zip extractors, which already validate entry paths. Reject any entry whose name is not local using filepath.IsLocal before joining it with the destination directory. IsLocal also rejects absolute paths and (on Windows) reserved names, making it stricter than a manual prefix check while keeping the intent obvious. Add unit tests covering normal extraction and several path-traversal attempts to lock in the behavior. Assisted-By: Cursor * osquerybeat: address review feedback on pkg traversal test Switch the cpio header helper to fmt.Fprintf(&buf, ...) to satisfy staticcheck QF1012, and add an absolute-path (/escape.txt) traversal case to round out the coverage. Assisted-By: Cursor (cherry picked from commit 5041330) Co-authored-by: Marc Guasch <marc-gr@users.noreply.github.com>
…51446) (#52127) * x-pack/osquerybeat: prevent path traversal in .pkg CPIO extraction The macOS .pkg installer for osquery is extracted by expandPayload() in pkgutil/expand.go, which reads CPIO entries and joins each entry's FilePath directly onto the destination directory. The entry name comes unsanitized from the archive, so a crafted .pkg containing "../" components could write files outside the destination directory (CWE-22). This was inconsistent with the sibling tar and zip extractors, which already validate entry paths. Reject any entry whose name is not local using filepath.IsLocal before joining it with the destination directory. IsLocal also rejects absolute paths and (on Windows) reserved names, making it stricter than a manual prefix check while keeping the intent obvious. Add unit tests covering normal extraction and several path-traversal attempts to lock in the behavior. Assisted-By: Cursor * osquerybeat: address review feedback on pkg traversal test Switch the cpio header helper to fmt.Fprintf(&buf, ...) to satisfy staticcheck QF1012, and add an absolute-path (/escape.txt) traversal case to round out the coverage. Assisted-By: Cursor (cherry picked from commit 5041330) Co-authored-by: Marc Guasch <marc-gr@users.noreply.github.com>
…51446) (#52128) * x-pack/osquerybeat: prevent path traversal in .pkg CPIO extraction The macOS .pkg installer for osquery is extracted by expandPayload() in pkgutil/expand.go, which reads CPIO entries and joins each entry's FilePath directly onto the destination directory. The entry name comes unsanitized from the archive, so a crafted .pkg containing "../" components could write files outside the destination directory (CWE-22). This was inconsistent with the sibling tar and zip extractors, which already validate entry paths. Reject any entry whose name is not local using filepath.IsLocal before joining it with the destination directory. IsLocal also rejects absolute paths and (on Windows) reserved names, making it stricter than a manual prefix check while keeping the intent obvious. Add unit tests covering normal extraction and several path-traversal attempts to lock in the behavior. Assisted-By: Cursor * osquerybeat: address review feedback on pkg traversal test Switch the cpio header helper to fmt.Fprintf(&buf, ...) to satisfy staticcheck QF1012, and add an absolute-path (/escape.txt) traversal case to round out the coverage. Assisted-By: Cursor (cherry picked from commit 5041330) Co-authored-by: Marc Guasch <marc-gr@users.noreply.github.com>
Proposed commit message
The macOS
.pkginstaller for osquery is extracted byexpandPayload()inx-pack/osquerybeat/internal/pkgutil/expand.go, which reads CPIO entries and joins each entry'sFilePathdirectly onto the destination directory. The entry name comes unsanitized from the archive, so a crafted.pkgcontaining../components could write files outside the destination directory (CWE-22). This path is only reachable on macOS, since.pkg/CPIO is the macOS osquery distribution format (Linux uses tar.gz, Windows uses msi/zip), and the siblingtarandzipextractors already validate entry paths — this brought CPIO extraction in line with them.WHAT: Reject any entry whose name is not local using
filepath.IsLocalbefore joining it with the destination directory, returning an error for traversal attempts.WHY: Prevents arbitrary file write outside the intended directory from a malicious archive.
filepath.IsLocalis the purpose-built stdlib check; it also rejects absolute paths and (on Windows) reserved names, making it stricter than a manual prefix comparison while keeping the intent self-documenting.Checklist
stresstest.shscript to run them under stress conditions and race detector to verify their stability../changelog/fragmentsusing the changelog tool.Disruptive User Impact
None. Well-formed osquery
.pkgartifacts continue to extract unchanged; only entries whose paths escape the destination directory are now rejected.How to test this PR locally
The new tests in
internal/pkgutil/expand_test.gocover normal extraction plus several path-traversal attempts (../escape.txt,../../escape-dir,a/../../escape.txt), asserting extraction fails and nothing is written outside the destination directory.Related issues
Use cases
Extraction of osquery's macOS
.pkgdistribution by osquerybeat during install/update.