C#: Add local filesystem writes as External Location sinks

csharp/ql/lib/ext/System.IO.model.yml

@@ -4,6 +4,23 @@ extensions:
       extensible: sourceModel
     data:
       - ["System.IO", "FileStream", False, "FileStream", "", "", "Argument[this]", "file", "manual"]
+      - ["System.IO", "FileStream", False, "FileStream", "", "", "Argument[this]", "file-write", "manual"]
+      - ["System.IO", "StreamWriter", False, "StreamWriter", "(System.String)", "", "Argument[this]", "file-write", "manual"]
+      - ["System.IO", "StreamWriter", False, "StreamWriter", "(System.String,System.Boolean)", "", "Argument[this]", "file-write", "manual"]
+      - ["System.IO", "StreamWriter", False, "StreamWriter", "(System.String,System.Boolean,System.Text.Encoding)", "", "Argument[this]", "file-write", "manual"]
+      - ["System.IO", "StreamWriter", False, "StreamWriter", "(System.String,System.Boolean,System.Text.Encoding,System.Int32)", "", "Argument[this]", "file-write", "manual"]
+      - ["System.IO", "StreamWriter", False, "StreamWriter", "(System.String,System.Text.Encoding,System.IO.FileStreamOptions)", "", "Argument[this]", "file-write", "manual"]
+      - ["System.IO", "StreamWriter", False, "StreamWriter", "(System.String,System.IO.FileStreamOptions)", "", "Argument[this]", "file-write", "manual"]
+      - ["System.IO", "File", False, "Open", "", "", "ReturnValue", "file-write", "manual"]
+      - ["System.IO", "File", False, "OpenWrite", "", "", "ReturnValue", "file-write", "manual"]
+      - ["System.IO", "File", False, "Create", "", "", "ReturnValue", "file-write", "manual"]
+      - ["System.IO", "File", False, "CreateText", "", "", "ReturnValue", "file-write", "manual"]
+      - ["System.IO", "File", False, "AppendText", "", "", "ReturnValue", "file-write", "manual"]
+      - ["System.IO", "FileInfo", False, "Open", "", "", "ReturnValue", "file-write", "manual"]
+      - ["System.IO", "FileInfo", False, "OpenWrite", "", "", "ReturnValue", "file-write", "manual"]
+      - ["System.IO", "FileInfo", False, "Create", "", "", "ReturnValue", "file-write", "manual"]
+      - ["System.IO", "FileInfo", False, "CreateText", "", "", "ReturnValue", "file-write", "manual"]
+      - ["System.IO", "FileInfo", False, "AppendText", "", "", "ReturnValue", "file-write", "manual"]
   - addsTo:
       pack: codeql/csharp-all
       extensible: summaryModel

csharp/ql/lib/semmle/code/csharp/dataflow/ExternalFlow.qll

@@ -212,7 +212,7 @@ module ModelValidation {
     )
     or
     exists(string kind | sourceModel(_, _, _, _, _, _, _, kind, _) |
-      not kind = ["local", "remote", "file"] and
+      not kind = ["local", "remote", "file", "file-write"] and
       result = "Invalid kind \"" + kind + "\" in source model."
     )
   }

csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsinks/ExternalLocationSink.qll

@@ -6,6 +6,7 @@ import csharp
 private import Remote
 private import semmle.code.csharp.commons.Loggers
 private import semmle.code.csharp.frameworks.system.Web
+private import semmle.code.csharp.frameworks.system.IO
 private import semmle.code.csharp.dataflow.ExternalFlow
 
 /**
@@ -63,3 +64,56 @@ class CookieStorageSink extends ExternalLocationSink, RemoteFlowSink {
     )
   }
 }
+
+private predicate isFileWriteCall(Expr stream, Expr data) {
+  exists(MethodCall mc, Method m | mc.getTarget() = m.getAnOverrider*() |
+    m.hasQualifiedName("System.IO", "Stream", ["Write", "WriteAsync"]) and
+    stream = mc.getQualifier() and
+    data = mc.getArgument(0)
+    or
+    m.hasQualifiedName("System.IO", "TextWriter",
+      ["Write", "WriteAsync", "WriteLine", "WriteLineAsync"]) and
+    stream = mc.getQualifier() and
+    data = mc.getArgument(0)
+    or
+    m.hasQualifiedName("System.Xml.Linq", "XDocument", ["Save", "SaveAsync"]) and
+    data = mc.getQualifier() and
+    stream = mc.getArgument(0)
+  )
+}
+
+/** A configuration for tracking flow from calls that open a file in write mode to methods that write to that file, excluding encrypted streams. */
+private module LocalFileOutputStreamConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node src) { sourceNode(src, "file-write") }
+
+  predicate isSink(DataFlow::Node sink) { isFileWriteCall(sink.asExpr(), _) }
+
+  predicate isBarrier(DataFlow::Node node) {
+    node.asExpr()
+        .(ObjectCreation)
+        .getObjectType()
+        .hasQualifiedName("System.Security.Cryptography", "CryptoStream")
+  }
+
+  predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
+    exists(ObjectCreation oc |
+      node2.asExpr() = oc and
+      node1.asExpr() = oc.getArgument(0) and
+      oc.getObjectType() instanceof SystemIOStreamWriterClass
+    )
+  }
+}
+
+private module LocalFileOutputStreamFlow = DataFlow::Global<LocalFileOutputStreamConfig>;
+
+/**
+ * A write to the local filesystem.
+ */
+class LocalFileOutputSink extends ExternalLocationSink {
+  LocalFileOutputSink() {
+    exists(DataFlow::Node streamSink |
+      LocalFileOutputStreamFlow::flowTo(streamSink) and
+      isFileWriteCall(streamSink.asExpr(), this.asExpr())
+    )
+  }
+}

csharp/ql/src/change-notes/2023-04-05-external-location-sinks.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Additional sinks modelling writes to unencrypted local files have been added to `ExternalLocationSink`, used by the `cs/cleartext-storage` and `cs/exposure-of-sensitive-information` queries.

csharp/ql/test/query-tests/Security Features/CWE-312/CleartextStorage.cs

@@ -2,6 +2,8 @@
 using System.Web;
 using System.Web.Security;
 using System.Windows.Forms;
+using System.IO;
+using System.Security.Cryptography;
 
 public class ClearTextStorageHandler : IHttpHandler
 {
@@ -24,13 +26,33 @@ public void ProcessRequest(HttpContext ctx)
         logger.Warn(GetPassword());
         // GOOD: Logging encrypted sensitive data
         logger.Warn(Encode(GetPassword(), "Password"));
+
+        // BAD: Storing sensitive data in local file 
+        using (var writeStream = File.Open("passwords.txt", FileMode.Create))
+        {
+            var writer = new StreamWriter(writeStream);
+            writer.Write(GetPassword());
+            writer.Close();
+        }
+
+        // GOOD: Storing encrypted sensitive data
+        using (var writeStream = File.Open("passwords.txt", FileMode.Create))
+        {
+            var writer = new StreamWriter(new CryptoStream(writeStream, GetEncryptor(), CryptoStreamMode.Write));
+            writer.Write(GetPassword());
+            writer.Close();
+        }
     }
 
     public string Encode(string value, string type)
     {
         return Encoding.UTF8.GetString(MachineKey.Protect(Encoding.UTF8.GetBytes(value), type));
     }
 
+    public ICryptoTransform GetEncryptor(){
+        return null;
+    }
+
     public string GetPassword()
     {
         return "password";

csharp/ql/test/query-tests/Security Features/CWE-312/CleartextStorage.expected

@@ -1,20 +1,22 @@
 edges
 nodes
-| CleartextStorage.cs:13:50:13:59 | access to field accountKey | semmle.label | access to field accountKey |
-| CleartextStorage.cs:14:62:14:74 | call to method GetPassword | semmle.label | call to method GetPassword |
-| CleartextStorage.cs:15:69:15:81 | call to method GetPassword | semmle.label | call to method GetPassword |
-| CleartextStorage.cs:16:50:16:63 | call to method GetAccountID | semmle.label | call to method GetAccountID |
-| CleartextStorage.cs:24:21:24:33 | call to method GetPassword | semmle.label | call to method GetPassword |
-| CleartextStorage.cs:72:21:72:33 | access to property Text | semmle.label | access to property Text |
-| CleartextStorage.cs:73:21:73:29 | access to property Text | semmle.label | access to property Text |
-| CleartextStorage.cs:74:21:74:29 | access to property Text | semmle.label | access to property Text |
+| CleartextStorage.cs:15:50:15:59 | access to field accountKey | semmle.label | access to field accountKey |
+| CleartextStorage.cs:16:62:16:74 | call to method GetPassword | semmle.label | call to method GetPassword |
+| CleartextStorage.cs:17:69:17:81 | call to method GetPassword | semmle.label | call to method GetPassword |
+| CleartextStorage.cs:18:50:18:63 | call to method GetAccountID | semmle.label | call to method GetAccountID |
+| CleartextStorage.cs:26:21:26:33 | call to method GetPassword | semmle.label | call to method GetPassword |
+| CleartextStorage.cs:34:26:34:38 | call to method GetPassword | semmle.label | call to method GetPassword |
+| CleartextStorage.cs:94:21:94:33 | access to property Text | semmle.label | access to property Text |
+| CleartextStorage.cs:95:21:95:29 | access to property Text | semmle.label | access to property Text |
+| CleartextStorage.cs:96:21:96:29 | access to property Text | semmle.label | access to property Text |
 subpaths
 #select
-| CleartextStorage.cs:13:50:13:59 | access to field accountKey | CleartextStorage.cs:13:50:13:59 | access to field accountKey | CleartextStorage.cs:13:50:13:59 | access to field accountKey | This stores sensitive data returned by $@ as clear text. | CleartextStorage.cs:13:50:13:59 | access to field accountKey | access to field accountKey |
-| CleartextStorage.cs:14:62:14:74 | call to method GetPassword | CleartextStorage.cs:14:62:14:74 | call to method GetPassword | CleartextStorage.cs:14:62:14:74 | call to method GetPassword | This stores sensitive data returned by $@ as clear text. | CleartextStorage.cs:14:62:14:74 | call to method GetPassword | call to method GetPassword |
-| CleartextStorage.cs:15:69:15:81 | call to method GetPassword | CleartextStorage.cs:15:69:15:81 | call to method GetPassword | CleartextStorage.cs:15:69:15:81 | call to method GetPassword | This stores sensitive data returned by $@ as clear text. | CleartextStorage.cs:15:69:15:81 | call to method GetPassword | call to method GetPassword |
-| CleartextStorage.cs:16:50:16:63 | call to method GetAccountID | CleartextStorage.cs:16:50:16:63 | call to method GetAccountID | CleartextStorage.cs:16:50:16:63 | call to method GetAccountID | This stores sensitive data returned by $@ as clear text. | CleartextStorage.cs:16:50:16:63 | call to method GetAccountID | call to method GetAccountID |
-| CleartextStorage.cs:24:21:24:33 | call to method GetPassword | CleartextStorage.cs:24:21:24:33 | call to method GetPassword | CleartextStorage.cs:24:21:24:33 | call to method GetPassword | This stores sensitive data returned by $@ as clear text. | CleartextStorage.cs:24:21:24:33 | call to method GetPassword | call to method GetPassword |
-| CleartextStorage.cs:72:21:72:33 | access to property Text | CleartextStorage.cs:72:21:72:33 | access to property Text | CleartextStorage.cs:72:21:72:33 | access to property Text | This stores sensitive data returned by $@ as clear text. | CleartextStorage.cs:72:21:72:33 | access to property Text | access to property Text |
-| CleartextStorage.cs:73:21:73:29 | access to property Text | CleartextStorage.cs:73:21:73:29 | access to property Text | CleartextStorage.cs:73:21:73:29 | access to property Text | This stores sensitive data returned by $@ as clear text. | CleartextStorage.cs:73:21:73:29 | access to property Text | access to property Text |
-| CleartextStorage.cs:74:21:74:29 | access to property Text | CleartextStorage.cs:74:21:74:29 | access to property Text | CleartextStorage.cs:74:21:74:29 | access to property Text | This stores sensitive data returned by $@ as clear text. | CleartextStorage.cs:74:21:74:29 | access to property Text | access to property Text |
+| CleartextStorage.cs:15:50:15:59 | access to field accountKey | CleartextStorage.cs:15:50:15:59 | access to field accountKey | CleartextStorage.cs:15:50:15:59 | access to field accountKey | This stores sensitive data returned by $@ as clear text. | CleartextStorage.cs:15:50:15:59 | access to field accountKey | access to field accountKey |
+| CleartextStorage.cs:16:62:16:74 | call to method GetPassword | CleartextStorage.cs:16:62:16:74 | call to method GetPassword | CleartextStorage.cs:16:62:16:74 | call to method GetPassword | This stores sensitive data returned by $@ as clear text. | CleartextStorage.cs:16:62:16:74 | call to method GetPassword | call to method GetPassword |
+| CleartextStorage.cs:17:69:17:81 | call to method GetPassword | CleartextStorage.cs:17:69:17:81 | call to method GetPassword | CleartextStorage.cs:17:69:17:81 | call to method GetPassword | This stores sensitive data returned by $@ as clear text. | CleartextStorage.cs:17:69:17:81 | call to method GetPassword | call to method GetPassword |
+| CleartextStorage.cs:18:50:18:63 | call to method GetAccountID | CleartextStorage.cs:18:50:18:63 | call to method GetAccountID | CleartextStorage.cs:18:50:18:63 | call to method GetAccountID | This stores sensitive data returned by $@ as clear text. | CleartextStorage.cs:18:50:18:63 | call to method GetAccountID | call to method GetAccountID |
+| CleartextStorage.cs:26:21:26:33 | call to method GetPassword | CleartextStorage.cs:26:21:26:33 | call to method GetPassword | CleartextStorage.cs:26:21:26:33 | call to method GetPassword | This stores sensitive data returned by $@ as clear text. | CleartextStorage.cs:26:21:26:33 | call to method GetPassword | call to method GetPassword |
+| CleartextStorage.cs:34:26:34:38 | call to method GetPassword | CleartextStorage.cs:34:26:34:38 | call to method GetPassword | CleartextStorage.cs:34:26:34:38 | call to method GetPassword | This stores sensitive data returned by $@ as clear text. | CleartextStorage.cs:34:26:34:38 | call to method GetPassword | call to method GetPassword |
+| CleartextStorage.cs:94:21:94:33 | access to property Text | CleartextStorage.cs:94:21:94:33 | access to property Text | CleartextStorage.cs:94:21:94:33 | access to property Text | This stores sensitive data returned by $@ as clear text. | CleartextStorage.cs:94:21:94:33 | access to property Text | access to property Text |
+| CleartextStorage.cs:95:21:95:29 | access to property Text | CleartextStorage.cs:95:21:95:29 | access to property Text | CleartextStorage.cs:95:21:95:29 | access to property Text | This stores sensitive data returned by $@ as clear text. | CleartextStorage.cs:95:21:95:29 | access to property Text | access to property Text |
+| CleartextStorage.cs:96:21:96:29 | access to property Text | CleartextStorage.cs:96:21:96:29 | access to property Text | CleartextStorage.cs:96:21:96:29 | access to property Text | This stores sensitive data returned by $@ as clear text. | CleartextStorage.cs:96:21:96:29 | access to property Text | access to property Text |

csharp/ql/test/query-tests/Security Features/CWE-312/options

@@ -1 +1 @@
-semmle-extractor-options: ${testdir}/../../../resources/stubs/System.Web.cs /r:System.Collections.Specialized.dll {testdir}/../../../../resources/stubs/System.Windows.cs
+semmle-extractor-options: ${testdir}/../../../resources/stubs/System.Web.cs /r:System.Collections.Specialized.dll /r:System.Security.Cryptography.dll {testdir}/../../../../resources/stubs/System.Windows.cs

csharp/ql/test/query-tests/Security Features/CWE-359/ExposureOfPrivateInformation.cs

@@ -1,4 +1,6 @@
 using System.Web;
+using System.Security.Cryptography; 
+using System.IO;
 
 public class Person
 {
@@ -21,9 +23,29 @@ public void ProcessRequest(HttpContext ctx)
         ILogger logger = new ILogger();
         logger.Warn(p.getTelephone());
 
+        // BAD: Storing sensitive data in unencrypted local file 
+        using (var writeStream = File.Open("telephones.txt", FileMode.Create))
+        {
+            var writer = new StreamWriter(writeStream);
+            writer.Write(p.getTelephone());
+            writer.Close();
+        }
+
+        // GOOD: Storing encrypted sensitive data
+        using (var writeStream = File.Open("telephones.txt", FileMode.Create))
+        {
+            var writer = new StreamWriter(new CryptoStream(writeStream, GetEncryptor(), CryptoStreamMode.Write));
+            writer.Write(p.getTelephone());
+            writer.Close();
+        }
+
         // GOOD: Don't write these values to sensitive locations in the first place
     }
 
+    public ICryptoTransform GetEncryptor(){
+        return null;
+    }
+
     public bool IsReusable
     {
         get

csharp/ql/test/query-tests/Security Features/CWE-359/ExposureOfPrivateInformation.expected

@@ -1,12 +1,14 @@
 edges
 nodes
-| ExposureOfPrivateInformation.cs:16:50:16:84 | access to indexer | semmle.label | access to indexer |
-| ExposureOfPrivateInformation.cs:18:50:18:65 | call to method getTelephone | semmle.label | call to method getTelephone |
-| ExposureOfPrivateInformation.cs:22:21:22:36 | call to method getTelephone | semmle.label | call to method getTelephone |
-| ExposureOfPrivateInformation.cs:40:21:40:33 | access to property Text | semmle.label | access to property Text |
+| ExposureOfPrivateInformation.cs:18:50:18:84 | access to indexer | semmle.label | access to indexer |
+| ExposureOfPrivateInformation.cs:20:50:20:65 | call to method getTelephone | semmle.label | call to method getTelephone |
+| ExposureOfPrivateInformation.cs:24:21:24:36 | call to method getTelephone | semmle.label | call to method getTelephone |
+| ExposureOfPrivateInformation.cs:30:26:30:41 | call to method getTelephone | semmle.label | call to method getTelephone |
+| ExposureOfPrivateInformation.cs:62:21:62:33 | access to property Text | semmle.label | access to property Text |
 subpaths
 #select
-| ExposureOfPrivateInformation.cs:16:50:16:84 | access to indexer | ExposureOfPrivateInformation.cs:16:50:16:84 | access to indexer | ExposureOfPrivateInformation.cs:16:50:16:84 | access to indexer | Private data returned by $@ is written to an external location. | ExposureOfPrivateInformation.cs:16:50:16:84 | access to indexer | access to indexer |
-| ExposureOfPrivateInformation.cs:18:50:18:65 | call to method getTelephone | ExposureOfPrivateInformation.cs:18:50:18:65 | call to method getTelephone | ExposureOfPrivateInformation.cs:18:50:18:65 | call to method getTelephone | Private data returned by $@ is written to an external location. | ExposureOfPrivateInformation.cs:18:50:18:65 | call to method getTelephone | call to method getTelephone |
-| ExposureOfPrivateInformation.cs:22:21:22:36 | call to method getTelephone | ExposureOfPrivateInformation.cs:22:21:22:36 | call to method getTelephone | ExposureOfPrivateInformation.cs:22:21:22:36 | call to method getTelephone | Private data returned by $@ is written to an external location. | ExposureOfPrivateInformation.cs:22:21:22:36 | call to method getTelephone | call to method getTelephone |
-| ExposureOfPrivateInformation.cs:40:21:40:33 | access to property Text | ExposureOfPrivateInformation.cs:40:21:40:33 | access to property Text | ExposureOfPrivateInformation.cs:40:21:40:33 | access to property Text | Private data returned by $@ is written to an external location. | ExposureOfPrivateInformation.cs:40:21:40:33 | access to property Text | access to property Text |
+| ExposureOfPrivateInformation.cs:18:50:18:84 | access to indexer | ExposureOfPrivateInformation.cs:18:50:18:84 | access to indexer | ExposureOfPrivateInformation.cs:18:50:18:84 | access to indexer | Private data returned by $@ is written to an external location. | ExposureOfPrivateInformation.cs:18:50:18:84 | access to indexer | access to indexer |
+| ExposureOfPrivateInformation.cs:20:50:20:65 | call to method getTelephone | ExposureOfPrivateInformation.cs:20:50:20:65 | call to method getTelephone | ExposureOfPrivateInformation.cs:20:50:20:65 | call to method getTelephone | Private data returned by $@ is written to an external location. | ExposureOfPrivateInformation.cs:20:50:20:65 | call to method getTelephone | call to method getTelephone |
+| ExposureOfPrivateInformation.cs:24:21:24:36 | call to method getTelephone | ExposureOfPrivateInformation.cs:24:21:24:36 | call to method getTelephone | ExposureOfPrivateInformation.cs:24:21:24:36 | call to method getTelephone | Private data returned by $@ is written to an external location. | ExposureOfPrivateInformation.cs:24:21:24:36 | call to method getTelephone | call to method getTelephone |
+| ExposureOfPrivateInformation.cs:30:26:30:41 | call to method getTelephone | ExposureOfPrivateInformation.cs:30:26:30:41 | call to method getTelephone | ExposureOfPrivateInformation.cs:30:26:30:41 | call to method getTelephone | Private data returned by $@ is written to an external location. | ExposureOfPrivateInformation.cs:30:26:30:41 | call to method getTelephone | call to method getTelephone |
+| ExposureOfPrivateInformation.cs:62:21:62:33 | access to property Text | ExposureOfPrivateInformation.cs:62:21:62:33 | access to property Text | ExposureOfPrivateInformation.cs:62:21:62:33 | access to property Text | Private data returned by $@ is written to an external location. | ExposureOfPrivateInformation.cs:62:21:62:33 | access to property Text | access to property Text |