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

Merged
merged 23 commits into from
Sep 26, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
5ec8e5d
Python: Setup support for threat-models
RasmusWL Aug 9, 2024
766dcc4
ThreatModels: Expose `knownThreatModel`
RasmusWL Aug 9, 2024
617ab27
Python: Add test showing default active threat-models
RasmusWL Aug 9, 2024
8f7dec0
Python: Remove 'response' from default threat-models
RasmusWL Aug 9, 2024
528f08f
Python: Make queries use ActiveThreatModelSource
RasmusWL Aug 9, 2024
b9239d7
Python: Add basic support for environment/commandargs threat-models
RasmusWL Aug 9, 2024
56c85ff
Python: Fixup threat-models for `os.environ.get()`
RasmusWL Aug 9, 2024
e1801f3
Python: Proper threat-model handling for argparse
RasmusWL Aug 9, 2024
66f389a
Python: Model stdin thread-model
RasmusWL Aug 9, 2024
d245db5
Python: Model `file` threat-model
RasmusWL Aug 12, 2024
7483075
Python: Fixup modeling of `os.open`
RasmusWL Aug 13, 2024
8d8cd05
Python: Add basic support for `database` threat-model
RasmusWL Aug 16, 2024
a0b24d6
Python: Add e2e threat-model test
RasmusWL Aug 16, 2024
0ccb5b1
Python: Add change-note
RasmusWL Aug 16, 2024
7d3793e
Docs: Update threat-model list to include Python
RasmusWL Aug 19, 2024
333367c
Python: Add threat-modeling of `raw_input`
RasmusWL Aug 16, 2024
cbebf7b
Python: Additional threatModelSource annotations
RasmusWL Aug 16, 2024
5ff7b65
Python: Add links to threat-model docs
RasmusWL Aug 30, 2024
e35c2b2
Docs: Include 'Threat models' for Python
RasmusWL Sep 10, 2024
e11bfc2
Docs: Fix link
RasmusWL Sep 10, 2024
4a21a85
Merge branch 'main' into threat-models
RasmusWL Sep 23, 2024
535db98
Python: Minor simplification of `ActiveThreatModelSource`
RasmusWL Sep 23, 2024
431a1af
Merge branch 'main' into threat-models
RasmusWL Sep 26, 2024
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
1 change: 1 addition & 0 deletions python/ql/lib/qlpack.yml
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ dependencies:
codeql/dataflow: ${workspace}
codeql/mad: ${workspace}
codeql/regex: ${workspace}
codeql/threat-models: ${workspace}
codeql/tutorial: ${workspace}
codeql/util: ${workspace}
codeql/xml: ${workspace}
Expand Down
48 changes: 48 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,54 @@ 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.
*/
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.
*/
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
tausbn marked this conversation as resolved.
Show resolved Hide resolved
)
}
}

/**
* 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" }
}
}
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