DataDog
GuardDog Only Scans "setup.py" For Code Execution
https://github.com/DataDog/guarddog/blob/e49bf3298e4601ba3e4dc00140bd54d02d14371f/guarddog/analyzer/sourcecode/code-execution.yml#L1
https://github.com/DataDog/guarddog/blob/e49bf3298e4601ba3e4dc00140bd54d02d14371f/guarddog/analyzer/sourcecode/code-execution.yml#L113-L116
This causes a lot of malicious packages not to be detected because they perform code execution in other files.
It's true that reporting every single code execution would result in a lot of noise though.
We should at least make this limitation clear, because a lot of people are surprised that GuardDog does not report some malicious packages. See:
- https://github.com/DataDog/guarddog/issues/306
- https://github.com/DataDog/guarddog/issues/311
Hello! if possible I think this limitation should be removed or given a way to bypass it. Or perhaps non-setup.py hits should be shown as warnings.
To properly review a package I don't think it's enough to just look at setup.py, as evidenced by malicious packages that aren't being flagged by GuardDog.
Hello @ColdHeat , thank you reaching out. We are considering this enhancement, personally I consider than malware is more prone to running executions in the setup.py but executions by themselves (In any other part of the program) might not be as correlated to malicious behaviour thus yielding lots of false positives.
But we're analyzing the path forward.
@sobregosodd Yes setup.py is going to be an easier place to get quick RCE since it will run on package install but the exploitation path would merely require that the user then run the package which is not unreasonable given that they just installed it.
The way I see it, Guarddog needs the ability to scan across an entire package and also to have the ability to keep a seperate list of allowed "unsafe" executions.
I see a similar project being worked on that analyzes the whole package, analyzes differences between versions, and I think could possibly get to the point where findings could be allowed/rejected.
https://github.com/ancat/whiskers https://github.com/ancat/whiskers/blob/f091b36f51f080f55b63a5348b5d3a290beaccec/cmd/gem_diff_scan.go#L19-L24 https://github.com/ancat/whiskers/blob/f091b36f51f080f55b63a5348b5d3a290beaccec/cmd/gem_diff_scan.go#L109-L123
At the very least, I think guarddog should have a way via CLI or config to scan all files for rule hits instead of just specific files. Perhaps this means that the user would need to have their own rule but I do think this is something Guarddog should provide out of the box.
I did some testing to back up your noise ideas using my dataset, which includes a mix of Malregistry, Maloss, and Backstabber's Knife Collection, plus about 5000 trusted samples. I've noticed that malicious code often pops up in __init__.py files, so maybe we could consider adding this file to the allowlist.
The results show that scanning the entire code with this rule isn't really worth it because the false positive rate is extremely high. But it's tricky to make a call here because scanning __init__.py gives us 109 new false positives but also adds 284 new true positives.
What do you all think about this trade-off? Should we go ahead with scanning __init__.py despite the false positives, or stick to the current approach?
Hello @xp4u1, Thank you for the analysis. I agree that extending the rule to the entire package will yield a lot of FP, but init.py looks promising, and FPs manageable. I'll issue a PR with the changes.