mirror of
https://github.com/github/codeql.git
synced 2025-12-21 19:26:31 +01:00
python: Rewrite path injection to use flow state
This removes the FP cause by chaining This PR also removes `ChainedConfigs12.qll`, as we hope to solve future problems via flow states.
This commit is contained in:
Rasmus Lerchedahl Petersen
@@ -1,83 +0,0 @@
|
||||
/**
|
||||
* This defines a `PathGraph` where sinks from `TaintTracking::Configuration`s are identified with
|
||||
* sources from `TaintTracking2::Configuration`s if they represent the same `ControlFlowNode`.
|
||||
*
|
||||
* Paths are then connected appropriately.
|
||||
*/
|
||||
|
||||
import python
|
||||
import semmle.python.dataflow.new.DataFlow
|
||||
import semmle.python.dataflow.new.DataFlow2
|
||||
import semmle.python.dataflow.new.TaintTracking
|
||||
import semmle.python.dataflow.new.TaintTracking2
|
||||
|
||||
/**
|
||||
* A `DataFlow::Node` that appears as a sink in Config1 and a source in Config2.
|
||||
*/
|
||||
private predicate crossoverNode(DataFlow::Node n) {
|
||||
any(TaintTracking::Configuration t1).isSink(n) and
|
||||
any(TaintTracking2::Configuration t2).isSource(n)
|
||||
}
|
||||
|
||||
/**
|
||||
* A new type which represents the union of the two sets of nodes.
|
||||
*/
|
||||
private newtype TCustomPathNode =
|
||||
Config1Node(DataFlow::PathNode node1) { not crossoverNode(node1.getNode()) } or
|
||||
Config2Node(DataFlow2::PathNode node2) { not crossoverNode(node2.getNode()) } or
|
||||
CrossoverNode(DataFlow::Node node) { crossoverNode(node) }
|
||||
|
||||
/**
|
||||
* A class representing the set of all the path nodes in either config.
|
||||
*/
|
||||
class CustomPathNode extends TCustomPathNode {
|
||||
/** Gets the PathNode if it is in Config1. */
|
||||
DataFlow::PathNode asNode1() {
|
||||
this = Config1Node(result) or this = CrossoverNode(result.getNode())
|
||||
}
|
||||
|
||||
/** Gets the PathNode if it is in Config2. */
|
||||
DataFlow2::PathNode asNode2() {
|
||||
this = Config2Node(result) or this = CrossoverNode(result.getNode())
|
||||
}
|
||||
|
||||
/**
|
||||
* Holds if this element is at the specified location.
|
||||
* The location spans column `startcolumn` of line `startline` to
|
||||
* column `endcolumn` of line `endline` in file `filepath`.
|
||||
* For more information, see
|
||||
* [Locations](https://codeql.github.com/docs/writing-codeql-queries/providing-locations-in-codeql-queries/).
|
||||
*/
|
||||
predicate hasLocationInfo(
|
||||
string filepath, int startline, int startcolumn, int endline, int endcolumn
|
||||
) {
|
||||
asNode1().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
|
||||
or
|
||||
asNode2().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
|
||||
}
|
||||
|
||||
/** Gets a textual representation of this element. */
|
||||
string toString() {
|
||||
result = asNode1().toString()
|
||||
or
|
||||
result = asNode2().toString()
|
||||
}
|
||||
}
|
||||
|
||||
/** Holds if `(a,b)` is an edge in the graph of data flow path explanations. */
|
||||
query predicate edges(CustomPathNode a, CustomPathNode b) {
|
||||
// Edge is in Config1 graph
|
||||
DataFlow::PathGraph::edges(a.asNode1(), b.asNode1())
|
||||
or
|
||||
// Edge is in Config2 graph
|
||||
DataFlow2::PathGraph::edges(a.asNode2(), b.asNode2())
|
||||
}
|
||||
|
||||
/** Holds if `n` is a node in the graph of data flow path explanations. */
|
||||
query predicate nodes(CustomPathNode n, string key, string val) {
|
||||
// Node is in Config1 graph
|
||||
DataFlow::PathGraph::nodes(n.asNode1(), key, val)
|
||||
or
|
||||
// Node is in Config2 graph
|
||||
DataFlow2::PathGraph::nodes(n.asNode2(), key, val)
|
||||
}
|
||||
@@ -2,107 +2,81 @@
|
||||
* Provides taint-tracking configurations for detecting "path injection" vulnerabilities.
|
||||
*
|
||||
* Note, for performance reasons: only import this file if
|
||||
* the Configurations or the `pathInjection` predicate are needed, otherwise
|
||||
* `PathInjection::Configuration` is needed, otherwise
|
||||
* `PathInjectionCustomizations` should be imported instead.
|
||||
*/
|
||||
|
||||
private import python
|
||||
private import semmle.python.Concepts
|
||||
private import semmle.python.dataflow.new.DataFlow
|
||||
private import semmle.python.dataflow.new.DataFlow2
|
||||
private import semmle.python.dataflow.new.TaintTracking
|
||||
private import semmle.python.dataflow.new.TaintTracking2
|
||||
import ChainedConfigs12
|
||||
import PathInjectionCustomizations::PathInjection
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Case 1. The path is never normalized.
|
||||
// ---------------------------------------------------------------------------
|
||||
/** Configuration to find paths from sources to sinks that contain no normalization. */
|
||||
class PathNotNormalizedConfiguration extends TaintTracking::Configuration {
|
||||
PathNotNormalizedConfiguration() { this = "PathNotNormalizedConfiguration" }
|
||||
|
||||
override predicate isSource(DataFlow::Node source) { source instanceof Source }
|
||||
|
||||
override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
|
||||
|
||||
override predicate isSanitizer(DataFlow::Node node) {
|
||||
node instanceof Sanitizer
|
||||
or
|
||||
node instanceof Path::PathNormalization
|
||||
}
|
||||
|
||||
override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
|
||||
guard instanceof SanitizerGuard
|
||||
}
|
||||
}
|
||||
import semmle.python.dataflow.new.DataFlow
|
||||
import semmle.python.dataflow.new.TaintTracking
|
||||
|
||||
/**
|
||||
* Holds if there is a path injection from source to sink, where the (python) path is
|
||||
* not normalized.
|
||||
* Provides a taint-tracking configuration for detecting "path injection" vulnerabilities.
|
||||
*/
|
||||
predicate pathNotNormalized(CustomPathNode source, CustomPathNode sink) {
|
||||
any(PathNotNormalizedConfiguration config).hasFlowPath(source.asNode1(), sink.asNode1())
|
||||
}
|
||||
module PathInjection {
|
||||
import PathInjectionCustomizations::PathInjection
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Case 2. The path is normalized at least once, but never checked afterwards.
|
||||
// ---------------------------------------------------------------------------
|
||||
/** Configuration to find paths from sources to normalizations that contain no prior normalizations. */
|
||||
class FirstNormalizationConfiguration extends TaintTracking::Configuration {
|
||||
FirstNormalizationConfiguration() { this = "FirstNormalizationConfiguration" }
|
||||
/**
|
||||
* A taint-tracking configuration for detecting "path injection" vulnerabilities.
|
||||
*
|
||||
* This configuration uses two flow states, `NotNormalized` and `NormalizedUnchecked`,
|
||||
* to track the requirement that a file path must be first normalized and then checked
|
||||
* before it is safe to use.
|
||||
*
|
||||
* At sources, paths are assumed not normalized. At normalization points, they change
|
||||
* state to `NormalizedUnchecked` after which they can be made safe by an appropriate
|
||||
* check of the prefix.
|
||||
*
|
||||
* Such checks are ineffective in the `NotNormalized` state.
|
||||
*/
|
||||
class Configuration extends TaintTracking::Configuration {
|
||||
Configuration() { this = "PathInjection" }
|
||||
|
||||
override predicate isSource(DataFlow::Node source) { source instanceof Source }
|
||||
override predicate isSource(DataFlow::Node source, DataFlow::FlowState state) {
|
||||
source instanceof Source and state instanceof NotNormalized
|
||||
}
|
||||
|
||||
override predicate isSink(DataFlow::Node sink) { sink instanceof Path::PathNormalization }
|
||||
override predicate isSink(DataFlow::Node sink, DataFlow::FlowState state) {
|
||||
sink instanceof Sink and
|
||||
(
|
||||
state instanceof NotNormalized or
|
||||
state instanceof NormalizedUnchecked
|
||||
)
|
||||
}
|
||||
|
||||
override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
|
||||
override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
|
||||
|
||||
override predicate isSanitizerOut(DataFlow::Node node) { node instanceof Path::PathNormalization }
|
||||
override predicate isBarrier(DataFlow::Node node, DataFlow::FlowState state) {
|
||||
// Block `NotNormalized` paths here, since they change state to `NormalizedUnchecked`
|
||||
node instanceof Path::PathNormalization and
|
||||
state instanceof NotNormalized
|
||||
or
|
||||
node = any(Path::SafeAccessCheck c).getAGuardedNode() and
|
||||
state instanceof NormalizedUnchecked
|
||||
}
|
||||
|
||||
override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
|
||||
guard instanceof SanitizerGuard
|
||||
override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
|
||||
guard instanceof SanitizerGuard
|
||||
}
|
||||
|
||||
override predicate isAdditionalFlowStep(
|
||||
DataFlow::Node nodeFrom, DataFlow::FlowState stateFrom, DataFlow::Node nodeTo,
|
||||
DataFlow::FlowState stateTo
|
||||
) {
|
||||
nodeFrom = nodeTo.(Path::PathNormalization).getPathArg() and
|
||||
stateFrom instanceof NotNormalized and
|
||||
stateTo instanceof NormalizedUnchecked
|
||||
}
|
||||
}
|
||||
|
||||
/** A state signifying that the file path has not been normalized. */
|
||||
class NotNormalized extends DataFlow::FlowState {
|
||||
NotNormalized() { this = "NotNormalized" }
|
||||
}
|
||||
|
||||
/** A state signifying that the file path has been normalized, but not checked. */
|
||||
class NormalizedUnchecked extends DataFlow::FlowState {
|
||||
NormalizedUnchecked() { this = "NormalizedUnchecked" }
|
||||
}
|
||||
}
|
||||
|
||||
/** Configuration to find paths from normalizations to sinks that do not go through a check. */
|
||||
class NormalizedPathNotCheckedConfiguration extends TaintTracking2::Configuration {
|
||||
NormalizedPathNotCheckedConfiguration() { this = "NormalizedPathNotCheckedConfiguration" }
|
||||
|
||||
override predicate isSource(DataFlow::Node source) { source instanceof Path::PathNormalization }
|
||||
|
||||
override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
|
||||
|
||||
override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
|
||||
|
||||
override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
|
||||
guard instanceof Path::SafeAccessCheck
|
||||
or
|
||||
guard instanceof SanitizerGuard
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Holds if there is a path injection from source to sink, where the (python) path is
|
||||
* normalized at least once, but never checked afterwards.
|
||||
*/
|
||||
predicate pathNotCheckedAfterNormalization(CustomPathNode source, CustomPathNode sink) {
|
||||
exists(
|
||||
FirstNormalizationConfiguration config, DataFlow::PathNode mid1, DataFlow2::PathNode mid2,
|
||||
NormalizedPathNotCheckedConfiguration config2
|
||||
|
|
||||
config.hasFlowPath(source.asNode1(), mid1) and
|
||||
config2.hasFlowPath(mid2, sink.asNode2()) and
|
||||
mid1.getNode().asCfgNode() = mid2.getNode().asCfgNode()
|
||||
)
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Query: Either case 1 or case 2.
|
||||
// ---------------------------------------------------------------------------
|
||||
/** Holds if there is a path injection from source to sink */
|
||||
predicate pathInjection(CustomPathNode source, CustomPathNode sink) {
|
||||
pathNotNormalized(source, sink)
|
||||
or
|
||||
pathNotCheckedAfterNormalization(source, sink)
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user