Python : Add Flask sinks for path injection query #6330

ghost · 2021-07-19T20:46:27Z

Flask's send_file and send_from_directory calls can allow a remote attacker to potentially access any file stored on the system if the path is controlled by them.

This query modifies the existing Python path injection query to add support for the same.

ghost · 2021-07-19T21:03:42Z

Just a small note : https://github.com/cksgf/ServerManagement/blob/49491cc6f94980e6be7791d17be947c27071eb56/route/file.py#L93 should be a true positive for this case but since add a flow to track that may add a lot of FP's, I am in two minds here. @RasmusWL Any thoughts on this?

RasmusWL · 2021-07-22T13:12:46Z

I think having additional taint steps for os.path.dirname, os.path.split and so on sound like a good improvement for path injection.

We've added these things like this in the past:

codeql/python/ql/src/semmle/python/frameworks/Stdlib.qll

Lines 44 to 52 in d282f6a

    
           /** An additional taint step for calls to `os.path.normpath` */ 
        
           private class OsPathNormpathCallAdditionalTaintStep extends TaintTracking::AdditionalTaintStep { 
        
             override predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) { 
        
               exists(OsPathNormpathCall call | 
        
                 nodeTo = call and 
        
                 nodeFrom = call.getPathArg() 
        
               ) 
        
             } 
        
           }

so please follow that style as well 😊

Would also be great if you can implement the new sink by using FileSystemAccess::Range, like in

codeql/python/ql/src/semmle/python/frameworks/Stdlib.qll

Lines 394 to 404 in d282f6a

    
             /** 
        
              * A call to the builtin `open` function. 
        
              * See https://docs.python.org/3/library/functions.html#open 
        
              */ 
        
             private class OpenCall extends FileSystemAccess::Range, DataFlow::CallCfgNode { 
        
               OpenCall() { this = getOpenFunctionRef().getACall() } 
        
               override DataFlow::Node getAPathArgument() { 
        
                 result in [this.getArg(0), this.getArgByName("file")] 
        
               } 
        
             }

RasmusWL

Since the creation of this PR, we have added support for os.path.dirname (see below), so please merge in newest main, and remove that part.

codeql/python/ql/lib/semmle/python/frameworks/Stdlib.qll

Lines 283 to 291 in 8b6baa2

    
           /** An additional taint step for path computations. */ 
        
           private class OsPathComputationAdditionalTaintStep extends TaintTracking::AdditionalTaintStep { 
        
             override predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) { 
        
               exists(OsPathComputation call | 
        
                 nodeTo = call and 
        
                 nodeFrom = call.getPathArg() 
        
               ) 
        
             } 
        
           }

If you can add your new modeling to this file instead https://github.com/github/codeql/blob/main/python/ql/lib/semmle/python/frameworks/Flask.qll (and adjust QLDoc accordingly), I would be happy to merge this PR 👍 I'm not sure how I did not spot this on my last review, sorry about that 🤷‍♂️

RasmusWL · 2021-10-25T09:55:07Z

Having a test that shows off the new modeling would also be very good, and should be added somewhere within this folder: https://github.com/github/codeql/tree/main/python/ql/test/library-tests/frameworks/flask

it should use the format expected by our ConceptsTest, so something like the one below, highlight all the interesting different variations that we handle (that is, using both normal arguments and keyword arguments)

flask.send_from_directory("filepath")  # $ getAPathArgument="filepath"

ghost · 2021-10-27T20:42:48Z

@RasmusWL I need some help fixing the tests here. the tests fail with the output I attach below. The arguments are detected by CodeQL on real projects otherwise. So, this looks like an issue with the test runner.

--- expected
+++ actual
@@ -1,1 +1,8 @@
-
+| save_uploaded_file.py:12:51:12:105 | Comment # $ getAPathArgument="filepath" getAPathArgument="file" | Missing result:getAPathArgument="file" |
+| save_uploaded_file.py:12:51:12:105 | Comment # $ getAPathArgument="filepath" getAPathArgument="file" | Missing result:getAPathArgument="filepath" |
+| save_uploaded_file.py:13:30:13:56 | Comment # $ getAPathArgument="file" | Missing result:getAPathArgument="file" |
+| save_uploaded_file.py:15:60:15:114 | Comment # $ getAPathArgument="filepath" getAPathArgument="file" | Missing result:getAPathArgument="file" |
+| save_uploaded_file.py:15:60:15:114 | Comment # $ getAPathArgument="filepath" getAPathArgument="file" | Missing result:getAPathArgument="filepath" |
+| save_uploaded_file.py:16:59:16:113 | Comment # $ getAPathArgument="filepath" getAPathArgument="file" | Missing result:getAPathArgument="file" |
+| save_uploaded_file.py:16:59:16:113 | Comment # $ getAPathArgument="filepath" getAPathArgument="file" | Missing result:getAPathArgument="filepath" |
+| save_uploaded_file.py:17:45:17:71 | Comment # $ getAPathArgument="file" | Missing result:getAPathArgument="file" |

I don't have a clue what's wrong here. Can you please take a look?

and auto-formatting

RasmusWL · 2021-10-28T09:16:48Z

@RasmusWL I need some help fixing the tests here. the tests fail with the output I attach below. The arguments are detected by CodeQL on real projects otherwise. So, this looks like an issue with the test runner.

it was a simple mistake, I fixed it in 358663f

RasmusWL

LGTM now, thanks for updating it 👍

ghost self-requested a review as a code owner July 19, 2021 20:46

github-actions bot added the Python label Jul 19, 2021

ghost mentioned this pull request Jul 19, 2021

[porcupiney.hairs]: [Python] Add Flask Path injection sinks github/securitylab#407

Closed

ghost force-pushed the pyPathTraversal branch 2 times, most recently from a476229 to c1c1d89 Compare October 21, 2021 22:40

RasmusWL requested changes Oct 25, 2021

View reviewed changes

Python : Add Flask sinks for path injection query

4fd3f21

ghost force-pushed the pyPathTraversal branch from c1c1d89 to 4fd3f21 Compare October 27, 2021 20:42

RasmusWL added 2 commits October 28, 2021 11:14

Python: Fix tests

358663f

Python: Minor fix to QLDoc

d9e5d17

and auto-formatting

RasmusWL approved these changes Oct 28, 2021

View reviewed changes

RasmusWL merged commit 6d09334 into github:main Oct 28, 2021

RasmusWL self-assigned this Oct 28, 2021

ghost deleted the pyPathTraversal branch October 28, 2021 10:45

RasmusWL mentioned this pull request Oct 28, 2021

Python: Small fixup for flask.send_from_directory #6989

Merged

ghost mentioned this pull request Nov 15, 2021

Python : Add Flask Path Injection Sinks github/securitylab#480

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Python : Add Flask sinks for path injection query #6330

Python : Add Flask sinks for path injection query #6330

ghost commented Jul 19, 2021

ghost commented Jul 19, 2021

RasmusWL commented Jul 22, 2021

RasmusWL left a comment

RasmusWL commented Oct 25, 2021

ghost commented Oct 27, 2021

RasmusWL commented Oct 28, 2021

RasmusWL left a comment

	/** An additional taint step for path computations. */
	private class OsPathComputationAdditionalTaintStep extends TaintTracking::AdditionalTaintStep {
	override predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
	exists(OsPathComputation call \|
	nodeTo = call and
	nodeFrom = call.getPathArg()
	)
	}
	}

Python : Add Flask sinks for path injection query #6330

Python : Add Flask sinks for path injection query #6330

Conversation

ghost commented Jul 19, 2021

ghost commented Jul 19, 2021

RasmusWL commented Jul 22, 2021

RasmusWL left a comment

Choose a reason for hiding this comment

RasmusWL commented Oct 25, 2021

ghost commented Oct 27, 2021

RasmusWL commented Oct 28, 2021

RasmusWL left a comment

Choose a reason for hiding this comment