Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
4 changes: 0 additions & 4 deletions config/identical-files.json
Original file line number Diff line number Diff line change
Expand Up @@ -454,10 +454,6 @@
"ruby/ql/lib/codeql/ruby/security/internal/SensitiveDataHeuristics.qll",
"swift/ql/lib/codeql/swift/security/internal/SensitiveDataHeuristics.qll"
],
"SummaryTypeTracker": [
"python/ql/lib/semmle/python/dataflow/new/internal/SummaryTypeTracker.qll",
"ruby/ql/lib/codeql/ruby/typetracking/internal/SummaryTypeTracker.qll"
],
"IncompleteUrlSubstringSanitization": [
"javascript/ql/src/Security/CWE-020/IncompleteUrlSubstringSanitization.qll",
"ruby/ql/src/queries/security/cwe-020/IncompleteUrlSubstringSanitization.qll"
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* Python now makes use of the shared type tracking library, exposed as `semmle.python.dataflow.new.TypeTracking`. The existing type tracking library, `semmle.python.dataflow.new.TypeTracker`, has consequently been deprecated.
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@
*/

import python
private import semmle.python.dataflow.new.internal.TypeTrackerSpecific
private import semmle.python.dataflow.new.internal.TypeTrackingImpl
private import semmle.python.ApiGraphs

class CallCfgNodeWithTarget extends DataFlow::Node instanceof DataFlow::CallCfgNode {
DataFlow::Node getTarget() { returnStep(result, this) }
DataFlow::Node getTarget() { TypeTrackingInput::returnStep(result, this) }
}
20 changes: 12 additions & 8 deletions python/ql/lib/semmle/python/dataflow/new/TypeTracker.qll
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
/**
* DEPRECATED: Use `semmle.python.dataflow.new.TypeTracking` instead.
*
* This file acts as a wrapper for `internal.TypeTracker`, exposing some of the functionality with
* names that are more appropriate for Python.
*/
Expand All @@ -8,12 +10,14 @@ private import internal.TypeTracker as Internal
private import internal.TypeTrackerSpecific as InternalSpecific

/** A string that may appear as the name of an attribute or access path. */
class AttributeName = InternalSpecific::TypeTrackerContent;
deprecated class AttributeName = InternalSpecific::TypeTrackerContent;

/** An attribute name, or the empty string (representing no attribute). */
class OptionalAttributeName = InternalSpecific::OptionalTypeTrackerContent;
deprecated class OptionalAttributeName = InternalSpecific::OptionalTypeTrackerContent;

/**
* DEPRECATED: Use `semmle.python.dataflow.new.TypeTracking` instead.
*
* The summary of the steps needed to track a value to a given dataflow node.
*
* This can be used to track objects that implement a certain API in order to
Expand All @@ -40,7 +44,7 @@ class OptionalAttributeName = InternalSpecific::OptionalTypeTrackerContent;
* `t = t2.step(myType(t2), result)`. If you additionally want to track individual
* intra-procedural steps, use `t = t2.smallstep(myCallback(t2), result)`.
*/
class TypeTracker extends Internal::TypeTracker {
deprecated class TypeTracker extends Internal::TypeTracker {
/**
* Holds if this is the starting point of type tracking, and the value starts in the attribute named `attrName`.
* The type tracking only ends after the attribute has been loaded.
Expand All @@ -55,12 +59,12 @@ class TypeTracker extends Internal::TypeTracker {
string getAttr() { result = this.getContent() }
}

module TypeTracker = Internal::TypeTracker;
deprecated module TypeTracker = Internal::TypeTracker;

class StepSummary = Internal::StepSummary;
deprecated class StepSummary = Internal::StepSummary;

module StepSummary = Internal::StepSummary;
deprecated module StepSummary = Internal::StepSummary;

class TypeBackTracker = Internal::TypeBackTracker;
deprecated class TypeBackTracker = Internal::TypeBackTracker;

module TypeBackTracker = Internal::TypeBackTracker;
deprecated module TypeBackTracker = Internal::TypeBackTracker;
56 changes: 56 additions & 0 deletions python/ql/lib/semmle/python/dataflow/new/TypeTracking.qll
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
/**
* Provides classes and predicates for simple data-flow reachability suitable
* for tracking types.
*/

private import internal.TypeTrackingImpl as Impl
import Impl::Shared::TypeTracking<Impl::TypeTrackingInput>

/** A string that may appear as the name of an attribute or access path. */
class AttributeName = Impl::TypeTrackingInput::Content;

/**
* A summary of the steps needed to track a value to a given dataflow node.
*
* This can be used to track objects that implement a certain API in order to
* recognize calls to that API. Note that type-tracking does not by itself provide a
* source/sink relation, that is, it may determine that a node has a given type,
* but it won't determine where that type came from.
*
* It is recommended that all uses of this type are written in the following form,
* for tracking some type `myType`:
* ```ql
* Node myType(TypeTracker tt) {
* tt.start() and
* result = < source of myType >
* or
* exists(TypeTracker tt2 |
* tt = tt2.step(myType(tt2), result)
* )
* }
*
* Node myType() { myType(TypeTracker::end()).flowsTo(result) }
* ```
*
* If you want to track individual intra-procedural steps, use `tt2.smallstep`
* instead of `tt2.step`.
*/
class TypeTracker extends Impl::TypeTracker {
/**
* Holds if this is the starting point of type tracking, and the value starts in the attribute named `attrName`.
* The type tracking only ends after the attribute has been loaded.
*/
predicate startInAttr(string attrName) { this.startInContent(attrName) }

/**
* INTERNAL. DO NOT USE.
*
* Gets the attribute associated with this type tracker.
*/
string getAttr() {
result = this.getContent().asSome()
or
this.getContent().isNone() and
result = ""
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ private import DataFlowPublic
private import DataFlowPrivate
private import FlowSummaryImpl as FlowSummaryImpl
private import semmle.python.internal.CachedStages
private import semmle.python.dataflow.new.internal.TypeTracker::CallGraphConstruction as CallGraphConstruction
private import semmle.python.dataflow.new.internal.TypeTrackingImpl::CallGraphConstruction as CallGraphConstruction

newtype TParameterPosition =
/** Used for `self` in methods, and `cls` in classmethods. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

private import python
private import DataFlowPrivate
import semmle.python.dataflow.new.TypeTracker
import semmle.python.dataflow.new.TypeTracking
import Attributes
import LocalSources
private import semmle.python.essa.SsaCompute
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
private import python
private import semmle.python.dataflow.new.DataFlow
private import semmle.python.dataflow.new.internal.ImportStar
private import semmle.python.dataflow.new.TypeTracker
private import semmle.python.dataflow.new.TypeTracking
private import semmle.python.dataflow.new.internal.DataFlowPrivate

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,8 @@ class LocalSourceNode extends Node {
// We include all scope entry definitions, as these act as the local source within the scope they
// enter.
this.asCfgNode() = any(ScopeEntryDefinition def).getDefiningNode()
or
this instanceof ParameterNode
Comment on lines +75 to +76
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, I am surprised we did not have this before!

I am curious, did you observe test-failures from this?
Given the first case

this instanceof ExprNode and
    not simpleLocalFlowStepForTypetracking(_, this)

this will only add synthetic parameter nodes (I also verified this with a quick query), but there can still be lots of those...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow, I am surprised we did not have this before!

Me too.

I am curious, did you observe test-failures from this?

The shared type tracking library (sensibly) assumes parameters to be local source nodes:

predicate callStep(Node nodeFrom, LocalSourceNode nodeTo);

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah!

}

/** Holds if this `LocalSourceNode` can flow to `nodeTo` in one or more local flow steps. */
Expand Down Expand Up @@ -151,7 +153,7 @@ class LocalSourceNode extends Node {
* See `TypeBackTracker` for more details about how to use this.
*/
pragma[inline]
LocalSourceNode backtrack(TypeBackTracker t2, TypeBackTracker t) { t2 = t.step(result, this) }
LocalSourceNode backtrack(TypeBackTracker t2, TypeBackTracker t) { t = t2.step(result, this) }
Copy link
Contributor

Choose a reason for hiding this comment

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

This change makes me wonder if the QLDoc for track and backtrack ought to mention t and t2. But I guess now it is at least aligned with other language implementations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think the QL doc changed here?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, and even if it had mentioned t and t2 it would not have to change. It just makes it clear that there are two possible conventions here and we have two implementations that made different choices. So we should probably make explicit what role t and t2 are supposed to play. It is tangential to this PR, though.

}

/**
Expand Down Expand Up @@ -238,7 +240,7 @@ private module Cached {
* Helper predicate for `hasLocalSource`. Removes any steps go to module variable reads, as these
* are already local source nodes in their own right.
*/
cached
pragma[nomagic]
private predicate localSourceFlowStep(Node nodeFrom, Node nodeTo) {
simpleLocalFlowStep(nodeFrom, nodeTo) and
not nodeTo = any(ModuleVariableNode v).getARead()
Expand Down
Loading