Python: Can the data stream be passed across files?

[veno7998]

Hello,

In codeql-python, I quoted the function of file1 in file2. In this case, can the data stream be passed?

file1： cmd.py

import subprocess
from subprocess import check_output
def exec_cmd(cmd):
    return subprocess.check_output(cmd)

file2:

import flask
import subprocess
from subprocess import check_output
from flask import request
from cmd import exec_cmd
app = flask.Flask(__name__)

@app.route('/index15')
def index15():
    tmp = request.args.get('c', 'ls')
    return exec_cmd(rmp)

[RasmusWL]

Hi @P0desta, yes that should just work.

Module resolution in Python is a bit complicated, so there might be a few tricky edge cases that we don't handle this correctly, but in general we're doing pretty good on this 👍

If you're not seeing any results for these snippets of code, that might be due to the typo, fixed by:

 @app.route('/index15')
 def index15():
     tmp = request.args.get('c', 'ls')
-    return exec_cmd(rmp)
+    return exec_cmd(tmp)

[veno7998]

Hello @RasmusWL,
I think what you said is right, but I haven't worked properly yet.
test5.py

import flask
import subprocess
from subprocess import check_output
from flask import request
from cmd import exec_cmd
app = flask.Flask(__name__)

def exec_cmd2(cmd):
    return subprocess.check_output(cmd)
@app.route('/index15')
def index15():
    tmp = request.args.get('c', 'ls')
    return exec_cmd(tmp)
@app.route('/index16')
def index16():
    tmp = request.args.get('c', 'ls')
    return exec_cmd2(tmp)

app.run()

cmd.py

import subprocess
from subprocess import check_output
def exec_cmd(cmd):
    return subprocess.check_output(cmd)

query.ql

/**
 * @name Uncontrolled command line
 * @description Using externally controlled strings in a command line may allow a malicious
 *              user to change the meaning of the command.
 * @kind path-problem
 * @problem.severity error
 * @sub-severity high
 * @precision high
 * @id py/command-line-injection
 * @tags correctness
 *       security
 *       external/owasp/owasp-a1
 *       external/cwe/cwe-078
 *       external/cwe/cwe-088
 */

import python
import semmle.python.security.Paths
/* Sources */
import semmle.python.web.HttpRequest
/* Sinks */
import semmle.python.security.injection.Command

class AnyCallFlow extends DataFlowExtension::DataFlowNode {
    AnyCallFlow() {
        exists(CallNode call |
           call.getFunction().(AttrNode).getObject() = this
        )
        
    }

    override ControlFlowNode getASuccessorNode() {
        result.(CallNode).getFunction().(AttrNode).getObject() = this
    }
}
class AnyCallStringFlow extends DataFlowExtension::DataFlowNode {
   AnyCallStringFlow() { any(CallNode call).getAnArg() = this }
 
   override ControlFlowNode getASuccessorNode() { result.(CallNode).getAnArg() = this }
 }

class CommandInjectionConfiguration extends TaintTracking::Configuration {
  CommandInjectionConfiguration() { this = "Command injection configuration" }

  override predicate isSource(TaintTracking::Source source) {
    source instanceof HttpRequestTaintSource
  }

  override predicate isSink(TaintTracking::Sink sink) { sink instanceof CommandSink }

  override predicate isExtension(TaintTracking::Extension extension) {
    extension instanceof AnyCallFlow
    or 
    extension instanceof AnyCallStringFlow
  }
}

from CommandInjectionConfiguration config, TaintedPathSource src, TaintedPathSink sink
where config.hasFlowPath(src, sink)
select sink.getSink(), src, sink, "This command depends on $@.", src.getSource(),
  "a user-provided value"

In the picture, you can see that index15 is not traceable, but index16 is traced.

[veno7998]

[RasmusWL]

@P0desta just to double check, did you remember to rebuild the database after fixing up your code?

[veno7998]

@RasmusWL I have to re-establish the database, but still not cross files.

[veno7998]

It can be seen from this picture that the database is new.

[RasmusWL]

@P0desta I don't think anything in the screenshot tells me that you have run codeql database create after fixing the typo in index15...

It might have been the case that index16 also had the typo, and then it makes sense -- but I don't have any way to know anything about that 😄

[veno7998]

@RasmusWL
The new code is indeed without typos. In addition, I found that two separate files cannot cross modules, but if you write a complete flask-demo, you can cross modules;
in addition, the picture is the code after importing the database, you can see that it is There are no typos, and you can know from the number of lines of code that the path found is index16, which means that index15 is not working properly.

[RasmusWL]

Strange... let me try to reproduce

[RasmusWL]

@P0desta remember when I said Module resolution in Python is a bit complicated? 😰

In from cmd import exec_cmd we resolve cmd to be the cmd module from the standard library, and not the file right next to your code...

I'll add this as a test-case to our codebase, so we can fix it in the future. For the meantime, if you rename your cmd module to something that is not conflicting with the standard library, that should make the query work.

Thanks for your help 👍

[veno7998]

@RasmusWL
What you said is correct, but after I changed cmd.py to cmd_utils.py, I encountered another problem, that is, the data flow path is incomplete, you can see the picture below，

The complete data flow I got using flask-demo is like this,

In addition, when I use flask-demo, I also use cmd.py to name it, but it can be traced back correctly.

[veno7998]

@RasmusWL I don't know if you are reproducing this problem, change cmd.py to cmd_utils.py, check the path in vscode, can you see the completed path? This question is very strange.

[RasmusWL]

@P0desta I had to spend a bit of time looking into this, but it turns out the problem with cmd_utils is a known problem, since it doesn't belong to a Python package (I had just forgotten the details).

As an intermediate work-around while you are testing out CodeQL, you can create a dummy package and place all your files inside it. So your folder-structure becomes

mypkg/
├── cmd_utils.py
├── __init__.py
└── test5.py

and you change

-from cmd_utils import exec_cmd
+from mypkg.cmd_utils import exec_cmd

Sorry for the inconvenience.

P.S. With the change mentioned above, the query finds both command injections in its original form (without the data-flow extensions)

Query without any extensions

/**
 * @name Uncontrolled command line
 * @description Using externally controlled strings in a command line may allow a malicious
 *              user to change the meaning of the command.
 * @kind path-problem
 * @problem.severity error
 * @sub-severity high
 * @precision high
 * @id py/command-line-injection
 * @tags correctness
 *       security
 *       external/owasp/owasp-a1
 *       external/cwe/cwe-078
 *       external/cwe/cwe-088
 */

import python
import semmle.python.security.Paths
/* Sources */
import semmle.python.web.HttpRequest
/* Sinks */
import semmle.python.security.injection.Command

class CommandInjectionConfiguration extends TaintTracking::Configuration {
  CommandInjectionConfiguration() { this = "Command injection configuration" }

  override predicate isSource(TaintTracking::Source source) {
    source instanceof HttpRequestTaintSource
  }

  override predicate isSink(TaintTracking::Sink sink) { sink instanceof CommandSink }

  override predicate isExtension(TaintTracking::Extension extension) {
    extension instanceof FirstElementFlow
    or
    extension instanceof FabricExecuteExtension
  }
}

from CommandInjectionConfiguration config, TaintedPathSource src, TaintedPathSink sink
where config.hasFlowPath(src, sink)
select sink.getSink(), src, sink, "This command depends on $@.", src.getSource(),
  "a user-provided value"