Go: Decompression Bombs #13553
Go: Decompression Bombs #13553
Conversation
|
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 |
|
Hi, I've completed the work on this query and I don't have any further updates/commits here. |
…kage command line source
|
@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 |
|
@owen-mc I'm sorry for the late response. I caught a cold this whole week. |
| class TheSink extends Sink { | ||
| TheSink() { | ||
| exists(Method f | f.hasQualifiedName("github.com/DataDog/zstd", "reader", "Read") | | ||
| this = f.getACall().getReceiver() | ||
| ) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Why is this being removed? And a few methods called "WriteTo"?
There was a problem hiding this comment.
zstdDataDog.NewReader(file) returns io.ReadCloser so the Read call wasn't from the reader type of github.com/DataDog/zstd. This sink is included in another place now.
| TheSink() { | ||
| exists(Method m, DataFlow::CallNode cn | | ||
| m.hasQualifiedName("github.com/golang/snappy", "Reader", ["Read", "ReadByte"]) and | ||
| cn = m.getACall() | ||
| | | ||
| this = cn.getReceiver() and | ||
| not hasFlowToComparison(cn.getResult(0)) | ||
| ) | ||
| } |
There was a problem hiding this comment.
The docs for snappy say that these two methods just implement interface methods from the standard library, one of which you model. I think it would be better to just model those interface methods. Please check if the same is true for the other libraries. (Annoyingly it isn't very clear in Go if a method is implementing an interface method, but there are limited interface methods that make sense here so you can just compare with those function names and signatures.)
There was a problem hiding this comment.
Hi, I'm not sure if I understand this this correctly.
There was a problem hiding this comment.
do you mean something like this?
exists(Method m, DataFlow::CallNode cn |
m.implements("io", "Reader", "Read") and
cn = m.getACall()
|
result = cn.getReceiver() and
not hasFlowToComparison(cn.getResult(0))
)There was a problem hiding this comment.
please review the last commit to check if it is your suggestion.
so we can reduce many repetitive parts of query
|
hmm, the tests are done locally without error. |
There was a problem hiding this comment.
The commit "use implements predicate for io interfaces" is what I meant. Could you perhaps put in comments to make it easier to see at a glance which methods are modeled for each package? Something like "// Decoder.WriteTo and Decoder.Read already modeled" for the first deletion in that commit.
go/ql/src/experimental/frameworks/DecompressionBombsCustomizations.qll
Outdated
Show resolved
Hide resolved
remove some sinks about `"decompressor"` which was added wrongly. change `GeneralReadIoSink` type from module to class. separate `KlauspostGzipAndPgzip` `KlauspostPgzip` and `KlauspostGzip`.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| /** | ||
| * Provides decompression bomb sinks for packages that use some standard IO interfaces/methods for reading decompressed data | ||
| */ |
Check warning
Code scanning / CodeQL
Class QLDoc style. Warning
|
The integration test failure is unrelated. |
|
@owen-mc Thanks again for helping me with this PR, do you have any suggestions to improve future PRs for me? I tried my best to consider your previous suggestions in this PR as much as possible. |
|
Hi @am0o0 . I'm sorry this PR took so many rounds of review. I'm sure you've learned a lot from it. CodeQL has a very steep learning curve. It also suffers from the fact that things are done a bit differently in the libraries for the different languages that we support. (We are trying to improve that, slowly.) The only way to get better is by practice, and to some extent by reading the existing CodeQL code. There is also, of course, general software development advice:
|
These were contributed by @am0o0 in github#13553 .
These were contributed by @am0o0 in github#13553 .
These were contributed by @am0o0 in github#13553 .
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
isControlledReadpredicate 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.