Skip to content

Commit

Permalink
Merge pull request #18265 from asgerf/jss/flow-labels2
Browse files Browse the repository at this point in the history
JS: Migrate all queries to proper flow states and deprecate FlowLabel
  • Loading branch information
asgerf authored Dec 17, 2024
2 parents a53d294 + e5ae7e0 commit 729efff
Show file tree
Hide file tree
Showing 77 changed files with 1,135 additions and 653 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -416,11 +416,11 @@ additional taint step from the first argument of ``resolveSymlinks`` to its resu
// ...
predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
exists(DataFlow::CallNode c |
c = DataFlow::moduleImport("resolve-symlinks").getACall() and
pred = c.getArgument(0) and
succ = c
node1 = c.getArgument(0) and
node2 = c
)
}
}
Expand All @@ -431,11 +431,11 @@ to wrap it in a new subclass of ``TaintTracking::SharedTaintStep`` like this:
.. code-block:: ql
class StepThroughResolveSymlinks extends TaintTracking::SharedTaintStep {
override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
override predicate step(DataFlow::Node node1, DataFlow::Node node2) {
exists(DataFlow::CallNode c |
c = DataFlow::moduleImport("resolve-symlinks").getACall() and
pred = c.getArgument(0) and
succ = c
node1 = c.getArgument(0) and
node2 = c
)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,9 @@ module IdorTaintConfig implements DataFlow::ConfigSig {

predicate isSink(DataFlow::Node node) { exists(ClientRequest req | node = req.getADataNode()) }

predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
// Step from x -> { userId: x }
succ.(DataFlow::SourceNode).getAPropertyWrite("userId").getRhs() = pred
node2.(DataFlow::SourceNode).getAPropertyWrite("userId").getRhs() = node1
}

predicate isBarrier(DataFlow::Node node) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,16 +37,16 @@ module AuthKeyTrackingConfig implements DataFlow::ConfigSig {
)
}

predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
// Step into objects: x -> { f: x }
succ.(DataFlow::SourceNode).getAPropertyWrite().getRhs() = pred
node2.(DataFlow::SourceNode).getAPropertyWrite().getRhs() = node1
or
// Step through JSON serialization: x -> JSON.stringify(x)
// Note: TaintTracking::Configuration includes this step by default, but not DataFlow::Configuration
exists(DataFlow::CallNode call |
call = DataFlow::globalVarRef("JSON").getAMethodCall("stringify") and
pred = call.getArgument(0) and
succ = call
node1 = call.getArgument(0) and
node2 = call
)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -147,10 +147,12 @@ class LegacyFlowStep extends Unit {
predicate step(DataFlow::Node pred, DataFlow::Node succ) { none() }

/**
* DEPRECATED. The `FlowLabel` class and steps involving flow labels are no longer used by any queries.
*
* Holds if `pred` → `succ` should be considered a data flow edge
* transforming values with label `predlbl` to have label `succlbl`.
*/
predicate step(
deprecated predicate step(
DataFlow::Node pred, DataFlow::Node succ, DataFlow::FlowLabel predlbl,
DataFlow::FlowLabel succlbl
) {
Expand Down Expand Up @@ -199,11 +201,13 @@ module LegacyFlowStep {
}

/**
* DEPRECATED. The `FlowLabel` class and steps involving flow labels are no longer used by any queries.
*
* Holds if `pred` → `succ` should be considered a data flow edge
* transforming values with label `predlbl` to have label `succlbl`.
*/
cached
predicate step(
deprecated predicate step(
DataFlow::Node pred, DataFlow::Node succ, DataFlow::FlowLabel predlbl,
DataFlow::FlowLabel succlbl
) {
Expand Down Expand Up @@ -273,10 +277,12 @@ class SharedFlowStep extends Unit {
predicate step(DataFlow::Node pred, DataFlow::Node succ) { none() }

/**
* DEPRECATED. The `FlowLabel` class and steps involving flow labels are no longer used by any queries.
*
* Holds if `pred` → `succ` should be considered a data flow edge
* transforming values with label `predlbl` to have label `succlbl`.
*/
predicate step(
deprecated predicate step(
DataFlow::Node pred, DataFlow::Node succ, DataFlow::FlowLabel predlbl,
DataFlow::FlowLabel succlbl
) {
Expand Down Expand Up @@ -353,10 +359,12 @@ module SharedFlowStep {

// The following are aliases for old step predicates that have no corresponding predicate in AdditionalFlowStep
/**
* DEPRECATED. The `FlowLabel` class and steps involving flow labels are no longer used by any queries.
*
* Holds if `pred` → `succ` should be considered a data flow edge
* transforming values with label `predlbl` to have label `succlbl`.
*/
predicate step(
deprecated predicate step(
DataFlow::Node pred, DataFlow::Node succ, DataFlow::FlowLabel predlbl,
DataFlow::FlowLabel succlbl
) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,9 @@ deprecated private predicate isBarrierGuardInternal(
}

/**
* DEPRECATED. Use a query-specific `FlowState` class instead.
* See [guide on using flow state](https://codeql.github.com/docs/codeql-language-guides/using-flow-labels-for-precise-data-flow-analysis) for more details.
*
* A label describing the kind of information tracked by a flow configuration.
*
* There are two standard labels "data" and "taint".
Expand All @@ -303,7 +306,7 @@ deprecated private predicate isBarrierGuardInternal(
* - "taint" additionally permits flow through transformations such as string operations,
* and is the default flow source for a `TaintTracking::Configuration`.
*/
abstract class FlowLabel extends string {
abstract deprecated class FlowLabel extends string {
bindingset[this]
FlowLabel() { any() }

Expand Down Expand Up @@ -341,7 +344,7 @@ deprecated class StandardFlowLabel extends FlowLabel {
StandardFlowLabel() { this = "data" or this = "taint" }
}

module FlowLabel {
deprecated module FlowLabel {
/**
* Gets the standard flow label for describing values that directly originate from a flow source.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,17 +36,19 @@ module MakeBarrierGuard<BarrierGuardSig BaseGuard> {
}
}

private signature class LabeledBarrierGuardSig extends DataFlow::Node {
/**
* Holds if this node acts as a barrier for `label`, blocking further flow from `e` if `this` evaluates to `outcome`.
*/
predicate blocksExpr(boolean outcome, Expr e, DataFlow::FlowLabel label);
deprecated private module DeprecationWrapper {
signature class LabeledBarrierGuardSig extends DataFlow::Node {
/**
* Holds if this node acts as a barrier for `label`, blocking further flow from `e` if `this` evaluates to `outcome`.
*/
predicate blocksExpr(boolean outcome, Expr e, DataFlow::FlowLabel label);
}
}

/**
* Converts a barrier guard class to a set of nodes to include in an implementation of `isBarrier(node, label)`.
*/
module MakeLabeledBarrierGuard<LabeledBarrierGuardSig BaseGuard> {
deprecated module MakeLabeledBarrierGuard<DeprecationWrapper::LabeledBarrierGuardSig BaseGuard> {
final private class FinalBaseGuard = BaseGuard;

private class Adapter extends FinalBaseGuard {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -545,9 +545,9 @@ class Boolean extends boolean {
/**
* A summary of an inter-procedural data flow path.
*/
newtype TPathSummary =
deprecated newtype TPathSummary =
/** A summary of an inter-procedural data flow path. */
MkPathSummary(Boolean hasReturn, Boolean hasCall, FlowLabel start, FlowLabel end)
deprecated MkPathSummary(Boolean hasReturn, Boolean hasCall, FlowLabel start, FlowLabel end)

/**
* A summary of an inter-procedural data flow path.
Expand All @@ -560,7 +560,7 @@ newtype TPathSummary =
* We only want to build properly matched call/return sequences, so if a path has both
* call steps and return steps, all return steps must precede all call steps.
*/
class PathSummary extends TPathSummary {
deprecated class PathSummary extends TPathSummary {
Boolean hasReturn;
Boolean hasCall;
FlowLabel start;
Expand Down Expand Up @@ -634,7 +634,7 @@ class PathSummary extends TPathSummary {
}
}

module PathSummary {
deprecated module PathSummary {
/**
* Gets a summary describing a path without any calls or returns.
*/
Expand Down
90 changes: 90 additions & 0 deletions javascript/ql/lib/semmle/javascript/security/CommonFlowState.qll
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
/**
* Contains a class with flow states that are used by multiple queries.
*/

private import javascript
private import TaintedUrlSuffixCustomizations
private import TaintedObjectCustomizations

private newtype TFlowState =
TTaint() or
TTaintedUrlSuffix() or
TTaintedPrefix() or
TTaintedObject()

/**
* A flow state indicating which part of a value is tainted.
*/
class FlowState extends TFlowState {
/**
* Holds if this represents a value that is considered entirely tainted, except the first character
* might not be user-controlled.
*/
predicate isTaint() { this = TTaint() }

/**
* Holds if this represents a URL whose fragment and/or query parts are considered tainted.
*/
predicate isTaintedUrlSuffix() { this = TTaintedUrlSuffix() }

/**
* Holds if this represents a string whose prefix is known to be tainted.
*/
predicate isTaintedPrefix() { this = TTaintedPrefix() }

/**
* Holds if this represents a deeply tainted object, such as a JSON object
* parsed from user-controlled data.
*/
predicate isTaintedObject() { this = TTaintedObject() }

/** Gets a string representation of this flow state. */
string toString() {
this.isTaint() and result = "taint"
or
this.isTaintedUrlSuffix() and result = "tainted-url-suffix"
or
this.isTaintedPrefix() and result = "tainted-prefix"
or
this.isTaintedObject() and result = "tainted-object"
}

/** DEPRECATED. Gets the corresponding flow label. */
deprecated DataFlow::FlowLabel toFlowLabel() {
this.isTaint() and result.isTaint()
or
this.isTaintedUrlSuffix() and result = TaintedUrlSuffix::label()
or
this.isTaintedPrefix() and result = "PrefixString"
or
this.isTaintedObject() and result = TaintedObject::label()
}
}

/** Convenience predicates for working with common flow states. */
module FlowState {
/**
* Gets the flow state representing a value that is considered entirely tainted, except the first character
* might not be user-controlled.
*/
FlowState taint() { result.isTaint() }

/**
* Gets the flow state representing a URL whose fragment and/or query parts are considered tainted.
*/
FlowState taintedUrlSuffix() { result.isTaintedUrlSuffix() }

/**
* Gets the flow state representing a string whose prefix is known to be tainted.
*/
FlowState taintedPrefix() { result.isTaintedPrefix() }

/**
* Gets the flow state representing a deeply tainted object, such as a JSON object
* parsed from user-controlled data.
*/
FlowState taintedObject() { result.isTaintedObject() }

/** DEPRECATED. Gets the flow state corresponding to `label`. */
deprecated FlowState fromFlowLabel(DataFlow::FlowLabel label) { result.toFlowLabel() = label }
}
Loading

0 comments on commit 729efff

Please sign in to comment.