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

Python: Add support for threat models #17203

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -427,7 +427,7 @@ Kinds
Source kinds
~~~~~~~~~~~~

- **remote**: A generic source of remote flow. Most taint-tracking queries will use such a source. Currently this is the only supported source kind.
See documentation below for :ref:`Threat models <threat-models-python>`.

Sink kinds
~~~~~~~~~~
Expand All @@ -449,3 +449,10 @@ Summary kinds

- **taint**: A summary that propagates taint. This means the output is not necessarily equal to the input, but it was derived from the input in an unrestrictive way. An attacker who controls the input will have significant control over the output as well.
- **value**: A summary that preserves the value of the input or creates a copy of the input such that all of its object properties are preserved.

.. _threat-models-python:

Threat models
-------------

.. include:: ../reusables/threat-model-description.rst
2 changes: 1 addition & 1 deletion docs/codeql/reusables/beta-note-threat-models.rst
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,4 @@

Note

Threat models are currently in beta and subject to change. During the beta, threat models are supported only by Java and C# analysis.
Threat models are currently in beta and subject to change. During the beta, threat models are supported only by Java, C# and Python analysis.
4 changes: 4 additions & 0 deletions python/ql/lib/change-notes/2024-08-16-threat-models.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: feature
---
* Added support for custom threat-models, which can be used in most of our taint-tracking queries, see our [documentation](https://docs.github.com/en/code-security/code-scanning/creating-an-advanced-setup-for-code-scanning/customizing-your-advanced-setup-for-code-scanning#extending-codeql-coverage-with-threat-models) for more details.
8 changes: 8 additions & 0 deletions python/ql/lib/ext/default-threat-models-fixup.model.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
extensions:
- addsTo:
pack: codeql/threat-models
extensible: threatModelConfiguration
data:
# Since responses are enabled by default in the shared threat-models configuration,
# we need to disable it here to keep existing behavior for the python analysis.
- ["response", false, -2147483647]
2 changes: 2 additions & 0 deletions python/ql/lib/qlpack.yml
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,12 @@ dependencies:
codeql/dataflow: ${workspace}
codeql/mad: ${workspace}
codeql/regex: ${workspace}
codeql/threat-models: ${workspace}
codeql/tutorial: ${workspace}
codeql/util: ${workspace}
codeql/xml: ${workspace}
codeql/yaml: ${workspace}
dataExtensions:
- semmle/python/frameworks/**/*.model.yml
- ext/*.model.yml
warnOnImplicitThis: true
56 changes: 56 additions & 0 deletions python/ql/lib/semmle/python/Concepts.qll
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,62 @@ private import semmle.python.dataflow.new.RemoteFlowSources
private import semmle.python.dataflow.new.TaintTracking
private import semmle.python.Frameworks
private import semmle.python.security.internal.EncryptionKeySizes
private import codeql.threatmodels.ThreatModels

/**
* A data flow source, for a specific threat-model.
*
* Extend this class to refine existing API models. If you want to model new APIs,
* extend `ThreatModelSource::Range` instead.
*/
class ThreatModelSource extends DataFlow::Node instanceof ThreatModelSource::Range {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might I suggest the name FlowSource for this class? It seems consistent with C++ and Swift at least, and it works nicely with RemoteFlowSource being a special case of it.

Then we could rename ActiveThreatModelSource to ThreatModelFlowSource to be consistent with other languages. I do agree that the "active" prefix makes sense, but given that this will be the new go-to thing to use in isSource() predicates it seems that we really do want consistency for that class name.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've persuaded @michaelnebel to be in favor of ActiveThreatModelSource, so I've just filed a PR to make the existing languages use that as well 👍 #17424

FlowSource

Regarding renaming to FlowSource, I've tried to do that here: RasmusWL@dec5daa

I'm slightly hesitant to accept it, I can't quite put my finger on it, but I think it's because it's so generic that name could be used to capture the set of sources for any data-flow/taint-tracking configuration, no matter the logic, whereas ThreatModelSource seem to convey a more specific meaning to me.

I realize that your suggestion probably fits pretty well with current naming in C#/Java/Go with SourceNode and C++/Swift with FlowSource 🤔 Maybe I'll see if anyone comes with a convincing argument during next round of review, otherwise it looks like I should just disagree-and-commit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whatever we do we ought to treat the naming of the two classes as a single decision; not something where we try to make a decision for each class in isolation.

The proposed rename to FlowSource made sense when the other class would be called ThreatModelFlowSource; but it doesn't work so so nicely with ActiveThreatModelSource. I'd prefer the combination ThreatModelSource/ActiveThreatModelSource over FlowSource/ActiveThreatModelSource.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I'm feeling strongly in favor of ActiveThreatModelSource, so it seems like I won't add the FlowSource commit 👍

/**
* Gets a string that represents the source kind with respect to threat modeling.
*
* See
* - https://github.com/github/codeql/blob/main/docs/codeql/reusables/threat-model-description.rst
* - https://github.com/github/codeql/blob/main/shared/threat-models/ext/threat-model-grouping.model.yml
*/
string getThreatModel() { result = super.getThreatModel() }

/** Gets a string that describes the type of this threat-model source. */
string getSourceType() { result = super.getSourceType() }
}

/** Provides a class for modeling new sources for specific threat-models. */
module ThreatModelSource {
/**
* A data flow source, for a specific threat-model.
*
* Extend this class to model new APIs. If you want to refine existing API models,
* extend `ThreatModelSource` instead.
*/
abstract class Range extends DataFlow::Node {
/**
* Gets a string that represents the source kind with respect to threat modeling.
*
* See
* - https://github.com/github/codeql/blob/main/docs/codeql/reusables/threat-model-description.rst
* - https://github.com/github/codeql/blob/main/shared/threat-models/ext/threat-model-grouping.model.yml
*/
abstract string getThreatModel();

/** Gets a string that describes the type of this threat-model source. */
abstract string getSourceType();
}
}

/**
* A data flow source that is enabled in the current threat model configuration.
*/
class ActiveThreatModelSource extends DataFlow::Node {
ActiveThreatModelSource() {
exists(string kind |
currentThreatModel(kind) and
this.(ThreatModelSource).getThreatModel() = kind
Comment on lines +61 to +65
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Silly question, but why doesn't this extend ThreatModelSource? After all, it only holds for instances of that class anyway.

)
}
}

/**
* A data-flow node that executes an operating system command,
Expand Down
10 changes: 3 additions & 7 deletions python/ql/lib/semmle/python/dataflow/new/RemoteFlowSources.qll
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,7 @@ private import semmle.python.Concepts
* Extend this class to refine existing API models. If you want to model new APIs,
* extend `RemoteFlowSource::Range` instead.
*/
class RemoteFlowSource extends DataFlow::Node instanceof RemoteFlowSource::Range {
/** Gets a string that describes the type of this remote flow source. */
string getSourceType() { result = super.getSourceType() }
}
class RemoteFlowSource extends ThreatModelSource instanceof RemoteFlowSource::Range { }

/** Provides a class for modeling new sources of remote user input. */
module RemoteFlowSource {
Expand All @@ -28,8 +25,7 @@ module RemoteFlowSource {
* Extend this class to model new APIs. If you want to refine existing API models,
* extend `RemoteFlowSource` instead.
*/
abstract class Range extends DataFlow::Node {
/** Gets a string that describes the type of this remote flow source. */
abstract string getSourceType();
abstract class Range extends ThreatModelSource::Range {
override string getThreatModel() { result = "remote" }
}
}
18 changes: 18 additions & 0 deletions python/ql/lib/semmle/python/frameworks/PEP249.qll
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,24 @@ module PEP249 {
}
}

/** A call to a method that fetches rows from a previous execution. */
private class FetchMethodCall extends ThreatModelSource::Range, API::CallNode {
FetchMethodCall() {
exists(API::Node start |
start instanceof DatabaseCursor or start instanceof DatabaseConnection
|
// note: since we can't currently provide accesspaths for sources, these are all
// lumped together, although clearly the fetchmany/fetchall returns a
// list/iterable with rows.
this = start.getMember(["fetchone", "fetchmany", "fetchall"]).getACall()
)
}

override string getThreatModel() { result = "database" }

override string getSourceType() { result = "cursor.fetch*()" }
}

// ---------------------------------------------------------------------------
// asyncio implementations
// ---------------------------------------------------------------------------
Expand Down
29 changes: 29 additions & 0 deletions python/ql/lib/semmle/python/frameworks/Stdlib.model.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
extensions:
- addsTo:
pack: codeql/python-all
extensible: sourceModel
data:
- ['os', 'Member[getenv].ReturnValue', 'environment']
- ['os', 'Member[getenvb].ReturnValue', 'environment']
- ['os', 'Member[environ]', 'environment']
- ['os', 'Member[environb]', 'environment']
- ['posix', 'Member[environ]', 'environment']

- ['sys', 'Member[argv]', 'commandargs']
- ['sys', 'Member[orig_argv]', 'commandargs']

- ['sys', 'Member[stdin]', 'stdin']
- ['builtins', 'Member[input].ReturnValue', 'stdin']
- ['builtins', 'Member[raw_input].ReturnValue', 'stdin'] # python 2 only


# if no argument is given, the default is to use sys.argv[1:]
- ['argparse.ArgumentParser', 'Member[parse_args,parse_known_args].WithArity[0].ReturnValue', 'commandargs']

- ['os', 'Member[read].ReturnValue', 'file']
- addsTo:
pack: codeql/python-all
extensible: summaryModel
data:
- ['argparse.ArgumentParser', 'Member[parse_args,parse_known_args]', 'Argument[0,args:]', 'ReturnValue', 'taint']
# note: taint of attribute lookups is handled in QL
50 changes: 46 additions & 4 deletions python/ql/lib/semmle/python/frameworks/Stdlib.qll
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,7 @@ module StdlibPrivate {
* Modeling of path related functions in the `os` module.
* Wrapped in QL module to make it easy to fold/unfold.
*/
private module OsFileSystemAccessModeling {
module OsFileSystemAccessModeling {
/**
* A call to the `os.fsencode` function.
*
Expand Down Expand Up @@ -395,7 +395,7 @@ module StdlibPrivate {
*
* See https://docs.python.org/3/library/os.html#os.open
*/
private class OsOpenCall extends FileSystemAccess::Range, DataFlow::CallCfgNode {
class OsOpenCall extends FileSystemAccess::Range, DataFlow::CallCfgNode {
OsOpenCall() { this = os().getMember("open").getACall() }

override DataFlow::Node getAPathArgument() {
Expand Down Expand Up @@ -1499,13 +1499,22 @@ module StdlibPrivate {
* See https://docs.python.org/3/library/functions.html#open
*/
private class OpenCall extends FileSystemAccess::Range, Stdlib::FileLikeObject::InstanceSource,
DataFlow::CallCfgNode
ThreatModelSource::Range, DataFlow::CallCfgNode
{
OpenCall() { this = getOpenFunctionRef().getACall() }
OpenCall() {
this = getOpenFunctionRef().getACall() and
// when analyzing stdlib code for os.py we wrongly assume that `os.open` is an
// alias of the builtins `open` function
not this instanceof OsFileSystemAccessModeling::OsOpenCall
}

override DataFlow::Node getAPathArgument() {
result in [this.getArg(0), this.getArgByName("file")]
}

override string getThreatModel() { result = "file" }

override string getSourceType() { result = "open()" }
}

/**
Expand Down Expand Up @@ -4989,6 +4998,39 @@ module StdlibPrivate {

override string getKind() { result = Escaping::getHtmlKind() }
}

// ---------------------------------------------------------------------------
// argparse
// ---------------------------------------------------------------------------
/**
* if result of `parse_args` is tainted (because it uses command-line arguments),
* then the parsed values accesssed on any attribute lookup is also tainted.
*/
private class ArgumentParserAnyAttributeStep extends TaintTracking::AdditionalTaintStep {
override predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
nodeFrom =
API::moduleImport("argparse")
.getMember("ArgumentParser")
.getReturn()
.getMember("parse_args")
.getReturn()
.getAValueReachableFromSource() and
nodeTo.(DataFlow::AttrRead).getObject() = nodeFrom
}
}

// ---------------------------------------------------------------------------
// sys
// ---------------------------------------------------------------------------
/**
* An access of `sys.stdin`/`sys.stdout`/`sys.stderr`, to get additional FileLike
* modeling.
*/
private class SysStandardStreams extends Stdlib::FileLikeObject::InstanceSource, DataFlow::Node {
SysStandardStreams() {
this = API::moduleImport("sys").getMember(["stdin", "stdout", "stderr"]).asSource()
}
}
}

// ---------------------------------------------------------------------------
Expand Down
13 changes: 9 additions & 4 deletions python/ql/lib/semmle/python/frameworks/data/ModelsAsData.qll
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,19 @@ private import semmle.python.dataflow.new.RemoteFlowSources
private import semmle.python.dataflow.new.DataFlow
private import semmle.python.ApiGraphs
private import semmle.python.dataflow.new.FlowSummary
private import semmle.python.Concepts

/**
* A remote flow source originating from a CSV source row.
* A threat-model flow source originating from a data extension.
*/
private class RemoteFlowSourceFromCsv extends RemoteFlowSource::Range {
RemoteFlowSourceFromCsv() { this = ModelOutput::getASourceNode("remote").asSource() }
private class ThreatModelSourceFromDataExtension extends ThreatModelSource::Range {
ThreatModelSourceFromDataExtension() { this = ModelOutput::getASourceNode(_).asSource() }

override string getSourceType() { result = "Remote flow (from model)" }
override string getThreatModel() { this = ModelOutput::getASourceNode(result).asSource() }

override string getSourceType() {
result = "Source node (" + this.getThreatModel() + ") [from data-extension]"
}
}

private class SummarizedCallableFromModel extends SummarizedCallable {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,14 @@ module CodeInjection {
abstract class Sanitizer extends DataFlow::Node { }

/**
* A source of remote user input, considered as a flow source.
* DEPRECATED: Use `ActiveThreatModelSource` from Concepts instead!
*/
class RemoteFlowSourceAsSource extends Source, RemoteFlowSource { }
deprecated class RemoteFlowSourceAsSource = ActiveThreatModelSourceAsSource;

/**
* An active threat-model source, considered as a flow source.
*/
private class ActiveThreatModelSourceAsSource extends Source, ActiveThreatModelSource { }

/**
* A code execution, considered as a flow sink.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,14 @@ module CommandInjection {
abstract class Sanitizer extends DataFlow::Node { }

/**
* A source of remote user input, considered as a flow source.
* DEPRECATED: Use `ActiveThreatModelSource` from Concepts instead!
*/
class RemoteFlowSourceAsSource extends Source, RemoteFlowSource { }
deprecated class RemoteFlowSourceAsSource = ActiveThreatModelSourceAsSource;

/**
* An active threat-model source, considered as a flow source.
*/
private class ActiveThreatModelSourceAsSource extends Source, ActiveThreatModelSource { }

/**
* A command execution, considered as a flow sink.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,14 @@ module CookieInjection {
abstract class Sanitizer extends DataFlow::Node { }

/**
* A source of remote user input, considered as a flow source.
* DEPRECATED: Use `ActiveThreatModelSource` from Concepts instead!
*/
class RemoteFlowSourceAsSource extends Source, RemoteFlowSource { }
deprecated class RemoteFlowSourceAsSource = ActiveThreatModelSourceAsSource;

/**
* An active threat-model source, considered as a flow source.
*/
private class ActiveThreatModelSourceAsSource extends Source, ActiveThreatModelSource { }

/**
* A write to a cookie, considered as a sink.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,14 @@ module HttpHeaderInjection {
abstract class Sanitizer extends DataFlow::Node { }

/**
* A source of remote user input, considered as a flow source.
* DEPRECATED: Use `ActiveThreatModelSource` from Concepts instead!
*/
class RemoteFlowSourceAsSource extends Source, RemoteFlowSource { }
deprecated class RemoteFlowSourceAsSource = ActiveThreatModelSourceAsSource;

/**
* An active threat-model source, considered as a flow source.
*/
private class ActiveThreatModelSourceAsSource extends Source, ActiveThreatModelSource { }

/**
* A HTTP header write, considered as a flow sink.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,14 @@ module LdapInjection {
abstract class FilterSanitizer extends DataFlow::Node { }

/**
* A source of remote user input, considered as a flow source.
* DEPRECATED: Use `ActiveThreatModelSource` from Concepts instead!
*/
class RemoteFlowSourceAsSource extends Source, RemoteFlowSource { }
deprecated class RemoteFlowSourceAsSource = ActiveThreatModelSourceAsSource;

/**
* An active threat-model source, considered as a flow source.
*/
private class ActiveThreatModelSourceAsSource extends Source, ActiveThreatModelSource { }

/**
* A logging operation, considered as a flow sink.
Expand Down
Loading
Loading