diff --git a/change-notes/1.23/analysis-csharp.md b/change-notes/1.23/analysis-csharp.md index 7836345fdf67..7ec412a0eb23 100644 --- a/change-notes/1.23/analysis-csharp.md +++ b/change-notes/1.23/analysis-csharp.md @@ -40,5 +40,8 @@ The following changes in version 1.23 affect C# analysis in all applications. overriding `int explorationLimit()`. * `foreach` statements where the body is guaranteed to be executed at least once, such as `foreach (var x in new string[]{ "a", "b", "c" }) { ... }`, are now recognized by all analyses based on the control flow graph (such as SSA, data flow and taint tracking). * Fixed the control flow graph for `switch` statements where the `default` case was not the last case. This had caused the remaining cases to be unreachable. `SwitchStmt.getCase(int i)` now puts the `default` case last. +* There is now a `DataFlow::localExprFlow` predicate and a + `TaintTracking::localExprTaint` predicate to make it easy to use the most + common case of local data flow and taint: from one `Expr` to another. ## Changes to autobuilder diff --git a/csharp/ql/src/Bad Practices/Implementation Hiding/ExposeRepresentation.ql b/csharp/ql/src/Bad Practices/Implementation Hiding/ExposeRepresentation.ql index 3847eae6e824..26327ed6d87a 100644 --- a/csharp/ql/src/Bad Practices/Implementation Hiding/ExposeRepresentation.ql +++ b/csharp/ql/src/Bad Practices/Implementation Hiding/ExposeRepresentation.ql @@ -32,7 +32,7 @@ predicate returnsCollection(Callable c, Field f) { predicate mayWriteToCollection(Expr modified) { modified instanceof CollectionModificationAccess or - exists(Expr mid | mayWriteToCollection(mid) | localFlow(exprNode(modified), exprNode(mid))) + exists(Expr mid | mayWriteToCollection(mid) | localExprFlow(modified, mid)) or exists(MethodCall mid, Callable c | mayWriteToCollection(mid) | mid.getTarget() = c and diff --git a/csharp/ql/src/Dead Code/DeadStoreOfLocal.ql b/csharp/ql/src/Dead Code/DeadStoreOfLocal.ql index 74fa9b28c0e1..ac6b258f0be6 100644 --- a/csharp/ql/src/Dead Code/DeadStoreOfLocal.ql +++ b/csharp/ql/src/Dead Code/DeadStoreOfLocal.ql @@ -62,7 +62,7 @@ predicate nonEscapingCall(Call c) { predicate mayEscape(LocalVariable v) { exists(Callable c, Expr e, Expr succ | c = getACapturingCallableAncestor(v) | e = getADelegateExpr(c) and - DataFlow::localFlow(DataFlow::exprNode(e), DataFlow::exprNode(succ)) and + DataFlow::localExprFlow(e, succ) and not succ = any(DelegateCall dc).getDelegateExpr() and not succ = any(Cast cast).getExpr() and not succ = any(Call call | nonEscapingCall(call)).getAnArgument() and diff --git a/csharp/ql/src/Security Features/CWE-119/LocalUnvalidatedArithmetic.ql b/csharp/ql/src/Security Features/CWE-119/LocalUnvalidatedArithmetic.ql index 9dca3ce97810..9f88c2a4eacc 100644 --- a/csharp/ql/src/Security Features/CWE-119/LocalUnvalidatedArithmetic.ql +++ b/csharp/ql/src/Security Features/CWE-119/LocalUnvalidatedArithmetic.ql @@ -22,11 +22,9 @@ where // `add` is performing pointer arithmetic add.getType() instanceof PointerType and // one of the operands comes, in zero or more steps, from a virtual method call - DataFlow::localFlow(DataFlow::exprNode(taintSrc), DataFlow::exprNode(add.getAnOperand())) and + DataFlow::localExprFlow(taintSrc, add.getAnOperand()) and // virtual method call result has not been validated - not exists(Expr check, ComparisonOperation cmp | - DataFlow::localFlow(DataFlow::exprNode(taintSrc), DataFlow::exprNode(check)) - | + not exists(Expr check, ComparisonOperation cmp | DataFlow::localExprFlow(taintSrc, check) | cmp.getAnOperand() = check and add.getAnOperand().(GuardedExpr).isGuardedBy(cmp, check, _) ) diff --git a/csharp/ql/src/semmle/code/csharp/Property.qll b/csharp/ql/src/semmle/code/csharp/Property.qll index d1f15e1152af..d7013927846d 100644 --- a/csharp/ql/src/semmle/code/csharp/Property.qll +++ b/csharp/ql/src/semmle/code/csharp/Property.qll @@ -520,7 +520,7 @@ class IndexerProperty extends Property { pragma[nomagic] private IndexerCall getAnIndexerCall0() { exists(Expr qualifier | qualifier = result.getQualifier() | - DataFlow::localFlow(DataFlow::exprNode(this.getAnAccess()), DataFlow::exprNode(qualifier)) + DataFlow::localExprFlow(this.getAnAccess(), qualifier) ) } diff --git a/csharp/ql/src/semmle/code/csharp/dataflow/internal/DataFlowPublic.qll b/csharp/ql/src/semmle/code/csharp/dataflow/internal/DataFlowPublic.qll index 46680761e354..4b28170e07ac 100644 --- a/csharp/ql/src/semmle/code/csharp/dataflow/internal/DataFlowPublic.qll +++ b/csharp/ql/src/semmle/code/csharp/dataflow/internal/DataFlowPublic.qll @@ -164,6 +164,12 @@ predicate localFlowStep(Node nodeFrom, Node nodeTo) { */ predicate localFlow(Node source, Node sink) { localFlowStep*(source, sink) } +/** + * Holds if data can flow from `e1` to `e2` in zero or more + * local (intra-procedural) steps. + */ +predicate localExprFlow(Expr e1, Expr e2) { localFlow(exprNode(e1), exprNode(e2)) } + /** * A data flow node that jumps between callables. This can be extended in * framework code to add additional data flow steps. diff --git a/csharp/ql/src/semmle/code/csharp/dataflow/internal/TaintTrackingPublic.qll b/csharp/ql/src/semmle/code/csharp/dataflow/internal/TaintTrackingPublic.qll index 165172be2a6a..eda33f2fcd90 100755 --- a/csharp/ql/src/semmle/code/csharp/dataflow/internal/TaintTrackingPublic.qll +++ b/csharp/ql/src/semmle/code/csharp/dataflow/internal/TaintTrackingPublic.qll @@ -7,6 +7,14 @@ private import TaintTrackingPrivate */ predicate localTaint(DataFlow::Node source, DataFlow::Node sink) { localTaintStep*(source, sink) } +/** + * Holds if taint can flow from `e1` to `e2` in zero or more + * local (intra-procedural) steps. + */ +predicate localExprTaint(Expr e1, Expr e2) { + localTaint(DataFlow::exprNode(e1), DataFlow::exprNode(e2)) +} + /** A member (property or field) that is tainted if its containing object is tainted. */ abstract class TaintedMember extends AssignableMember { } diff --git a/csharp/ql/src/semmle/code/csharp/frameworks/system/Xml.qll b/csharp/ql/src/semmle/code/csharp/frameworks/system/Xml.qll index 38754f4251ab..eee5ef27ae15 100644 --- a/csharp/ql/src/semmle/code/csharp/frameworks/system/Xml.qll +++ b/csharp/ql/src/semmle/code/csharp/frameworks/system/Xml.qll @@ -156,7 +156,7 @@ class XmlReaderSettingsCreation extends ObjectCreation { p = this.getType().(RefType).getAProperty() and exists(PropertyCall set, Expr arg | set.getTarget() = p.getSetter() and - DataFlow::localFlow(DataFlow::exprNode(this), DataFlow::exprNode(set.getQualifier())) and + DataFlow::localExprFlow(this, set.getQualifier()) and arg = set.getAnArgument() and result = getBitwiseOrOperand*(arg) ) diff --git a/csharp/ql/src/semmle/code/csharp/frameworks/system/text/RegularExpressions.qll b/csharp/ql/src/semmle/code/csharp/frameworks/system/text/RegularExpressions.qll index 0914499fd0d2..ce03f5275d9d 100644 --- a/csharp/ql/src/semmle/code/csharp/frameworks/system/text/RegularExpressions.qll +++ b/csharp/ql/src/semmle/code/csharp/frameworks/system/text/RegularExpressions.qll @@ -71,7 +71,7 @@ class RegexOperation extends Call { | // e.g. `new Regex(...).Match(...)` // or `var r = new Regex(...); r.Match(...)` - DataFlow::localFlow(DataFlow::exprNode(this), DataFlow::exprNode(call.getQualifier())) + DataFlow::localExprFlow(this, call.getQualifier()) or // e.g. `private string r = new Regex(...); public void foo() { r.Match(...); }` call.getQualifier().(FieldAccess).getTarget().getInitializer() = this diff --git a/csharp/ql/src/semmle/code/csharp/security/dataflow/ZipSlip.qll b/csharp/ql/src/semmle/code/csharp/security/dataflow/ZipSlip.qll index 30531b57f9eb..c2785173be4d 100644 --- a/csharp/ql/src/semmle/code/csharp/security/dataflow/ZipSlip.qll +++ b/csharp/ql/src/semmle/code/csharp/security/dataflow/ZipSlip.qll @@ -142,7 +142,7 @@ module ZipSlip { // not yet been resolved. not exists(MethodCall combineCall | combineCall.getTarget().hasQualifiedName("System.IO.Path", "Combine") and - DataFlow::localFlow(DataFlow::exprNode(combineCall), DataFlow::exprNode(q)) + DataFlow::localExprFlow(combineCall, q) ) } diff --git a/csharp/ql/src/semmle/code/csharp/security/xml/InsecureXML.qll b/csharp/ql/src/semmle/code/csharp/security/xml/InsecureXML.qll index e989f56e65f5..c9f738ef9977 100644 --- a/csharp/ql/src/semmle/code/csharp/security/xml/InsecureXML.qll +++ b/csharp/ql/src/semmle/code/csharp/security/xml/InsecureXML.qll @@ -87,8 +87,7 @@ module InsecureXML { or // values set on var that create is assigned to exists(Assignment propAssign | - DataFlow::localFlow(DataFlow::exprNode(create), - DataFlow::exprNode(propAssign.getLValue().(PropertyAccess).getQualifier())) and + DataFlow::localExprFlow(create, propAssign.getLValue().(PropertyAccess).getQualifier()) and propAssign.getLValue().(PropertyAccess).getTarget().hasName(prop) and result = propAssign.getRValue() ) @@ -253,9 +252,7 @@ module InsecureXML { } override predicate isUnsafe(string reason) { - exists(ObjectCreation creation | - DataFlow::localFlow(DataFlow::exprNode(creation), DataFlow::exprNode(this.getQualifier())) - | + exists(ObjectCreation creation | DataFlow::localExprFlow(creation, this.getQualifier()) | not exists(Expr xmlResolverVal | isSafeXmlResolver(xmlResolverVal) and xmlResolverVal = getAValueForProp(creation, "XmlResolver")