Unified: Support property source/sink models in LegacyMaD

unified/ql/lib/codeql/unified/internal/mad/LegacyMaD.qll

@@ -4,6 +4,7 @@
 
 private import unified
 private import codeql.dataflow.internal.AccessPathSyntax
+private import codeql.unified.internal.typeinference.TypeInference as T
 
 extensible predicate legacySourceModel(
   string namespace, string type, boolean subtypes, string name, string signature, string ext,
@@ -29,7 +30,6 @@ private predicate modelContainsMethodName(string name) {
 }
 
 private predicate parsedRawMethodName(string rawName, string name, string argLabels) {
-  // NOTE: We only support method names at the moment, not property names like `String.count`.
   modelContainsMethodName(rawName) and
   exists(string regex |
     regex = "([^(]+)\\((.*)\\)" and
@@ -123,8 +123,34 @@ private predicate callableSelector(
   )
 }
 
+/** Holds if `name` from a model refers to a property, such as `count` in `String.count`. */
+private predicate isRawPropertyName(string name) {
+  modelContainsMethodName(name) and
+  not name.matches("%(%")
+}
+
+/** Holds if `cls` is named `type` or, if `subtypes` is true, is a subclass of `type`. */
+bindingset[cls, type]
+private predicate classMatchesType(ClassLikeDeclaration cls, string type, boolean subtypes) {
+  cls.getQualifiedName() = type and subtypes = [true, false]
+  or
+  isSubclassOfType(cls, type) and subtypes = true
+}
+
+/**
+ * Holds if `access` may be an access to the property `name`, and its base has the inferred
+ * type `cls`.
+ */
+pragma[nomagic]
+private predicate propertyAccessSelector(MemberAccess access, ClassLikeDeclaration cls, string name) {
+  name = access.getMemberName() and
+  name = any(Selector s).getPropertyName() and
+  not access = any(CallExpr call).getCallee() and
+  cls = T::inferBaseType(access).asClass()
+}
+
 private newtype TSelector =
-  MkSelector(string type, boolean subtypes, string name, string argLabels, string argTypes) {
+  MkCallSelector(string type, boolean subtypes, string name, string argLabels, string argTypes) {
     exists(string rawName |
       legacySourceModel(_, type, subtypes, rawName, argTypes, _, _, _, _)
       or
@@ -134,21 +160,38 @@ private newtype TSelector =
     |
       parsedRawMethodName(rawName, name, argLabels)
     )
+  } or
+  MkPropertySelector(string type, boolean subtypes, string name) {
+    (
+      legacySourceModel(_, type, subtypes, name, _, _, _, _, _)
+      or
+      legacySinkModel(_, type, subtypes, name, _, _, _, _, _)
+    ) and
+    isRawPropertyName(name)
   }
 
 /**
- * A tuple from a model that identifies call sites.
+ * A tuple from a model that identifies call sites, callables, or property accesses.
  */
 class Selector extends TSelector {
-  string getTypeString() { this = MkSelector(result, _, _, _, _) }
+  string getTypeString() {
+    this = MkCallSelector(result, _, _, _, _) or this = MkPropertySelector(result, _, _)
+  }
+
+  boolean getSubtypesFlag() {
+    this = MkCallSelector(_, result, _, _, _) or this = MkPropertySelector(_, result, _)
+  }
 
-  boolean getSubtypesFlag() { this = MkSelector(_, result, _, _, _) }
+  string getName() {
+    this = MkCallSelector(_, _, result, _, _) or this = MkPropertySelector(_, _, result)
+  }
 
-  string getName() { this = MkSelector(_, _, result, _, _) }
+  /** Gets the property name, if this is a property selector. */
+  string getPropertyName() { this = MkPropertySelector(_, _, result) }
 
-  string getArgLabels() { this = MkSelector(_, _, _, result, _) }
+  string getArgLabels() { this = MkCallSelector(_, _, _, result, _) }
 
-  string getArgTypes() { this = MkSelector(_, _, _, _, result) }
+  string getArgTypes() { this = MkCallSelector(_, _, _, _, result) }
 
   private string ppSubtypes() {
     if this.getSubtypesFlag() = true then result = "+" else result = ""
@@ -158,19 +201,21 @@ class Selector extends TSelector {
     result =
       this.getTypeString() + this.ppSubtypes() + "." + this.getName() + "(" + this.getArgLabels() +
         ")" + this.getArgTypes()
+    or
+    result = this.getTypeString() + this.ppSubtypes() + "." + this.getPropertyName()
   }
 
   /** Holds if `name,argLabels` should be used to join with `methodCallSelector`. */
   pragma[nomagic]
   private predicate matchesMethodCallSelector(string name, string argLabels) {
-    this = MkSelector(_, _, name, argLabels, _) and
+    this = MkCallSelector(_, _, name, argLabels, _) and
     name != "init"
   }
 
   /** Holds if `name,argLabels` should be used to join with `constructorCallSelector`. */
   pragma[nomagic]
   private predicate matchesConstructorCallSelector(string name, string argLabels) {
-    this = MkSelector(name, _, "init", argLabels, _)
+    this = MkCallSelector(name, _, "init", argLabels, _)
   }
 
   predicate matchesCall(CallExpr call) {
@@ -186,7 +231,19 @@ class Selector extends TSelector {
   predicate matchesCallable(Callable callable) {
     exists(string type, string name, string argLabels |
       callableSelector(callable, type, name, argLabels) and
-      this = MkSelector(type, true, name, argLabels, _)
+      this = MkCallSelector(type, true, name, argLabels, _)
+    )
+  }
+
+  /**
+   * Holds if `access` is an access to the property identified by this selector, as determined
+   * by the inferred type of its base.
+   */
+  predicate matchesPropertyAccess(MemberAccess access) {
+    exists(ClassLikeDeclaration cls, string type, boolean subtypes, string name |
+      this = MkPropertySelector(type, subtypes, name) and
+      propertyAccessSelector(access, cls, name) and
+      classMatchesType(cls, type, subtypes)
     )
   }
 
@@ -267,27 +324,57 @@ private DataFlow::Node getSourceFromCallable(Callable callable, AccessPathToken
   result.isReceiverParameter(callable)
 }
 
+/**
+ * Gets the source from the property `access` specified by `path`.
+ */
+bindingset[access, path]
+private DataFlow::Node getSourceFromPropertyAccess(MemberAccess access, AccessPath path) {
+  path = "" and
+  result.isResultValue(access)
+}
+
+/**
+ * Gets the sink from the property `access` specified by `path`.
+ */
+bindingset[access, path]
+private DataFlow::Node getSinkFromPropertyAccess(MemberAccess access, AccessPath path) {
+  path = "PostUpdate" and
+  (
+    // The value being assigned to the property, as in `x.prop = value`
+    result.isIncomingValue(access)
+    or
+    // The property value after being mutated in-place, as in `x.prop.append(value)`
+    result.isPostUpdate(access)
+  )
+}
+
 private predicate normalizedSourceModel(
   Selector selector, AccessPath output, string kind, string provenance
 ) {
-  exists(
-    string type, boolean subtypes, string rawName, string name, string argLabels, string argTypes
+  exists(string type, boolean subtypes, string rawName, string argTypes |
+    legacySourceModel(_, type, subtypes, rawName, argTypes, _, output, kind, provenance)
   |
-    legacySourceModel(_, type, subtypes, rawName, argTypes, _, output, kind, provenance) and
-    parsedRawMethodName(rawName, name, argLabels) and
-    selector = MkSelector(type, subtypes, name, argLabels, argTypes)
+    exists(string name, string argLabels |
+      parsedRawMethodName(rawName, name, argLabels) and
+      selector = MkCallSelector(type, subtypes, name, argLabels, argTypes)
+    )
+    or
+    selector = MkPropertySelector(type, subtypes, rawName)
   )
 }
 
 private predicate normalizedSinkModel(
   Selector selector, AccessPath output, string kind, string provenance
 ) {
-  exists(
-    string type, boolean subtypes, string rawName, string name, string argLabels, string argTypes
+  exists(string type, boolean subtypes, string rawName, string argTypes |
+    legacySinkModel(_, type, subtypes, rawName, argTypes, _, output, kind, provenance)
   |
-    legacySinkModel(_, type, subtypes, rawName, argTypes, _, output, kind, provenance) and
-    parsedRawMethodName(rawName, name, argLabels) and
-    selector = MkSelector(type, subtypes, name, argLabels, argTypes)
+    exists(string name, string argLabels |
+      parsedRawMethodName(rawName, name, argLabels) and
+      selector = MkCallSelector(type, subtypes, name, argLabels, argTypes)
+    )
+    or
+    selector = MkPropertySelector(type, subtypes, rawName)
   )
 }
 
@@ -299,7 +386,7 @@ predicate normalizedSummaryModel(
   |
     legacySummaryModel(_, type, subtypes, rawName, argTypes, _, input, output, kind, provenance) and
     parsedRawMethodName(rawName, name, argLabels) and
-    selector = MkSelector(type, subtypes, name, argLabels, argTypes)
+    selector = MkCallSelector(type, subtypes, name, argLabels, argTypes)
   )
 }
 
@@ -319,6 +406,11 @@ DataFlow::Node getASource(string kind, string provenance) {
       selector.matchesCallable(callable) and
       result = getSourceFromCallable(callable, output)
     )
+    or
+    exists(MemberAccess access |
+      selector.matchesPropertyAccess(access) and
+      result = getSourceFromPropertyAccess(access, output)
+    )
   )
 }
 
@@ -338,6 +430,11 @@ DataFlow::Node getASink(string kind, string provenance) {
       selector.matchesCallable(callable) and
       result = getSinkFromCallable(callable, input)
     )
+    or
+    exists(MemberAccess access |
+      selector.matchesPropertyAccess(access) and
+      result = getSinkFromPropertyAccess(access, input)
+    )
   )
 }
 

unified/ql/lib/codeql/unified/internal/typeinference/TypeInference.qll

@@ -331,14 +331,19 @@ module Input3 implements InputSig3 {
     }
   }
 
-  private Type getImplicitReceiverType(UnqualifiedMemberAccess uma, TypePath path) {
-    exists(Parameter p, Unified::Callable c |
-      p.getParameterEx().isImplicitReceiverParameter(c) and
-      c = uma.getImplicitQualifierVariable().getDeclaringCallable() and
-      result = p.getType().getTypeAt(path)
+  additional Type inferTypeFromVariable(LocalVariable v, TypePath path) {
+    result = inferType(v.getABinding(), path)
+    or
+    exists(Parameter param |
+      implicitParameterDecl(param, v) and
+      result = param.getType().getTypeAt(path)
     )
   }
 
+  private Type getImplicitReceiverType(UnqualifiedMemberAccess uma, TypePath path) {
+    result = inferTypeFromVariable(uma.getImplicitQualifierVariable(), path)
+  }
+
   private class UnqualifiedMemberAccessFieldAccess extends FieldAccess, UnqualifiedMemberAccess {
     override Expr getReceiver() { none() }
 
@@ -862,6 +867,16 @@ predicate inferTypeUnique = M3::inferTypeUnique/2;
 
 predicate inferTypeCertain = M3::inferTypeCertain/2;
 
+Type inferVariableType(LocalVariable variable) {
+  result = Input3::inferTypeFromVariable(variable, _)
+}
+
+Type inferBaseType(MemberAccess access) {
+  result = inferType(access.(MemberAccessExpr).getBase())
+  or
+  result = inferVariableType(access.(UnqualifiedMemberAccess).getImplicitQualifierVariable())
+}
+
 module Consistency = M3::Consistency;
 
 module CachedStage = M3::CachedStage;

unified/ql/src/queries/security/CWE-078/CommandInjection.ql

@@ -20,10 +20,7 @@ import unified
 module CommandInjectionConfig implements DataFlow::ConfigSig {
   predicate isSource(DataFlow::Node node) { Models::isSource(node, _) }
 
-  predicate isSink(DataFlow::Node node) {
-    // TODO: Support Process property-write sinks from original Swift's CommandInjectionExtensions.qll.
-    Models::isSink(node, "command-injection")
-  }
+  predicate isSink(DataFlow::Node node) { Models::isSink(node, "command-injection") }
 
   predicate isBarrier(DataFlow::Node node) {
     // TODO: Port the numeric-type barrier from original Swift's CommandInjectionExtensions.qll.

unified/ql/test/library-tests/mad-builtins/test.expected

@@ -0,0 +1,9 @@
+isSink
+| test.swift:15:5:15:52 | ... .tlsMinimumSupportedProtocolVersion | tls-protocol-version |
+| test.swift:17:5:17:22 | ... .launchPath | command-injection |
+| test.swift:18:5:18:21 | ... .arguments | command-injection |
+| test.swift:19:5:19:23 | ... .arguments | command-injection |
+| test.swift:22:5:22:26 | ... .arguments | command-injection |
+isSource
+| test.swift:13:9:13:22 | ... .text | local |
+missingBuiltinType

unified/ql/test/library-tests/mad-builtins/test.qlref

@@ -0,0 +1,2 @@
+query: library-tests/mad/test.ql
+postprocess: utils/test/InlineExpectationsTestQuery.ql

unified/ql/test/library-tests/mad-builtins/test.swift

@@ -0,0 +1,23 @@
+// No stubs are declared in this file, so the types below must be resolved via the builtins,
+// as is the case when analyzing real code whose dependencies are not extracted.
+
+class MyProcess: Process {}
+
+func testPropertiesWithoutStubs(
+    string: String,
+    textField: UITextField,
+    configuration: URLSessionConfiguration,
+    process: Process,
+    myProcess: MyProcess
+) {
+    _ = textField.text  // $ isSource=local
+
+    configuration.tlsMinimumSupportedProtocolVersion = 0  // $ isSink=tls-protocol-version
+
+    process.launchPath = string  // $ isSink=command-injection
+    process.arguments = [string]  // $ isSink=command-injection
+    myProcess.arguments = [string]  // $ isSink=command-injection
+
+    let localProcess = Process()
+    localProcess.arguments = [string]  // $ isSink=command-injection
+}

unified/ql/test/library-tests/mad/test.expected

@@ -6,16 +6,28 @@ isSink
 | test.swift:21:5:21:16 | [self arg] ... .md5(...) | weak-hash-input-MD5 |
 | test.swift:23:20:23:25 | string | regex-use |
 | test.swift:24:20:24:25 | string | regex-use |
-| test.swift:71:37:71:49 | encodedOffset | string-length |
-| test.swift:72:42:72:54 | encodedOffset | string-length |
-| test.swift:76:24:76:36 | encryptionKey | encryption-key |
-| test.swift:77:18:77:24 | fileURL | path-injection |
-| test.swift:88:24:88:36 | encryptionKey | encryption-key |
-| test.swift:89:18:89:24 | fileURL | path-injection |
-| test.swift:100:24:100:36 | encryptionKey | encryption-key |
-| test.swift:101:18:101:24 | fileURL | path-injection |
-| test.swift:107:23:107:34 | seedFilePath | path-injection |
-| test.swift:119:29:119:34 | string | path-injection |
+| test.swift:55:5:55:45 | ... .tlsMaximumSupportedProtocol | tls-protocol-version |
+| test.swift:56:5:56:45 | ... .tlsMinimumSupportedProtocol | tls-protocol-version |
+| test.swift:57:5:57:52 | ... .tlsMaximumSupportedProtocolVersion | tls-protocol-version |
+| test.swift:58:5:58:52 | ... .tlsMinimumSupportedProtocolVersion | tls-protocol-version |
+| test.swift:93:5:93:22 | ... .launchPath | command-injection |
+| test.swift:94:5:94:21 | ... .arguments | command-injection |
+| test.swift:95:5:95:21 | [post] ... .arguments | command-injection |
+| test.swift:96:5:96:23 | ... .arguments | command-injection |
+| test.swift:123:37:123:49 | encodedOffset | string-length |
+| test.swift:124:42:124:54 | encodedOffset | string-length |
+| test.swift:128:24:128:36 | encryptionKey | encryption-key |
+| test.swift:129:18:129:24 | fileURL | path-injection |
+| test.swift:140:24:140:36 | encryptionKey | encryption-key |
+| test.swift:141:18:141:24 | fileURL | path-injection |
+| test.swift:152:24:152:36 | encryptionKey | encryption-key |
+| test.swift:153:18:153:24 | fileURL | path-injection |
+| test.swift:159:23:159:34 | seedFilePath | path-injection |
+| test.swift:171:29:171:34 | string | path-injection |
+| test.swift:183:9:183:18 | launchPath | command-injection |
+| test.swift:184:9:184:17 | arguments | command-injection |
+| test.swift:185:9:185:17 | [post] arguments | command-injection |
+| test.swift:191:9:191:18 | launchPath | command-injection |
 isSource
 | test.swift:4:5:4:27 | String(...) | remote |
 | test.swift:5:5:5:44 | String(...) | remote |
@@ -25,3 +37,9 @@ isSource
 | test.swift:10:5:12:24 | String(...) | local |
 | test.swift:13:5:15:28 | String(...) | local |
 | test.swift:29:29:29:32 | text | local |
+| test.swift:52:9:52:20 | ... .count | string-length |
+| test.swift:53:9:53:22 | ... .text | local |
+| test.swift:89:9:89:24 | ... .text | local |
+| test.swift:176:13:176:16 | text | local |
+| test.swift:177:13:177:21 | ... .text | local |
+missingBuiltinType