Skip to content
Open
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
6 changes: 6 additions & 0 deletions unified/ql/consistency-queries/DataFlowConsistency.ql
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,12 @@ module ConsistencyInput implements InputSig<Location, DataFlowInput> {
predicate argHasPostUpdateExclude(DataFlowInput::ArgumentNode n) {
not exists(n.getBasicBlock()) // ignore unreachable data flow nodes
}

predicate reverseReadExclude(DataFlow::Node n) {
// When read steps are contributed by a language plugin we currently don't expect them to
// have post-update nodes for reverse-reads.
any(DataFlowPlugin p).step(n, any(Step s | s.read(_)), _)
}
}

module ConsistencyOutput =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import CallGraph
import Content
import ConstructorPatterns
import DataFlowCall
import DataFlowCallable
import DataFlowGraph
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
/**
* Provides data-flow modelling of constructor patterns / enum-case constructors.
*/
Comment on lines +1 to +3

private import unified
private import AllDataFlow
private import codeql.unified.internal.ExprPositions
private import codeql.unified.internal.NameBinding as NameBinding
private import codeql.unified.internal.typeinference.TypeInference as T

/**
* A constructor pattern, such as `Optional.some(let x)`.
*/
class ConstructorPattern extends CallExpr {
ConstructorPattern() { isInBindingContext(this, _) }
}

/**
* Gets the unqualified name of the enum-case contructor that might be referenced by `call`.
*/
Comment on lines +18 to +20
private string getShortConstructorName(CallExpr call) {
result = call.getCallee().(MemberAccessExpr).getMemberName()
// note: enum constructors can only be accessed qualified (possibly with leading-dot syntax)
// so do not do this for Identifiers
}

/**
* Holds if `call` targets a member called `name` and has the given `arity`.
*/
pragma[nomagic]
private predicate callSiteHasSignature(CallExpr call, string name, int arity) {
name = call.getCallee().(MemberAccessExpr).getMemberName() and
arity = call.getNumberOfArguments()
}

/**
* Holds if a constructor pattern has the given short `name` and `arity`.
*/
pragma[nomagic]
private predicate isSignatureUsedInConstructorPattern(string name, int arity) {
callSiteHasSignature(any(ConstructorPattern p), name, arity)
}

/** Holds if `callable` is an enum-case constructor */
private predicate isEnumCaseConstructor(ConstructorDeclaration callable) {
callable = any(ClassLikeDeclaration cls | cls.hasModifier("enum_case")).getAMember()
}

/**
* Holds if `call` resolves to a known enum-case constructor, or is assumed to resolve to an unseen enum-case constructor.
*/
pragma[nomagic]
private predicate assumeResolvesToEnumCaseConstructor(CallExpr call) {
call instanceof ConstructorPattern
or
isEnumCaseConstructor(T::resolveCallTarget(call))
or
// If the `E` in `E.foo(...)` could not be resolved, check if the name `foo` matches a constructor pattern.
exists(MemberAccessExpr callee, Expr base, string name, int arity |
callee = call.getCallee() and
base = callee.getBase() and
not exists(NameBinding::getStaticBindingTargetFromRef(base)) and
not exists(T::inferType(base)) and
callSiteHasSignature(call, name, arity) and
isSignatureUsedInConstructorPattern(name, arity)
)
}

/**
* Gets the field name for the enum-case data parameter corresponding to the given argument.
*/
string getEnumCaseParameterFieldFromArgument(CallExpr call, Argument arg) {
assumeResolvesToEnumCaseConstructor(call) and
exists(int i |
// Note: The label name is optional when calling an enum-case constructor, but the arguments
// must occur in declaration order, so use the raw argument index to handle both the labelled and unlabelled cases.
arg = call.getArgument(i) and
result = getShortConstructorName(call) + "." + i
Comment thread
asgerf marked this conversation as resolved.
)
}
13 changes: 12 additions & 1 deletion unified/ql/lib/codeql/unified/internal/dataflow/Content.qll
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,27 @@ private import unified
private import AllDataFlow

private newtype TContent =
TArrayElement() or
TNamedMember(string name) {
name = any(Identifier id).getValue()
or
// Tuple elements can be accessed as named members, e.g. `tuple.0`, `tuple.1`, etc,
// so just model their elements as named members.
name = [0 .. 20].toString()
or
name = getEnumCaseParameterFieldFromArgument(_, _)
}

class Content extends TContent {
string asNamedMember() { this = TNamedMember(result) }

string toString() { result = this.asNamedMember() }
predicate isArrayElement() { this = TArrayElement() }

string toString() {
result = this.asNamedMember()
or
this.isArrayElement() and result = "ArrayElement"
}

Location getLocation() { none() }
}
Expand All @@ -34,4 +43,6 @@ class ContentSet extends TContentSet {

module ContentSet {
ContentSet namedMember(string name) { result.asSingleton().asNamedMember() = name }

ContentSet arrayElement() { result.asSingleton().isArrayElement() }
}
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,49 @@ predicate step(Node node1, Step step, Node node2) {
node2.isPostUpdate(expr.getBase())
)
or
// Calls and constructor-patterns targeting an enum-case constructor.
exists(CallExpr call, Argument arg, string field |
field = getEnumCaseParameterFieldFromArgument(call, arg)
|
node1.isResultValue(arg.getValue()) and
step.storeName(field) and
node2.isResultValue(call)
or
node1.isIncomingValue(call) and
step.readName(field) and
node2.isIncomingValue(arg.getValue())
)
or
exists(SwitchExpr expr |
node1.isResultValue(expr.getValue()) and
step.value() and
node2.isIncomingValue(expr.getACase().getPattern())
)
or
exists(PatternGuardExpr expr |
node1.isResultValue(expr.getValue()) and
step.value() and
node2.isIncomingValue(expr.getPattern())
Comment thread
asgerf marked this conversation as resolved.
)
or
exists(ExprPattern expr |
node1.isIncomingValue(expr) and
step.value() and
node2.isIncomingValue(expr.getExpr())
)
or
exists(ArrayLiteral expr |
node1.isResultValue(expr.getAnElement()) and
step.store(ContentSet::arrayElement()) and
node2.isResultValue(expr)
)
or
exists(ForEachStmt stmt |
node1.isResultValue(stmt.getIterable()) and
step.readArrayElement() and
node2.isIncomingValue(stmt.getPattern())
)
or
none() // Temporarily disable compilation errors from unsatisfiable types
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,10 @@ module DataFlowInput implements InputSig<Location> {
// Misc
//
additional predicate nodeIsVisible(Node node) {
node instanceof TValueNode
exists(Expr e |
node = TValueNode(e) and
not e instanceof ExprPattern
)
or
node instanceof TStrictlyIncomingValue
or
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,5 +25,36 @@ private class SwiftDataFlowPlugin extends DataFlowPlugin {
step.value() and
node2.isResultValue(call)
)
or
exists(UnaryExpr expr |
expr.getOperator().(PostfixOperator).getValue() = "!" and
node1.isResultValue(expr.getOperand()) and
(step.readName("some.0") or step.taint()) and
node2.isResultValue(expr)
or
expr.getOperator().(PrefixOperator).getValue() = ["try", "try!", "await"] and
node1.isResultValue(expr.getOperand()) and
step.value() and
node2.isResultValue(expr)
or
expr.getOperator().(PrefixOperator).getValue() = "try?" and
node1.isResultValue(expr.getOperand()) and
step.storeName("some.0") and
node2.isResultValue(expr)
Comment thread
asgerf marked this conversation as resolved.
)
or
exists(TypeCastExpr expr |
// The `as?` type cast boxes the incoming value in Optional depending on whether the type cast succeeded
expr.getOperator().getValue() = "as?" and
node1.isResultValue(expr.getExpr()) and
step.storeName("some.0") and
node2.isResultValue(expr)
or
// Safe upcast conversion ("as") and downcast-or-throw ("as!") propagate the value directly
expr.getOperator().getValue() = ["as", "as!"] and
node1.isResultValue(expr.getExpr()) and
step.value() and
node2.isResultValue(expr)
)
}
}
8 changes: 8 additions & 0 deletions unified/ql/lib/codeql/unified/internal/dataflow/Step.qll
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,21 @@ class Step extends TStep {
pragma[nomagic]
predicate readName(string name) { this.read(ContentSet::namedMember(name)) }

/** Holds if this represents a step reading an element from an array. */
pragma[nomagic]
predicate readArrayElement() { this.read(ContentSet::arrayElement()) }

/** Holds if this represents a step storing into `contents`. */
predicate store(ContentSet contents) { this = TStoreStep(contents) }

/** Holds if this represents a step storing into the named member `name`. */
pragma[nomagic]
predicate storeName(string name) { this.store(ContentSet::namedMember(name)) }

/** Holds if this represents a step storing a value into an array. */
pragma[nomagic]
predicate storeArrayElement() { this.store(ContentSet::arrayElement()) }

string toString() {
this.value() and result = "value"
or
Expand Down
1 change: 1 addition & 0 deletions unified/ql/lib/ext/legacy-swift.model.yml
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ extensions:
- ["", "NSString", true, "init(contentsOfFile:usedEncoding:)", "", "", "ReturnValue", "local", "manual"]
- ["", "FileManager", true, "contents(atPath:)", "", "", "ReturnValue", "local", "manual"]
- ["", "Data", true, "init(contentsOf:options:)", "", "", "ReturnValue", "remote", "manual"]
- ["", "Data", true, "init(contentsOf:)", "", "", "ReturnValue", "remote", "manual"]
Comment thread
asgerf marked this conversation as resolved.
- ["", "UISceneDelegate", true, "scene(_:continue:)", "", "", "Parameter[continue:]", "remote", "manual"]
- ["", "UISceneDelegate", true, "scene(_:didUpdate:)", "", "", "Parameter[didUpdate:]", "remote", "manual"]
- ["", "UISceneDelegate", true, "scene(_:openURLContexts:)", "", "", "Parameter[openURLContexts:]", "remote", "manual"]
Expand Down
8 changes: 7 additions & 1 deletion unified/ql/src/queries/security/CWE-022/PathInjection.ql
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,13 @@ module PathInjectionConfig implements DataFlow::ConfigSig {
heuristicSink(node)
}

predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { none() }
predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
exists(MemberAccessExpr expr |
expr.getMemberName() = "path" and
node1.isResultValue(expr.getBase()) and
node2.isResultValue(expr)
Comment on lines +49 to +53

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm OK with this for now

)
}

predicate isBarrier(DataFlow::Node node) {
// TODO: add barriers
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
consistencyOverview
| deadEnd | 2 |
deadEnd
| enums.swift:2:10:2:22 | Entry |
| enums.swift:3:10:3:22 | Entry |
83 changes: 83 additions & 0 deletions unified/ql/test/library-tests/dataflow/enums.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
enum E {
case case1(String)
case case2(String)
}

func t1() {
let e = E.case1(source("t1.1"))
sink(e) // no flow
switch e {
case E.case1(let x):
sink(x) // $ hasValueFlow=t1.1
default:
break
}
}

func t2() {
let e = E.case1(source("t2.1"))
sink(e) // no flow
switch e {
case .case1(let x): // use leading-dot syntax
sink(x) // $ hasValueFlow=t2.1
default:
break
}
}

func t3() {
let e = E.case1(source("t3.1"))
guard let E.case1(x) = e else { return }
sink(x) // $ MISSING: hasValueFlow=t3.1
}

func t4() {
let e = E.case2(source("t4.1"))
switch e {
case E.case1(let x):
sink(x) // no flow
case E.case2(let x):
sink(x) // $ hasValueFlow=t4.1
}
// same but in opposite match order
switch e {
case E.case2(let x):
sink(x) // $ hasValueFlow=t4.1
case E.case1(let x):
sink(x) // no flow
}
}

func t5() {
let opt_x = Optional.some(source("t5.1"))
guard let x = opt_x else { return }
sink(x) // $ hasValueFlow=t5.1
}

func t6() {
let opt_x = Optional.some(source("t6.1"))
guard let opt_x else { return }
sink(opt_x) // $ hasValueFlow=t6.1
}

enum OptionalLabel {
case foo(x: String)
}

func t7() {
let e = OptionalLabel.foo(x: source("t7.1"))
switch e {
case .foo(let x):
sink(x) // $ hasValueFlow=t7.1
default:
break
}
// Note: swift-format will try to remove the 'x:' label in the call below
// swift-format-ignore
switch e {
case .foo(x: let x):
sink(x) // $ hasValueFlow=t7.1
default:
break
}
}
Loading
Loading