Improve PowerShell deserialization precision

powershell/ql/lib/semmle/code/powershell/security/UnsafeDeserializationCustomizations.qll

@@ -42,14 +42,16 @@ module UnsafeDeserialization {
    * (lowercase) matches `fullTypeName`. Handles both `New-Object TypeName` and
    * `[TypeName]::new()` patterns.
    */
-  private predicate objectCreationMatchesType(
-    DataFlow::ObjectCreationNode ocn, string fullTypeName
-  ) {
+  private predicate objectCreationMatchesType(DataFlow::ObjectCreationNode ocn, string fullTypeName) {
     // New-Object TypeName: getLowerCaseConstructedTypeName() returns the full qualified name
     ocn.getLowerCaseConstructedTypeName() = fullTypeName
     or
     // [TypeName]::new(): access the qualifier TypeNameExpr for the full qualified name
-    ocn.getExprNode().getExpr().(ConstructorCall).getQualifier().(TypeNameExpr)
+    ocn.getExprNode()
+        .getExpr()
+        .(ConstructorCall)
+        .getQualifier()
+        .(TypeNameExpr)
         .getPossiblyQualifiedName() = fullTypeName
   }
 
@@ -106,20 +108,28 @@ module UnsafeDeserialization {
   }
 
   /**
-   * An argument to a BinaryFormatter deserialization method call, including
-   * Deserialize, UnsafeDeserialize, and UnsafeDeserializeMethodResponse.
+   * A BinaryFormatter deserialization method call, including Deserialize, UnsafeDeserialize,
+   * and UnsafeDeserializeMethodResponse.
    */
-  class BinaryFormatterDeserializeSink extends Sink {
-    BinaryFormatterDeserializeSink() {
-      exists(DataFlow::ObjectCreationNode ocn, DataFlow::CallNode cn |
-        cn.getQualifier().getALocalSource() = ocn and
+  class BinaryFormatterDeserializeCall extends DataFlow::CallNode {
+    BinaryFormatterDeserializeCall() {
+      exists(DataFlow::ObjectCreationNode ocn |
+        this.getQualifier().getALocalSource() = ocn and
         objectCreationMatchesType(ocn,
           "system.runtime.serialization.formatters.binary.binaryformatter") and
-        cn.getLowerCaseName() =
-          ["deserialize", "unsafedeserialize", "unsafedeserializemethodresponse"] and
-        cn.getAnArgument() = this
+        this.getLowerCaseName() =
+          ["deserialize", "unsafedeserialize", "unsafedeserializemethodresponse"]
       )
     }
+  }
+
+  /**
+   * An input argument to a BinaryFormatter deserialization method call.
+   */
+  class BinaryFormatterDeserializeSink extends Sink {
+    BinaryFormatterDeserializeSink() {
+      exists(BinaryFormatterDeserializeCall cn | cn.getAnArgument() = this)
+    }
 
     override string getSinkType() { result = "call to BinaryFormatter.Deserialize" }
   }
@@ -167,9 +177,7 @@ module UnsafeDeserialization {
       )
     }
 
-    override string getSinkType() {
-      result = "call to [" + typeName + "]::" + methodName
-    }
+    override string getSinkType() { result = "call to [" + typeName + "]::" + methodName }
   }
 
   /**

powershell/ql/lib/semmle/code/powershell/security/UnsafeDeserializationQuery.qll

@@ -17,12 +17,40 @@ module Config implements DataFlow::ConfigSig {
   predicate isSource(DataFlow::Node source) { source instanceof Source }
 
   predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
-  predicate isAdditionalFlowStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo){
-    exists(InvokeMemberExpr ime | 
-        nodeTo.asExpr().getExpr() = ime and 
-        nodeFrom.asExpr().getExpr() = ime.getAnArgument()
+
+  predicate isBarrier(DataFlow::Node node) { node instanceof Sanitizer }
+
+  private predicate isConvertType(TypeNameExpr type) {
+    type.getPossiblyQualifiedName() = ["system.convert", "convert"]
+  }
+
+  private predicate isReaderOrStreamType(TypeNameExpr type) {
+    type.getPossiblyQualifiedName() =
+      ["system.io.memorystream", "memorystream", "system.io.stringreader", "stringreader"]
+  }
+
+  private predicate isDeserializationInputTransform(InvokeMemberExpr ime) {
+    exists(TypeNameExpr type |
+      ime.getQualifier() = type and
+      (
+        isConvertType(type) and
+        ime.getLowerCaseName() = "frombase64string"
+        or
+        isReaderOrStreamType(type) and
+        ime.getLowerCaseName() = "new"
+      )
+    )
+    or
+    ime.getLowerCaseName() = "getbytes"
+  }
+
+  predicate isAdditionalFlowStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
+    exists(InvokeMemberExpr ime |
+      isDeserializationInputTransform(ime) and
+      nodeTo.asExpr().getExpr() = ime and
+      nodeFrom.asExpr().getExpr() = ime.getAnArgument()
     )
   }
 }
 
-module UnsafeDeserializationFlow = TaintTracking::Global<Config>; 
+module UnsafeDeserializationFlow = TaintTracking::Global<Config>;

powershell/ql/src/queries/security/cwe-502/BinaryFormatterDeserialization.qhelp

@@ -4,19 +4,28 @@
 <qhelp>
 <overview>
 
-<p>Using <code>BinaryFormatter</code> to deserialize an object from untrusted input may result in security problems, such
-as denial of service or remote code execution.</p>
+<p><code>BinaryFormatter</code> is unsafe for deserialization and should not be used, even when
+the data is produced locally. This query is an API-use warning: it reports calls to
+<code>BinaryFormatter</code> deserialization methods without proving that the serialized data is
+attacker-controlled.</p>
+
+<p>When attacker-controlled data is known to flow into a deserializer, the
+<code>powershell/microsoft/public/unsafe-deserialization</code> query reports that higher-confidence
+data-flow result.</p>
 
 </overview>
 <recommendation>
 
-<p>Avoid using <code>BinaryFormatter</code>.</p>
+<p>Avoid using <code>BinaryFormatter</code>. Prefer serializers that do not permit arbitrary type
+instantiation, and validate or authenticate any serialized data before deserializing it.</p>
 
 </recommendation>
 <example>
 
 <p>In this example, a string is deserialized using a
-<code>BinaryFormatter</code>. <code>BinaryFormatter</code> is an easily exploited deserializer.</p>
+<code>BinaryFormatter</code>. <code>BinaryFormatter</code> is an easily exploited deserializer. If the
+string is attacker-controlled, this is also reported by the taint-tracking unsafe
+deserialization query.</p>
 
 <sample src="examples/BinaryFormatterDeserialization.ps1" />
 

powershell/ql/src/queries/security/cwe-502/BinaryFormatterDeserialization.ql

@@ -1,9 +1,9 @@
 /**
- * @name Use of Binary Formatter deserialization
- * @description Use of Binary Formatter is unsafe
+ * @name Use of BinaryFormatter deserialization API
+ * @description BinaryFormatter deserialization is unsafe even when the input is trusted.
  * @kind problem
- * @problem.severity error
- * @security-severity 8.8
+ * @problem.severity warning
+ * @security-severity 7.5
  * @precision high
  * @id powershell/microsoft/public/binary-formatter-deserialization
  * @tags correctness
@@ -14,5 +14,6 @@
 import powershell
 import semmle.code.powershell.security.UnsafeDeserializationCustomizations::UnsafeDeserialization
 
-from BinaryFormatterDeserializeSink sink
-select sink, "Call to BinaryFormatter.Deserialize"
+from BinaryFormatterDeserializeCall call
+select call,
+  "This call uses BinaryFormatter deserialization. BinaryFormatter is unsafe; if this data can be attacker-controlled, the unsafe-deserialization query reports the data flow."

powershell/ql/src/queries/security/cwe-502/UnsafeDeserialization.qhelp

@@ -4,13 +4,21 @@
 <qhelp>
 <overview>
 
-<p>Deserializing an object from untrusted input may result in security problems, such
-as denial of service or remote code execution.</p>
+<p>Deserializing attacker-controlled data with an unsafe deserializer may result in
+security problems, such as denial of service or remote code execution. This query tracks
+untrusted data flow into unsafe deserialization APIs.</p>
+
+<p>Local, self-produced serialized data without any untrusted source is not reported by
+this query. The separate <code>powershell/microsoft/public/binary-formatter-deserialization</code>
+query reports <code>BinaryFormatter</code> API use even when no attacker-controlled data flow is
+known.</p>
 
 </overview>
 <recommendation>
 
-<p>Avoid using an unsafe deserialization framework.</p>
+<p>Avoid using unsafe deserialization frameworks for attacker-controlled data. Prefer safe
+serializers, and validate, authenticate, or otherwise constrain serialized data before
+deserializing it.</p>
 
 </recommendation>
 <example>

powershell/ql/test/query-tests/security/cwe-502/UnsafeDeserialization.expected

@@ -16,6 +16,8 @@
 | test.ps1:76:49:76:57 | stream15 | test.ps1:74:12:74:42 | Call to read-host | test.ps1:76:49:76:57 | stream15 | This unsafe deserializer deserializes  on a $@. | test.ps1:74:12:74:42 | Call to read-host | read from stdin |
 | test.ps1:81:40:81:47 | input16 | test.ps1:79:12:79:33 | Call to read-host | test.ps1:81:40:81:47 | input16 | This unsafe deserializer deserializes  on a $@. | test.ps1:79:12:79:33 | Call to read-host | read from stdin |
 | test.ps1:86:48:86:55 | bytes17 | test.ps1:84:12:84:40 | Call to read-host | test.ps1:86:48:86:55 | bytes17 | This unsafe deserializer deserializes  on a $@. | test.ps1:84:12:84:40 | Call to read-host | read from stdin |
+| test.ps1:98:39:98:49 | fileStream | test.ps1:96:14:96:58 | Call to readallbytes | test.ps1:98:39:98:49 | fileStream | This unsafe deserializer deserializes  on a $@. | test.ps1:96:14:96:58 | Call to readallbytes | file stream |
+| test.ps1:103:41:103:53 | remoteStream | test.ps1:101:16:101:92 | Call to downloaddata | test.ps1:103:41:103:53 | remoteStream | This unsafe deserializer deserializes  on a $@. | test.ps1:101:16:101:92 | Call to downloaddata | remote flow source |
 edges
 | test.ps1:2:1:2:16 | untrustedBase64 | test.ps1:4:69:4:84 | untrustedBase64 | provenance |  |
 | test.ps1:2:20:2:47 | Call to read-host | test.ps1:2:1:2:16 | untrustedBase64 | provenance | Src:MaD:0  |
@@ -85,6 +87,16 @@ edges
 | test.ps1:85:1:85:8 | bytes17 | test.ps1:86:48:86:55 | bytes17 | provenance |  |
 | test.ps1:85:12:85:48 | Call to frombase64string | test.ps1:85:1:85:8 | bytes17 | provenance |  |
 | test.ps1:85:40:85:47 | input17 | test.ps1:85:12:85:48 | Call to frombase64string | provenance | Config |
+| test.ps1:96:1:96:10 | fileBytes | test.ps1:97:45:97:54 | fileBytes | provenance |  |
+| test.ps1:96:14:96:58 | Call to readallbytes | test.ps1:96:1:96:10 | fileBytes | provenance | Src:MaD:83  |
+| test.ps1:97:1:97:11 | fileStream | test.ps1:98:39:98:49 | fileStream | provenance |  |
+| test.ps1:97:15:97:55 | Call to new | test.ps1:97:1:97:11 | fileStream | provenance |  |
+| test.ps1:97:45:97:54 | fileBytes | test.ps1:97:15:97:55 | Call to new | provenance | Config |
+| test.ps1:101:1:101:12 | remoteBytes | test.ps1:102:47:102:58 | remoteBytes | provenance |  |
+| test.ps1:101:16:101:92 | Call to downloaddata | test.ps1:101:1:101:12 | remoteBytes | provenance | Src:MaD:114  |
+| test.ps1:102:1:102:13 | remoteStream | test.ps1:103:41:103:53 | remoteStream | provenance |  |
+| test.ps1:102:17:102:59 | Call to new | test.ps1:102:1:102:13 | remoteStream | provenance |  |
+| test.ps1:102:47:102:58 | remoteBytes | test.ps1:102:17:102:59 | Call to new | provenance | Config |
 nodes
 | test.ps1:2:1:2:16 | untrustedBase64 | semmle.label | untrustedBase64 |
 | test.ps1:2:20:2:47 | Call to read-host | semmle.label | Call to read-host |
@@ -171,4 +183,16 @@ nodes
 | test.ps1:85:12:85:48 | Call to frombase64string | semmle.label | Call to frombase64string |
 | test.ps1:85:40:85:47 | input17 | semmle.label | input17 |
 | test.ps1:86:48:86:55 | bytes17 | semmle.label | bytes17 |
+| test.ps1:96:1:96:10 | fileBytes | semmle.label | fileBytes |
+| test.ps1:96:14:96:58 | Call to readallbytes | semmle.label | Call to readallbytes |
+| test.ps1:97:1:97:11 | fileStream | semmle.label | fileStream |
+| test.ps1:97:15:97:55 | Call to new | semmle.label | Call to new |
+| test.ps1:97:45:97:54 | fileBytes | semmle.label | fileBytes |
+| test.ps1:98:39:98:49 | fileStream | semmle.label | fileStream |
+| test.ps1:101:1:101:12 | remoteBytes | semmle.label | remoteBytes |
+| test.ps1:101:16:101:92 | Call to downloaddata | semmle.label | Call to downloaddata |
+| test.ps1:102:1:102:13 | remoteStream | semmle.label | remoteStream |
+| test.ps1:102:17:102:59 | Call to new | semmle.label | Call to new |
+| test.ps1:102:47:102:58 | remoteBytes | semmle.label | remoteBytes |
+| test.ps1:103:41:103:53 | remoteStream | semmle.label | remoteStream |
 subpaths

powershell/ql/test/query-tests/security/cwe-502/test.ps1

@@ -84,3 +84,20 @@ $obj16 = $yamlDeserializer.Deserialize($input16) # $ Alert
 $input17 = Read-Host "Enter packed data" # $ Source
 $bytes17 = [Convert]::FromBase64String($input17)
 [MemoryPack.MemoryPackSerializer]::Deserialize($bytes17) # $ Alert
+
+# Test 18: self-serialized BinaryFormatter data should not be reported by the taint-based query
+$safeFormatter = New-Object System.Runtime.Serialization.Formatters.Binary.BinaryFormatter
+$safeStream = [System.IO.MemoryStream]::new()
+$safeFormatter.Serialize($safeStream, [PSCustomObject]@{ Name = "local" })
+$safeStream.Position = 0
+$safeObj = $safeFormatter.Deserialize($safeStream)
+
+# Test 19: file-controlled bytes deserialized with BinaryFormatter
+$fileBytes = [System.IO.File]::ReadAllBytes("payload.bin") # $ Source
+$fileStream = [System.IO.MemoryStream]::new($fileBytes)
+$fileObj = $safeFormatter.Deserialize($fileStream) # $ Alert
+
+# Test 20: remote-controlled bytes deserialized with BinaryFormatter
+$remoteBytes = [System.Net.WebClient]::new().DownloadData("https://example.com/payload.bin") # $ Source
+$remoteStream = [System.IO.MemoryStream]::new($remoteBytes)
+$remoteObj = $safeFormatter.Deserialize($remoteStream) # $ Alert