Update addressing review comments

Author: Denis Levin

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

@@ -3,9 +3,8 @@
  * @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/do-not-add-certs-to-root-store
+ * @id cs/adding-cert-to-root-store
  * @problem.severity error
- * @precision high
  * @tags security
  *       external/cwe/cwe-327
  */
@@ -16,21 +15,22 @@ class AddCertToRootStoreConfig extends DataFlow::Configuration {
   AddCertToRootStoreConfig() { this = "Adding Certificate To Root Store" }
 
   override predicate isSource(DataFlow::Node source) {     
- 	 	exists(ObjectCreation oc | oc = source.asExpr().(ObjectCreation) | 
-			oc.getType().(RefType).hasQualifiedName("System.Security.Cryptography.X509Certificates.X509Store") 
-			and oc.getArgument(0).(Access).getTarget().hasName("Root")
-		)
+    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")
- 	 			and sink.asExpr() = mc.getQualifier()
+      (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, "Do not add certificates to root certificate store"  
+select mc, "Certificate added to the root certificate store. Do not add certificates to root certificate store."  
 

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

@@ -1,2 +1,3 @@
-| Test.cs:19:13:19:17 | access to local variable store | Do not add certificates to root certificate store |
-| Test.cs:28:13:28:17 | access to local variable store | Do not add certificates to root certificate store |
+| Test.cs:19:13:19:17 | access to local variable store | Certificate added to the root certificate store. Do not add certificates to root certificate store. |
+| Test.cs:28:13:28:17 | access to local variable store | Certificate added to the root certificate store. Do not add certificates to root certificate store. |
+| Test.cs:69:13:69:17 | access to local variable store | Certificate added to the root certificate store. Do not add certificates to root certificate store. |

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

@@ -55,5 +55,19 @@ public void RemoveRootCert()
             store.Remove(new X509Certificate2(X509Certificate2.CreateFromCertFile(file)));
             store.Close();
         }
+
+        public void InstallRoorCertRange()
+        {
+            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);
+            store.AddRange(new X509Certificate2Collection(certCollection));
+            store.Close();
+        }
     }
 }