Open jsoref opened 6 days ago
I'm afraid while the Go library does make an effort to 'promote' sanitizers or validation checks out of wrapper methods like Inbound
, the method at https://github.com/check-spelling-sandbox/argo-cd/blob/4014cc8b040f55dc698295d658cf0eb780ea7203/util/io/files/util.go#L83 defeats our current implementation in two ways:
return strings.hasPrefix(...)
relies on that being the unique return from the function, but we also have the return false
for the case where the input is malformed (to consider: should that be a panic, since it indicates a bug?)candidate
that gets tested but rather filepath.Join(candidate, ...)
-- we can't currently trace backwards through that and treat it as validation of candidate
.We'll take this into account for future improvements to our guard-promotion logic, but can't promise a timescale on that because it's a nontrivial effort. For the time being I'm afraid you'll have to dismiss this as a false alert.
In the interim, can the qhelp at least be improved to at least draw people's attention to these deficiencies? https://github.com/github/codeql/blob/590e93d8edec4d7216935ed4425a7ab77b3b2f34/go/ql/src/Security/CWE-022/ZipSlip.qhelp#L33-L41
I understand that improved algorithms take time, but if I can get some documentation improved to save me from similar reports in the interim I would definitely take/appreciate that.
I note this isn't zipslip-specific -- there are always a lot of ways to sanitize something, and all our queries do their best to identify when you have made an appropriate check, but will sometimes fail to identify the check and so raise an alert regardless. So I will share the feedback, but probably won't make a zipslip-specific note for this.
https://github.com/github/codeql/blob/590e93d8edec4d7216935ed4425a7ab77b3b2f34/go/ql/src/Security/CWE-022/ZipSlip.ql#L22-L23
Here's my fork's report: https://github.com/check-spelling-sandbox/argo-cd/security/code-scanning/4
Arbitrary file access during archive extraction ("Zip Slip")
Code snippet util/io/files/tar.go:75
Here's the accused flow:
Arbitrary file access during archive extraction ("Zip Slip") Step 1 ... := ...[0] Source util/io/files/tar.go:75
Step 3 call to Join util/io/files/tar.go:86
Step 4 target Sink util/io/files/tar.go:98