codeql
codeql copied to clipboard
github
Go: Decompression Bombs
This is part of All for one, one for all query submission, I'm going to submit an issue in github/securitylab for this pull request too.
I tried my best to cover Command line Flow sources in Golang too. these can be used in other queries as Flow sources too.
An unsolved problem still exists about the isControlledRead predicate which is act as a sanitizer. I wrote the sanitizer with the same method in C++ usage and sanitizer predicate but in Java and Go I couldn't do it and it is really weird because I test it with another global taint config and I didn't get the proper flow to my desired sink. I put comments I hope it is enough I'M ready to explain more.
Anyone not watching the repo in general, note this is part of a family of submissions:
https://github.com/github/codeql/pull/13553 https://github.com/github/codeql/pull/13554 https://github.com/github/codeql/pull/13555 https://github.com/github/codeql/pull/13556 https://github.com/github/codeql/pull/13557 https://github.com/github/codeql/pull/13558
Hi @smowton sorry about delay of writing proper comments, I thought it is weekend and I had enough time for writing details until Monday.
No problem, just getting out in front of anyone happens to be reviewing it right away -- you're probably right that they'll all wait til Monday
Anyone not watching the repo in general, note this is part of a family of submissions:
#13553 #13554 #13555 #13556 #13557 #13558
#13560 is added too
Hi, I've completed the work on this query and I don't have any further updates/commits here.
@amammad please remove Parameter as a source for the dataflow. Even after removing the Parameter source, I am still getting 10 alerts that are all the same path. Putting this for CodeQL go team cause I'm not quite sure why that is happening. Ex: https://gist.github.com/Kwstubbs/382d245e8e3ca9f424223aed75e67126
@Kwstubbs I think this is related to global additional steps which a method can have two nodeTo maybe! also I updated the sources for getting more results and I updated the messages which now the user controlled is replaced by uncontrolled decompression.
QHelp previews:
go/ql/src/experimental/CWE-522-DecompressionBombs/DecompressionBombs.qhelp
Uncontrolled file decompression
Extracting Compressed files with any compression algorithm like gzip can cause to denial of service attacks.
Attackers can compress a huge file which created by repeated similiar byte and convert it to a small compressed file.
Recommendation
When you want to decompress a user-provided compressed file you must be careful about the decompression ratio or read these files within a loop byte by byte to be able to manage the decompressed size in each cycle of the loop. Also you can limit the size of reader buffer.
Example
Using "io.LimitReader" and "io.CopyN" are the best option to prevent decompression bomb attacks.
package main
import (
"archive/zip"
"fmt"
"io"
"os"
)
func ZipOpenReader(filename string) {
// Open the zip file
r, _ := zip.OpenReader(filename)
var totalBytes int64
for _, f := range r.File {
rc, _ := f.Open()
totalBytes = 0
for {
result, _ := io.CopyN(os.Stdout, rc, 68)
if result == 0 {
break
}
totalBytes = totalBytes + result
if totalBytes > 1024*1024 {
fmt.Print(totalBytes)
_ = rc.Close()
break
}
}
}
}
package main
import (
"compress/gzip"
"io"
"os"
)
func safeReader() {
var src io.Reader
src, _ = os.Open("filename")
gzipR, _ := gzip.NewReader(src)
dstF, _ := os.OpenFile("./test", os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0755)
defer dstF.Close()
var newSrc io.Reader
newSrc = io.LimitReader(gzipR, 1024*1024*1024*5)
_, _ = io.Copy(dstF, newSrc)
}
References
- CVE-2023-26483
- A great research to gain more impact by this kind of attacks
- Common Weakness Enumeration: CWE-409.
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
The tests needs stubs for the dependencies, which you can make using depstubber. Please put the go generate commands as comments in the test file.
@owen-mc please wait to start the review process, I know many things from reviews of previous PRs of mine and I'm going to check those here too.
@owen-mc please wait to start the review process, I know many things from reviews of previous PRs of mine and I'm going to check those here too.
@owen-mc It's ready for review now.
@owen-mc could you please check why this query can not give me path-problem results? I tried to make a module with an extension point for additional steps and sinks.
@amammad when I run your query, I get path-problem results. Do you have an example of what you are expecting to see in the results, but aren't getting?
@mbg I'm really confused sometimes, I recreated the codeql DB and it is OK now!
Two errors in your test:
| test.go:122:31:122:31 | rKlauspost.File undefined (type *"github.com/klauspost/compress/zip".ReadCloser has no field or method File) |
| test.go:226:14:226:14 | cannot use snappyklauspost (variable of type interface{}) as *s2.Reader value in assignment: need type assertion |
Need to update expected results for DecompressionBombs.qlref
@owen-mc it seems that tests are OK in local I ran following command codeql test run --learn DecompressionBombs.qlref
Are the files in go/vendor that you add in this PR meant to be there? I'd expect stubs to only be in test folders. They are now causing conflicts because this PR was merged.
If the tests are giving different results locally than in CI then I would suggest (1) making sure your codeql CLI is up to date, (2) making sure that you have merged in/rebased on current main and (3) making sure that your version of Go is up to date.
Did you see my question about
go/vendor?
@owen-mc Sorry, I should rest a little bit, I removed them now.
+| vendor/github.com/klauspost/compress/snappy/stub.go:23:7:23:7 | cannot define new methods on non-local type s2.Reader | +| vendor/github.com/klauspost/compress/snappy/stub.go:27:7:27:7 | cannot define new methods on non-local type s2.Reader | +| vendor/github.com/klauspost/compress/snappy/stub.go:31:7:31:7 | cannot define new methods on non-local type s2.Reader | +| vendor/github.com/klauspost/compress/snappy/stub.go:35:7:35:7 | cannot define new methods on non-local type s2.Reader | +| vendor/github.com/klauspost/compress/snappy/stub.go:39:7:39:7 | cannot define new methods on non-local type s2.Reader | +| vendor/github.com/klauspost/compress/snappy/stub.go:41:7:41:7 | cannot define new methods on non-local type s2.Reader | +| vendor/github.com/klauspost/compress/snappy/stub.go:45:7:45:7 | cannot define new methods on non-local type s2.Reader |
The methods defined in snappy/stub.go need to be defined on s2.Reader (if they aren't already).
@owen-mc Good news!
I've added inline tests and I found a True negative result from the sanitizer. you can see this in inline tests.
Your last commit is very difficult to review, because so much code is moved around that it's hard to see what has changed. Would it be possible to split it into two commits, a first one which purely moves code around and a second one which makes functional changes?
@owen-mc IDK how to do this, but It can be a good experience for me to do this, let me think about how to split the last commit into two neat commits! I'm sorry about the bad commit sequences!
My recommendation is to checkout the commit before the last commit, make a new branch there, and do all the code moving you wanted to do. It's okay to do minor edits to make things compile, but no changes in functionality. Make a commit of that, with a message which makes it clear there are no functional changes. Then switch back to your original branch and rebase on the new branch. For any merge conflicts, accept incoming changes. Hopefully that will work without too much hassle. It will give you some experience editing git history, but life is much easier if you make the big code move in a separate commit in the first place. If you find it too hard then let me know and I will try to work around the problem.
@owen-mc I'm sorry for the late response. I caught a cold this whole week. I tried my best to have a good rebase. It was a good experience :)