Added support for the system CSPRNG

Author: fegge

csharp/ql/lib/experimental/quantum/Language.qll

@@ -1,4 +1,5 @@
 private import csharp as Language
+private import semmle.code.csharp.dataflow.DataFlow
 private import codeql.quantum.experimental.Model
 
 private class UnknownLocation extends Language::Location {
@@ -41,28 +42,142 @@ module CryptoInput implements InputSig<Language::Location> {
 // Instantiate the `CryptographyBase` module
 module Crypto = CryptographyBase<Language::Location, CryptoInput>;
 
-module ArtifactFlowConfig implements Language::DataFlow::ConfigSig {
-  predicate isSource(Language::DataFlow::Node source) {
+/**
+ * An additional flow step in generic data-flow configurations.
+ * Where a step is an edge between nodes `n1` and `n2`,
+ * `this` = `n1` and `getOutput()` = `n2`.
+ *
+ * FOR INTERNAL MODELING USE ONLY.
+ */
+abstract class AdditionalFlowInputStep extends DataFlow::Node {
+  abstract DataFlow::Node getOutput();
+
+  final DataFlow::Node getInput() { result = this }
+}
+
+/**
+ * Generic data source to node input configuration
+ */
+module GenericDataSourceFlowConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) {
+    source = any(Crypto::GenericSourceInstance i).getOutputNode()
+  }
+
+  predicate isSink(DataFlow::Node sink) {
+    sink = any(Crypto::FlowAwareElement other).getInputNode()
+  }
+
+  predicate isBarrierOut(DataFlow::Node node) {
+    node = any(Crypto::FlowAwareElement element).getInputNode()
+  }
+
+  predicate isBarrierIn(DataFlow::Node node) {
+    node = any(Crypto::FlowAwareElement element).getOutputNode()
+  }
+
+  predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
+    node1.(AdditionalFlowInputStep).getOutput() = node2
+  }
+}
+
+module ArtifactFlowConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) {
     source = any(Crypto::ArtifactInstance artifact).getOutputNode()
   }
 
-  predicate isSink(Language::DataFlow::Node sink) {
+  predicate isSink(DataFlow::Node sink) {
     sink = any(Crypto::FlowAwareElement other).getInputNode()
   }
 
-  predicate isBarrierOut(Language::DataFlow::Node node) {
+  predicate isBarrierOut(DataFlow::Node node) {
     node = any(Crypto::FlowAwareElement element).getInputNode()
   }
 
-  predicate isBarrierIn(Language::DataFlow::Node node) {
+  predicate isBarrierIn(DataFlow::Node node) {
     node = any(Crypto::FlowAwareElement element).getOutputNode()
   }
 
-  predicate isAdditionalFlowStep(Language::DataFlow::Node node1, Language::DataFlow::Node node2) {
-    none()
+  predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
+    node1.(AdditionalFlowInputStep).getOutput() = node2
+  }
+}
+
+module GenericDataSourceFlow = TaintTracking::Global<GenericDataSourceFlowConfig>;
+
+module ArtifactFlow = DataFlow::Global<ArtifactFlowConfig>;
+
+/**
+ * A method access that returns random data or writes random data to an argument.
+ */
+abstract class RandomnessSource extends MethodCall {
+  /**
+   * Gets the expression representing the output of this source.
+   */
+  abstract Expr getOutput();
+
+  /**
+   * Gets the type of the source of randomness used by this call.
+   */
+  Type getGenerator() { result = this.getQualifier().getType() }
+}
+
+/**
+ * A call to `System.Security.Cryptography.RandomNumberGenerator.GetBytes`.
+ */
+class SecureRandomnessSource extends RandomnessSource {
+  SecureRandomnessSource() {
+    this.getTarget()
+        .hasFullyQualifiedName("System.Security.Cryptography", "RandomNumberGenerator", "GetBytes")
+  }
+
+  override Expr getOutput() { result = this.getArgument(0) }
+}
+
+/**
+ * A call to `System.Random.NextBytes`.
+ */
+class InsecureRandomnessSource extends RandomnessSource {
+  InsecureRandomnessSource() {
+    this.getTarget().hasFullyQualifiedName("System", "Random", "NextBytes")
+  }
+
+  override Expr getOutput() { result = this.getArgument(0) }
+}
+
+/**
+ * An instance of random number generation, modelled as the expression
+ * tied to an output node (i.e., the RNG output)
+ */
+abstract class RandomnessInstance extends Crypto::RandomNumberGenerationInstance {
+  override DataFlow::Node getOutputNode() { result.asExpr() = this }
+}
+
+/**
+ * An output instance from the system cryptographically secure RNG.
+ */
+class SecureRandomnessInstance extends RandomnessInstance {
+  RandomnessSource source;
+
+  SecureRandomnessInstance() {
+    source instanceof SecureRandomnessSource and
+    this = source.getOutput()
   }
+
+  override string getGeneratorName() { result = source.getGenerator().getName() }
 }
 
-module ArtifactFlow = Language::DataFlow::Global<ArtifactFlowConfig>;
+/**
+ * An output instance from an insecure RNG.
+ */
+class InsecureRandomnessInstance extends RandomnessInstance {
+  RandomnessSource source;
+
+  InsecureRandomnessInstance() {
+    not source instanceof SecureRandomnessSource and
+    this = source.getOutput()
+  }
+
+  override string getGeneratorName() { result = source.getGenerator().getName() }
+}
 
 import dotnet

csharp/ql/lib/experimental/quantum/dotnet/FlowAnalysis.qll

@@ -1,5 +1,6 @@
 private import csharp
 private import semmle.code.csharp.dataflow.DataFlow
+private import experimental.quantum.Language
 private import OperationInstances
 private import AlgorithmValueConsumers
 private import Cryptography
@@ -51,15 +52,6 @@ module CreationToUseFlow<CreationCallSig Creation, UseCallSig Use> {
     sink.getNode().asExpr() = use.(QualifiableExpr).getQualifier() and
     CreationToUseFlow::flowPath(source, sink)
   }
-
-  // TODO: Remove this.
-  Expr flowsTo(Expr expr) {
-    exists(CreationToUseFlow::PathNode source, CreationToUseFlow::PathNode sink |
-      source.getNode().asExpr() = expr and
-      sink.getNode().asExpr() = result and
-      CreationToUseFlow::flowPath(source, sink)
-    )
-  }
 }
 
 /**
@@ -269,3 +261,20 @@ module StreamFlow {
     StreamFlow::flowPath(source, sink)
   }
 }
+
+/**
+ * An additional flow step across property assignments used to track flow from
+ * output artifacts to consumers.
+ *
+ * TODO: Figure out why this is needed.
+ */
+class PropertyWriteFlowStep extends AdditionalFlowInputStep {
+  Assignment assignment;
+
+  PropertyWriteFlowStep() {
+    this.asExpr() = assignment.getRValue() and
+    assignment.getLValue() instanceof PropertyWrite
+  }
+
+  override DataFlow::Node getOutput() { result.asExpr() = assignment.getLValue() }
+}