Python: Add shutil module sinks for path injection query #7455
Conversation
Thanks
Can you add the tests to the bottom of https://github.com/github/codeql/blob/cbd7434a7e5d48a16f0ae5840711ef9cabae3988/python/ql/test/library-tests/frameworks/stdlib/FileSystemAccess.py instead? Would be great to see tests using both positional and keyword arguments, like the rest of the tests
Besides that, just a minor thing about one of the functions you modeled
| /** | ||
| * A call to the `shutil.copyfileobj` function. | ||
| * | ||
| * See https://docs.python.org/3/library/shutil.html#shutil.copyfileobj | ||
| */ | ||
| private class ShutilCopyfileobjCall extends FileSystemAccess::Range, DataFlow::CallCfgNode { | ||
| ShutilCopyfileobjCall() { this = shutil().getMember("copyfileobj").getACall() } | ||
|
|
||
| override DataFlow::Node getAPathArgument() { | ||
| result in [ | ||
| this.getArg(0), this.getArgByName("fsrc"), this.getArg(1), this.getArgByName("fdst") | ||
| ] | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
These arguments are not paths, but file-likes (so already opened files).
I think there is good value in keeping a note around, so we can easily remember that there is potential in modeling this function
| /** | |
| * A call to the `shutil.copyfileobj` function. | |
| * | |
| * See https://docs.python.org/3/library/shutil.html#shutil.copyfileobj | |
| */ | |
| private class ShutilCopyfileobjCall extends FileSystemAccess::Range, DataFlow::CallCfgNode { | |
| ShutilCopyfileobjCall() { this = shutil().getMember("copyfileobj").getACall() } | |
| override DataFlow::Node getAPathArgument() { | |
| result in [ | |
| this.getArg(0), this.getArgByName("fsrc"), this.getArg(1), this.getArgByName("fdst") | |
| ] | |
| } | |
| } | |
| // TODO: once we have flow summaries, model `shutil.copyfileobj` which copies the content between its' file-like arguments. | |
| // See https://docs.python.org/3/library/shutil.html#shutil.copyfileobj |
There was a problem hiding this comment.
Does ShutilCopyfileobjCall need to be deleted?
There was a problem hiding this comment.
Either that (as in my suggestion, which you can commit directly by accepting the suggestion), or by making the body of getAPathArgument() to be none()
There was a problem hiding this comment.
Updated.
There was a problem hiding this comment.
Do you think I can apply for a bounty next?
I don't quite understand the meaning of this sentence. |
Oh, that was just referring to the inline comment I made. |
Can you remove the entire file python/ql/test/query-tests/Security/CWE-022-PathInjection/shutil_path_injection.py? Since we now highlight that we model these in python/ql/test/library-tests/frameworks/stdlib/FileSystemAccess.py, that is good enough
No problem! |
Ok. I will try to apply for the bounty next. Do you think it's okay? |
I'm not officially approving/rejecting bounties. If this additional modeling finds real vulnerabilities (that you can show), I don't see why you shouldn't be able to apply for a bounty. |
The application has been submitted. |
If the path is controlled by some function calls of the
shutilmodule, it may cause malicious attacks. For example, thermtreefunction will delete the specified directory. Themovefunction will move the specified file or move the specified file to the specified location, causing the file content to leak.The text was updated successfully, but these errors were encountered: