The Wayback Machine - https://web.archive.org/web/20211101022422/https://github.com/github/codeql/pull/7009
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Python : Add sanitizers for Path Injection Query #7009

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Python : Add sanitizers for Path Injection Query #7009

wants to merge 2 commits into from
+19 −0

Conversation

Labels
Python
2 participants
@porcupineyhairs @intrigus-lgtm
@porcupineyhairs
Copy link
Contributor

@porcupineyhairs porcupineyhairs commented Oct 29, 2021

This PR adds a few sanitizers to the exisitng Path injection query.
@porcupineyhairs
Python : Add sanitizers for Path Injection Query
cd61294 
This PR adds a few sanitizers to the exisitng Path injection query.
@porcupineyhairs porcupineyhairs requested a review from as a code owner Oct 29, 2021
@porcupineyhairs porcupineyhairs mentioned this pull request Oct 29, 2021
Open
intrigus-lgtm
intrigus-lgtm reviewed Oct 31, 2021
View changes
python/ql/lib/semmle/python/security/dataflow/PathInjectionCustomizations.qll
*/
private class OsPathAbsoluteCallAsSanitizer extends Sanitizer {
OsPathAbsoluteCallAsSanitizer() {
this = API::moduleImport("os").getMember("path").getMember("abspath").getACall().getArg(_)
Copy link
Contributor

@intrigus-lgtm intrigus-lgtm Oct 31, 2021

I might be wrong, but isn't this saying that the argument to abspath has been sanitized?
Don't we instead want to say that the result of such a call has been sanitized?

So in this example:

sanitizedPath = os.path.abspath(taintedPath)
dangerousUse(taintedPath)

taintedPath could not flow to dangerousUse because it is used once as an argument to abspath and therefore considered sanitized (which is not correct).

If I'm right you could add new tests that show the improvements of this PR.
Copy link
Contributor Author

@porcupineyhairs porcupineyhairs Nov 1, 2021

I considered it when writing this PR. Yes, what you are saying is true. But consider the case here. https://github.com/kizniche/Mycodo/blob/6c0d710c8138aa06e6dcd909097854afe0d5cebe/mycodo/mycodo_flask/routes_general.py#L120-L121

In this case, since file_path is checked earlier, the code is not exploitable. So I thought it would be best to avoid multiple FPs and accept a reduced TP count instead.
@porcupineyhairs
Copy link
Contributor Author

@porcupineyhairs porcupineyhairs commented Nov 1, 2021

@RasmusWL The query currently flags some sinks which shouldn't otherwise be there. For ex, a simple run of the query with the proposed changes included leads to 3 results. Of which 2 of them are calls to os.path.getctime and os.path.exists. I think these can be easily avoided if we were to use code from the StdLibPrivate module. This would reduce code duplication and let us reduce FP's. Should I mark it public?
@porcupineyhairs
include sanitizers for all cnfigurations
51ca480
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment