Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
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 @@ -643,6 +643,15 @@ module ModelOutput {
baseNode = getInvocationFromPath(type, path)
}

/**
* Holds if a `baseNode` is an invocation identified by the `type,path` part of a summary row.
*/
cached
predicate resolvedSummaryRefBase(string type, string path, API::Node baseNode) {
summaryModel(type, path, _, _, _) and
baseNode = getNodeFromPath(type, path)
}

/**
* Holds if `node` is seen as an instance of `type` due to a type definition
* contributed by a CSV model.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: feature
---
* It is now possible to specify flow summaries in the format "MyPkg;Member[list_map];Argument[1].ListElement;Argument[0].Parameter[0];value"
65 changes: 29 additions & 36 deletions python/ql/lib/semmle/python/dataflow/new/FlowSummary.qll
Original file line number Diff line number Diff line change
Expand Up @@ -90,39 +90,32 @@ abstract class SummarizedCallable extends LibraryCallable, Impl::Public::Summari
}

class RequiredSummaryComponentStack = Impl::Public::RequiredSummaryComponentStack;
// // This gives access to getNodeFromPath, which is not constrained to `CallNode`s
// // as `resolvedSummaryBase` is.
// private import semmle.python.frameworks.data.internal.ApiGraphModels as AGM
//
// private class SummarizedCallableFromModel extends SummarizedCallable {
// string package;
// string type;
// string path;
// SummarizedCallableFromModel() {
// ModelOutput::relevantSummaryModel(package, type, path, _, _, _) and
// this = package + ";" + type + ";" + path
// }
// override CallCfgNode getACall() {
// exists(API::CallNode base |
// ModelOutput::resolvedSummaryBase(package, type, path, base) and
// result = base.getACall()
// )
// }
// override ArgumentNode getACallback() {
// exists(API::Node base |
// base = AGM::getNodeFromPath(package, type, path) and
// result = base.getAValueReachableFromSource()
// )
// }
// override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
// exists(string kind |
// ModelOutput::relevantSummaryModel(package, type, path, input, output, kind)
// |
// kind = "value" and
// preservesValue = true
// or
// kind = "taint" and
// preservesValue = false
// )
// }
// }

private class SummarizedCallableFromModel extends SummarizedCallable {
string type;
string path;

SummarizedCallableFromModel() {
ModelOutput::relevantSummaryModel(type, path, _, _, _) and
this = type + ";" + path
}

override CallCfgNode getACall() { ModelOutput::resolvedSummaryBase(type, path, result) }

override ArgumentNode getACallback() {
exists(API::Node base |
ModelOutput::resolvedSummaryRefBase(type, path, base) and
result = base.getAValueReachableFromSource()
)
}

override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
exists(string kind | ModelOutput::relevantSummaryModel(type, path, input, output, kind) |
kind = "value" and
preservesValue = true
or
kind = "taint" and
preservesValue = false
)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -643,6 +643,15 @@ module ModelOutput {
baseNode = getInvocationFromPath(type, path)
}

/**
* Holds if a `baseNode` is an invocation identified by the `type,path` part of a summary row.
*/
cached
predicate resolvedSummaryRefBase(string type, string path, API::Node baseNode) {
summaryModel(type, path, _, _, _) and
baseNode = getNodeFromPath(type, path)
}

/**
* Holds if `node` is seen as an instance of `type` due to a type definition
* contributed by a CSV model.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
argumentToEnsureNotTaintedNotMarkedAsSpurious
untaintedArgumentToEnsureTaintedNotMarkedAsMissing
failures
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import python
private import TestSummaries
import experimental.meta.InlineTaintTest
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
missingAnnotationOnSink
failures
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import python
private import TestSummaries
import experimental.dataflow.TestUtil.NormalDataflowTest
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
private import python
private import semmle.python.dataflow.new.FlowSummary
private import semmle.python.frameworks.data.ModelsAsData
private import semmle.python.ApiGraphs

private class StepsFromModel extends ModelInput::SummaryModelCsv {
override predicate row(string row) {
row =
[
"foo;Member[MS_identity];Argument[0];ReturnValue;value",
"foo;Member[MS_apply_lambda];Argument[1];Argument[0].Parameter[0];value",
"foo;Member[MS_apply_lambda];Argument[0].ReturnValue;ReturnValue;value",
"foo;Member[MS_reversed];Argument[0].ListElement;ReturnValue.ListElement;value",
"foo;Member[MS_reversed];Argument[0];ReturnValue;taint",
"foo;Member[MS_list_map];Argument[1].ListElement;Argument[0].Parameter[0];value",
"foo;Member[MS_list_map];Argument[0].ReturnValue;ReturnValue.ListElement;value",
"foo;Member[MS_list_map];Argument[1];ReturnValue;taint",
"foo;Member[MS_append_to_list];Argument[0].ListElement;ReturnValue.ListElement;value",
"foo;Member[MS_append_to_list];Argument[1];ReturnValue.ListElement;value",
"foo;Member[MS_append_to_list];Argument[0];ReturnValue;taint",
"foo;Member[MS_append_to_list];Argument[1];ReturnValue;taint",
"json;Member[MS_loads];Argument[0];ReturnValue;taint"
]
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,128 @@

import sys
import os

sys.path.append(os.path.dirname(os.path.dirname((__file__))))
from testlib import expects

# These are defined so that we can evaluate the test code.
NONSOURCE = "not a source"
SOURCE = "source"


def is_source(x):
return x == "source" or x == b"source" or x == 42 or x == 42.0 or x == 42j


def SINK(x):
if is_source(x):
print("OK")
else:
print("Unexpected flow", x)


def SINK_F(x):
if is_source(x):
print("Unexpected flow", x)
else:
print("OK")

ensure_tainted = ensure_not_tainted = print
TAINTED_STRING = "TAINTED_STRING"

from foo import MS_identity, MS_apply_lambda, MS_reversed, MS_list_map, MS_append_to_list

# Simple summary
via_identity = MS_identity(SOURCE)
SINK(via_identity) # $ flow="SOURCE, l:-1 -> via_identity"

tainted = MS_identity(TAINTED_STRING)
ensure_tainted(tainted) # $ tainted
Copy link
Member

Choose a reason for hiding this comment

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

Since you have just shown that we have data-flow, and we know all dataflow steps are also taint-flow steps, I think it's fine to only check taint-flow for the cases where there is NOT dataflow.

I think that will make the test-file a bit easier to read as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I tried to make it more consistent now by not checking taint when dataflow is already established and when we do check taint, check both the collection and the expected element.

Copy link
Member

Choose a reason for hiding this comment

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

Thinking some more about this, I think these tests should be about whether you can write flow summaries in CSV files that do the right thing.

Having all these extra taint steps are closer to what our current taint-tracking does in the end, but from my point of view, doesn't help us achieve the goal of the tests.

I removed the TAINTED_LIST parts locally, but thought it was a bit too controversial to just commit directly to your PR -- instead I've put the commits here: yoff#79 (if you agree, we can add these to main later on 👍)



# Lambda summary
via_lambda = MS_apply_lambda(lambda x: [x], SOURCE)
SINK(via_lambda[0]) # $ flow="SOURCE, l:-1 -> via_lambda[0]"

tainted_lambda = MS_apply_lambda(lambda x: [x], TAINTED_STRING)
ensure_tainted(tainted_lambda) # $ tainted


# A lambda that breaks the flow
not_via_lambda = MS_apply_lambda(lambda x: 1, SOURCE)
SINK_F(not_via_lambda)

untainted_lambda = MS_apply_lambda(lambda x: 1, TAINTED_STRING)
ensure_not_tainted(untainted_lambda)

# Collection summaries
via_reversed = MS_reversed([SOURCE])
SINK(via_reversed[0]) # $ flow="SOURCE, l:-1 -> via_reversed[0]"

tainted_list = MS_reversed([TAINTED_STRING])
ensure_tainted(tainted_list[0]) # $ tainted

# Complex summaries
def box(x):
return [x]

via_map = MS_list_map(box, [SOURCE])
SINK(via_map[0][0]) # $ flow="SOURCE, l:-1 -> via_map[0][0]"

tainted_mapped = MS_list_map(box, [TAINTED_STRING])
ensure_tainted(tainted_mapped[0][0]) # $ tainted

def explicit_identity(x):
return x

via_map_explicit = MS_list_map(explicit_identity, [SOURCE])
SINK(via_map_explicit[0]) # $ flow="SOURCE, l:-1 -> via_map_explicit[0]"

tainted_mapped_explicit = MS_list_map(explicit_identity, [TAINTED_STRING])
tainted_mapped_explicit_implicit = MS_list_map(explicit_identity, TAINTED_LIST)
ensure_tainted(
tainted_mapped_explicit, # $ tainted
tainted_mapped_explicit[0], # $ tainted
tainted_mapped_explicit_implicit, # $ tainted
tainted_mapped_explicit_implicit[0] # $ tainted
)

via_map_summary = MS_list_map(MS_identity, [SOURCE])
SINK(via_map_summary[0]) # $ flow="SOURCE, l:-1 -> via_map_summary[0]"

tainted_mapped_summary = MS_list_map(MS_identity, [TAINTED_STRING])
tainted_mapped_summary_implicit = MS_list_map(MS_identity, TAINTED_LIST)
ensure_tainted(
tainted_mapped_summary, # $ tainted
tainted_mapped_summary[0], # $ tainted
tainted_mapped_summary_implicit, # $ tainted
tainted_mapped_summary_implicit[0] # $ tainted
)

via_append_el = MS_append_to_list([], SOURCE)
SINK(via_append_el[0]) # $ flow="SOURCE, l:-1 -> via_append_el[0]"

tainted_list_el = MS_append_to_list([], TAINTED_STRING)
ensure_tainted(
tainted_list_el, # $ tainted
tainted_list_el[0] # $ tainted
)

via_append = MS_append_to_list([SOURCE], NONSOURCE)
SINK(via_append[0]) # $ flow="SOURCE, l:-1 -> via_append[0]"

tainted_list = MS_append_to_list([TAINTED_STRING], NONSOURCE)
tainted_list_implicit = MS_append_to_list(TAINTED_LIST, NONSOURCE)
ensure_tainted(
tainted_list, # $ tainted
tainted_list[0], # $ tainted
tainted_list_implicit, # $ tainted
tainted_list_implicit[0] # $ tainted
)

from json import MS_loads as json_loads
tainted_resultlist = json_loads(TAINTED_STRING)
ensure_tainted(
tainted_resultlist, # $ tainted
tainted_resultlist[0] # $ tainted
)
Original file line number Diff line number Diff line change
Expand Up @@ -643,6 +643,15 @@ module ModelOutput {
baseNode = getInvocationFromPath(type, path)
}

/**
* Holds if a `baseNode` is an invocation identified by the `type,path` part of a summary row.
*/
cached
predicate resolvedSummaryRefBase(string type, string path, API::Node baseNode) {
summaryModel(type, path, _, _, _) and
baseNode = getNodeFromPath(type, path)
}

/**
* Holds if `node` is seen as an instance of `type` due to a type definition
* contributed by a CSV model.
Expand Down