Merge pull request #55 from denislevin/denisl/cs/DontInstallRootCertificate

cs: Don't Install Root Certificate (CWE-327)

Author: calumgrant

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

@@ -0,0 +1,36 @@
+/**
+ * @name Do not add certificates to the system root store.
+ * @description Application- or user-specific certificates placed in the system root store could
+ *              weaken security for other processing running on the same system.
+ * @kind problem
+ * @id cs/adding-cert-to-root-store
+ * @problem.severity error
+ * @tags security
+ *       external/cwe/cwe-327
+ */
+import csharp
+import semmle.code.csharp.dataflow.DataFlow::DataFlow
+
+class AddCertToRootStoreConfig extends DataFlow::Configuration {
+  AddCertToRootStoreConfig() { this = "Adding Certificate To Root Store" }
+ 
+  override predicate isSource(DataFlow::Node source) {     
+    exists(ObjectCreation oc | oc = source.asExpr() | 
+      oc.getType().(RefType).hasQualifiedName("System.Security.Cryptography.X509Certificates.X509Store") and
+      oc.getArgument(0).(Access).getTarget().hasName("Root")
+    )
+  }
+ 
+  override predicate isSink(DataFlow::Node sink) {
+    exists(MethodCall mc |
+      (mc.getTarget().hasQualifiedName("System.Security.Cryptography.X509Certificates.X509Store", "Add") or
+       mc.getTarget().hasQualifiedName("System.Security.Cryptography.X509Certificates.X509Store", "AddRange")) and
+       sink.asExpr() = mc.getQualifier()
+    )
+  }
+}
+
+from Expr oc, Expr mc, AddCertToRootStoreConfig config
+where config.hasFlow(DataFlow::exprNode(oc), DataFlow::exprNode(mc))
+select mc, "Certificate added to the root certificate store."
+

csharp/ql/test/query-tests/Security Features/CWE-327/DontInstallRootCert/DontInstallRootCert.expected

@@ -0,0 +1,3 @@
+| Test.cs:20:13:20:17 | access to local variable store | Certificate added to the root certificate store. |
+| Test.cs:30:13:30:17 | access to local variable store | Certificate added to the root certificate store. |
+| Test.cs:75:13:75:17 | access to local variable store | Certificate added to the root certificate store. |

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

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

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

@@ -0,0 +1,79 @@
+// semmle-extractor-options: /r:System.Security.Cryptography.X509Certificates.dll
+
+using System;
+using System.Collections.Generic;
+using System.Linq;
+using System.Security.Cryptography.X509Certificates;
+using System.Text;
+using System.Threading.Tasks;
+
+namespace RootCert
+{
+    public class Class1
+    {
+        public void InstallRootCert()
+        {
+            string file = "mytest.pfx"; // Contains name of certificate file
+            X509Store store = new X509Store(StoreName.Root);
+            store.Open(OpenFlags.ReadWrite);
+            // BAD: adding a certificate to the Root store
+            store.Add(new X509Certificate2(X509Certificate2.CreateFromCertFile(file)));
+            store.Close();
+        }
+
+        public void InstallRootCert2()
+        {
+            string file = "mytest.pfx"; // Contains name of certificate file
+            X509Store store = new X509Store(StoreName.Root, StoreLocation.CurrentUser);
+            store.Open(OpenFlags.ReadWrite);
+            // BAD: adding a certificate to the Root store
+            store.Add(new X509Certificate2(X509Certificate2.CreateFromCertFile(file)));
+            store.Close();
+        }
+
+        public void InstallUserCert()
+        {
+            string file = "mytest.pfx"; // Contains name of certificate file
+            X509Store store = new X509Store(StoreName.My);
+            store.Open(OpenFlags.ReadWrite);
+            // GOOD: adding a certificate to My store
+            store.Add(new X509Certificate2(X509Certificate2.CreateFromCertFile(file)));
+            store.Close();
+        }
+
+        public void RemoveUserCert()
+        {
+            string file = "mytest.pfx"; // Contains name of certificate file
+            X509Store store = new X509Store(StoreName.My);
+            store.Open(OpenFlags.ReadWrite);
+            // GOOD: removing a certificate from My store
+            store.Remove(new X509Certificate2(X509Certificate2.CreateFromCertFile(file)));
+            store.Close();
+        }
+
+        public void RemoveRootCert()
+        {
+            string file = "mytest.pfx"; // Contains name of certificate file
+            X509Store store = new X509Store(StoreName.Root);
+            store.Open(OpenFlags.ReadWrite);
+            // GOOD: removing a certificate from Root store
+            store.Remove(new X509Certificate2(X509Certificate2.CreateFromCertFile(file)));
+            store.Close();
+        }
+
+        public void InstallRootCertRange()
+        {
+            string file1 = "mytest1.pfx"; // Contains name of certificate file
+            string file2 = "mytest2.pfx"; // Contains name of certificate file
+            var certCollection = new X509Certificate2[] { 
+                new X509Certificate2(X509Certificate2.CreateFromCertFile(file1)), 
+                new X509Certificate2(X509Certificate2.CreateFromCertFile(file2)),
+            };
+            X509Store store = new X509Store(StoreName.Root, StoreLocation.CurrentUser);
+            store.Open(OpenFlags.ReadWrite);
+            // BAD: adding multiple certificates to the Root store
+            store.AddRange(new X509Certificate2Collection(certCollection));
+            store.Close();
+        }
+    }
+}