Fixing a false positive in cs/insecure-sql-connection, and adding a new query to remediate a false negative

Author: raulgarciamsft

csharp/ql/src/Security Features/CWE-327/InsecureSQLConnection.ql

@@ -12,6 +12,7 @@
 
 import csharp
 import InsecureSqlConnection::PathGraph
+import InsecureSQLConnection
 
 class Source extends DataFlow::Node {
   string sourcestring;
@@ -48,6 +49,11 @@ class Sink extends DataFlow::Node {
 predicate isEncryptTrue(Source source, Sink sink) {
   sink.isEncryptedByDefault() and
   not source.setsEncryptFalse()
+  or
+  exists(ObjectCreation oc, Expr e | oc.getRuntimeArgument(0) = sink.asExpr() |
+    getInfoForInitializedConnEncryption(oc, e) and
+    e.getValue().toString().toLowerCase() = "true"
+  )
 }
 
 /**

csharp/ql/src/Security Features/CWE-327/InsecureSQLConnection.qll

@@ -0,0 +1,11 @@
+import csharp
+
+/**
+ * Holds if `ObjectCreation` has an initializer for a member named `Encrypt`, set to `e`
+ */
+predicate getInfoForInitializedConnEncryption(ObjectCreation oc, Expr e) {
+  exists(MemberInitializer mi | mi.getInitializedMember().hasName("Encrypt") |
+    e = mi.getRValue() and
+    oc.getInitializer().(ObjectInitializer).getAMemberInitializer() = mi
+  )
+}

csharp/ql/src/Security Features/CWE-327/InsecureSQLConnectionInitializer.qhelp

@@ -0,0 +1,45 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+
+  <p>
+    SQL Server connections where the client is not enforcing the encryption in transit are susceptible to multiple attacks, including a man-in-the-middle, that would potentially compromise the user credentials and/or the TDS session.
+  </p>
+
+</overview>
+<recommendation>
+
+<p>Ensure that the client code enforces the <code>Encrypt</code> option by setting it to <code>true</code> in the connection string or as a property.</p>
+<p>Explicitly setting the property <code>Encrypt</code> to <code>false</code> will result in unprotected connections.</p>
+
+</recommendation>
+<example>
+
+<p>The following example shows a SQL connection string that is explicitly disabling the <code>Encrypt</code> setting.</p>
+
+<sample src="InsecureSQLConnectionInitializerBad.cs" />
+
+  <p>
+    The following example shows a SQL connection string that is explicitly enabling the <code>Encrypt</code> setting to force encryption in transit.
+  </p>
+
+  <sample src="InsecureSQLConnectionInitializerGood.cs" />
+
+</example>
+<references>
+  <li>Microsoft, SQL Protocols blog: 
+    <a href="https://blogs.msdn.microsoft.com/sql_protocols/2009/10/19/selectively-using-secure-connection-to-sql-server/">Selectively using secure connection to SQL Server</a>.
+  </li>
+  <li>Microsoft:
+    <a href="https://msdn.microsoft.com/en-us/library/system.data.sqlclient.sqlconnection.connectionstring(v=vs.110).aspx">SqlConnection.ConnectionString Property</a>.
+  </li>
+  <li>Microsoft: 
+    <a href="https://msdn.microsoft.com/en-us/library/ms130822.aspx">Using Connection String Keywords with SQL Server Native Client</a>.
+  </li>
+  <li>Microsoft: 
+    <a href="https://msdn.microsoft.com/en-us/library/ms378988(v=sql.110).aspx">Setting the connection properties</a>.
+  </li>
+</references>
+</qhelp>

csharp/ql/src/Security Features/CWE-327/InsecureSQLConnectionInitializer.ql

@@ -0,0 +1,48 @@
+/**
+ * @name Insecure SQL connection in Initializer
+ * @description Using an SQL Server connection without enforcing encryption is a security vulnerability.
+ *              This rule variant will flag when the `encrypt` property is explicitly set to `false` during the object initializer
+ * @kind path-problem
+ * @id cs/insecure-sql-connection-initializer
+ * @problem.severity error
+ * @security-severity 7.5
+ * @precision medium
+ * @tags security
+ *       external/cwe/cwe-327
+ */
+
+import csharp
+import InsecureSqlConnectionInitialize::PathGraph
+import InsecureSQLConnection
+
+class Source extends DataFlow::Node {
+  Source() { this.asExpr().(BoolLiteral).getBoolValue() = false }
+}
+
+class Sink extends DataFlow::Node {
+  Sink() { getInfoForInitializedConnEncryption(_, this.asExpr()) }
+}
+
+/**
+ * A data flow configuration for tracking strings passed to `SqlConnection[StringBuilder]` instances.
+ */
+module InsecureSqlConnectionInitializeConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) { source instanceof Source }
+
+  predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
+}
+
+/**
+ * A data flow configuration for tracking strings passed to `SqlConnection[StringBuilder]` instances.
+ */
+module InsecureSqlConnectionInitialize = DataFlow::Global<InsecureSqlConnectionInitializeConfig>;
+
+from
+  ObjectCreation oc, InsecureSqlConnectionInitialize::PathNode source,
+  InsecureSqlConnectionInitialize::PathNode sink
+where
+  InsecureSqlConnectionInitialize::flowPath(source, sink) and
+  getInfoForInitializedConnEncryption(oc, sink.getNode().asExpr())
+select sink.getNode(), source, sink,
+  "A value evaluating to $@ flows to $@ and sets the `encrypt` property.", source.getNode(),
+  "`false`", oc, "this SQL connection initializer"

csharp/ql/src/Security Features/CWE-327/InsecureSQLConnectionInitializerBad.cs

@@ -0,0 +1,7 @@
+using System.Data.SqlClient;
+
+// BAD, Encrypt not specified
+string connectString =
+    "Server=1.2.3.4;Database=Anything;Integrated Security=true;";
+var builder = new SqlConnectionStringBuilder(SetToTrueConnStr) { Encrypt = false }
+var conn = new SqlConnection(builder.ConnectionString);

csharp/ql/src/Security Features/CWE-327/InsecureSQLConnectionInitializerGood.cs

@@ -0,0 +1,7 @@
+using System.Data.SqlClient;
+
+// BAD, Encrypt not specified
+string connectString =
+    "Server=1.2.3.4;Database=Anything;Integrated Security=true;";
+var builder = new SqlConnectionStringBuilder(SetToTrueConnStr) { Encrypt = true }
+var conn = new SqlConnection(builder.ConnectionString);

csharp/ql/test/query-tests/Security Features/CWE-327/InsecureSQLConnection/InsecureSQLConnection.cs

@@ -1,3 +1,4 @@
+using System;
 using System.Data.SqlClient;
 
 namespace InsecureSQLConnection
@@ -34,21 +35,21 @@ public void StringInBuilderProperty()
         public void StringInInitializer()
         {
             string connectString = "Server=1.2.3.4;Database=Anything;UID=ab;Pwd=cd;Encrypt=false";
-            SqlConnectionStringBuilder conBuilder = new SqlConnectionStringBuilder(connectString) { Encrypt = true};
+            SqlConnectionStringBuilder conBuilder = new SqlConnectionStringBuilder(connectString) { Encrypt = true}; // False Positive
         }
 
 
         public void TriggerThis()
         {
-            // BAD, Encrypt not specified
+            // BAD, Encrypt not specified (version dependent)
             SqlConnection conn = new SqlConnection("Server=myServerName\\myInstanceName;Database=myDataBase;User Id=myUsername;");
         }
 
         void Test4()
         {
             string connectString =
                 "Server=1.2.3.4;Database=Anything;UID=ab;Pwd=cd";
-            // BAD, Encrypt not specified
+            // BAD, Encrypt not specified (version dependent)
             SqlConnectionStringBuilder builder = new SqlConnectionStringBuilder(connectString);
             var conn = new SqlConnection(builder.ConnectionString);
         }
@@ -61,5 +62,20 @@ void Test5()
             SqlConnectionStringBuilder builder = new SqlConnectionStringBuilder(connectString);
             var conn = new SqlConnection(builder.ConnectionString);
         }
+
+        void Test6()
+        {
+            var conn = new SqlConnectionStringBuilder(SetToTrueConnStr) { Encrypt = false };    // Bug - cs/insecure-sql-connection-initializer
+        }
+
+        void Test72ndPhase(bool encrypt)
+        {
+            var conn = new SqlConnectionStringBuilder(SetToTrueConnStr) { Encrypt = encrypt };    // Bug - cs/insecure-sql-connection-initializer (sink)
+        }
+
+        void Test7()
+        {
+            Test72ndPhase(false);    // Bug - cs/insecure-sql-connection-initializer (source)
+        }
     }
 }

csharp/ql/test/query-tests/Security Features/CWE-327/InsecureSQLConnection/InsecureSQLConnection.qlref

@@ -1 +1 @@
-Security Features/CWE-327/InsecureSQLConnection.ql
+Security Features/CWE-327/InsecureSQLConnectionInitializer.ql

csharp/ql/test/query-tests/Security Features/CWE-327/InsecureSQLConnection/InsecureSQLConnectionInitializer.qlref

@@ -0,0 +1 @@
+Security Features/CWE-327/InsecureSQLConnection.ql