Merge branch 'tob/quantum-csharp' of github.com:trailofbits/codeql into tob/quantum-csharp

Author: fcasal

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

@@ -72,6 +72,10 @@ class HashAlgorithmNameInstance extends Crypto::HashAlgorithmInstance instanceof
 
 class SymmetricAlgorithmInstance extends Crypto::KeyOperationAlgorithmInstance instanceof SymmetricAlgorithmCreation
 {
+  SymmetricAlgorithmConsumer consumer;
+
+  SymmetricAlgorithmInstance() { consumer = SymmetricAlgorithmFlow::getUseFromCreation(this, _, _) }
+
   override string getRawAlgorithmName() { result = super.getSymmetricAlgorithm().getName() }
 
   override Crypto::KeyOpAlg::Algorithm getAlgorithmType() {
@@ -80,13 +84,33 @@ class SymmetricAlgorithmInstance extends Crypto::KeyOperationAlgorithmInstance i
     else result = Crypto::KeyOpAlg::TSymmetricCipher(Crypto::KeyOpAlg::OtherSymmetricCipherType())
   }
 
-  override Crypto::ModeOfOperationAlgorithmInstance getModeOfOperationAlgorithm() { none() }
+  // The cipher mode is set by assigning it to the `Mode` property of the
+  // symmetric algorithm.
+  override Crypto::ModeOfOperationAlgorithmInstance getModeOfOperationAlgorithm() {
+    result.(CipherModeLiteralInstance).getConsumer() = this.getCipherModeAlgorithmValueConsumer()
+  }
 
-  override Crypto::PaddingAlgorithmInstance getPaddingAlgorithm() { none() }
+  // The padding mode is set by assigning it to the `Padding` property of the
+  // symmetric algorithm.
+  override Crypto::PaddingAlgorithmInstance getPaddingAlgorithm() {
+    result.(PaddingModeLiteralInstance).getConsumer() = this.getPaddingAlgorithmValueConsumer()
+  }
+
+  Crypto::AlgorithmValueConsumer getPaddingAlgorithmValueConsumer() {
+    result = SymmetricAlgorithmFlow::getUseFromCreation(this, _, _) and
+    result instanceof PaddingPropertyWrite
+  }
+
+  Crypto::AlgorithmValueConsumer getCipherModeAlgorithmValueConsumer() {
+    result = SymmetricAlgorithmFlow::getUseFromCreation(this, _, _) and
+    result instanceof CipherModePropertyWrite
+  }
 
   override int getKeySizeFixed() { none() }
 
   override Crypto::ConsumerInputDataFlowNode getKeySizeConsumer() { none() }
+
+  Crypto::AlgorithmValueConsumer getConsumer() { result = consumer }
 }
 
 /**
@@ -98,7 +122,7 @@ class PaddingModeLiteralInstance extends Crypto::PaddingAlgorithmInstance instan
 
   PaddingModeLiteralInstance() {
     this = any(PaddingMode mode).getAnAccess() and
-    consumer = PaddingModeLiteralFlow::getConsumer(this, _, _)
+    consumer = ModeLiteralFlow::getConsumer(this, _, _)
   }
 
   override string getRawPaddingAlgorithmName() { result = super.getTarget().getName() }
@@ -112,6 +136,29 @@ class PaddingModeLiteralInstance extends Crypto::PaddingAlgorithmInstance instan
   Crypto::AlgorithmValueConsumer getConsumer() { result = consumer }
 }
 
+/**
+ * A padding mode literal, such as `PaddingMode.PKCS7`.
+ */
+class CipherModeLiteralInstance extends Crypto::ModeOfOperationAlgorithmInstance instanceof MemberConstantAccess
+{
+  Crypto::AlgorithmValueConsumer consumer;
+
+  CipherModeLiteralInstance() {
+    this = any(CipherMode mode).getAnAccess() and
+    consumer = ModeLiteralFlow::getConsumer(this, _, _)
+  }
+
+  override string getRawModeAlgorithmName() { result = super.getTarget().getName() }
+
+  override Crypto::TBlockCipherModeOfOperationType getModeType() {
+    if exists(modeNameToType(this.getRawModeAlgorithmName()))
+    then result = modeNameToType(this.getRawModeAlgorithmName())
+    else result = Crypto::OtherMode()
+  }
+
+  Crypto::AlgorithmValueConsumer getConsumer() { result = consumer }
+}
+
 private Crypto::KeyOpAlg::Algorithm symmetricAlgorithmNameToType(string algorithmName) {
   algorithmName = "Aes" and result = Crypto::KeyOpAlg::TSymmetricCipher(Crypto::KeyOpAlg::AES())
   or
@@ -133,3 +180,13 @@ private Crypto::TPaddingType paddingNameToType(string paddingName) {
   or
   paddingName = "PKCS7" and result = Crypto::PKCS7()
 }
+
+private Crypto::TBlockCipherModeOfOperationType modeNameToType(string modeName) {
+  modeName = "CBC" and result = Crypto::CBC()
+  or
+  modeName = "CFB" and result = Crypto::CFB()
+  or
+  modeName = "ECB" and result = Crypto::ECB()
+  or
+  modeName = "OFB" and result = Crypto::OFB()
+}

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

@@ -40,3 +40,32 @@ class PaddingPropertyWrite extends Crypto::AlgorithmValueConsumer instanceof Sym
     result.(PaddingModeLiteralInstance).getConsumer() = this
   }
 }
+
+/**
+ * A write access to the `Mode` property of a `SymmetricAlgorithm` instance.
+ */
+class CipherModePropertyWrite extends Crypto::AlgorithmValueConsumer instanceof SymmetricAlgorithmUse
+{
+  CipherModePropertyWrite() { super.isModeConsumer() }
+
+  override Crypto::ConsumerInputDataFlowNode getInputNode() { result.asExpr() = this }
+
+  override Crypto::AlgorithmInstance getAKnownAlgorithmSource() {
+    result.(CipherModeLiteralInstance).getConsumer() = this
+  }
+}
+
+/**
+ * A call to a `SymmetricAlgorithm.CreateEncryptor` or `SymmetricAlgorithm.CreateDecryptor`
+ * method that returns a `CryptoTransform` instance.
+ */
+class SymmetricAlgorithmConsumer extends Crypto::AlgorithmValueConsumer instanceof CryptoTransformCreation
+{
+  override Crypto::ConsumerInputDataFlowNode getInputNode() {
+    result.asExpr() = super.getQualifier()
+  }
+
+  override Crypto::AlgorithmInstance getAKnownAlgorithmSource() {
+    result.(SymmetricAlgorithmInstance).getConsumer() = this
+  }
+}

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

@@ -31,15 +31,15 @@ module CreationToUseFlow<CreationCallSig Creation, UseCallSig Use> {
     Use use, CreationToUseFlow::PathNode source, CreationToUseFlow::PathNode sink
   ) {
     source.getNode().asExpr() = result and
-    sink.getNode().asExpr() = use.(MethodCall).getQualifier() and
+    sink.getNode().asExpr() = use.(QualifiableExpr).getQualifier() and
     CreationToUseFlow::flowPath(source, sink)
   }
 
   Use getUseFromCreation(
     Creation creation, CreationToUseFlow::PathNode source, CreationToUseFlow::PathNode sink
   ) {
     source.getNode().asExpr() = creation and
-    sink.getNode().asExpr() = result.(MethodCall).getQualifier() and
+    sink.getNode().asExpr() = result.(QualifiableExpr).getQualifier() and
     CreationToUseFlow::flowPath(source, sink)
   }
 
@@ -145,7 +145,9 @@ module CryptoTransformFlow {
 /**
  * A flow analysis module that tracks the flow from a `PaddingMode` member
  * access (e.g. `PaddingMode.PKCS7`) to a `Padding` property write on a
- * `SymmetricAlgorithm` instance.
+ * `SymmetricAlgorithm` instance, or from a `CipherMode` member access
+ * (e.g. `CipherMode.CBC`) to a `Mode` property write on a `SymmetricAlgorithm`
+ * instance.
  *
  * Example:
  * ```
@@ -154,13 +156,18 @@ module CryptoTransformFlow {
  *  ...
  * ```
  */
-module PaddingModeLiteralFlow {
-  private module PaddingModeLiteralConfig implements DataFlow::ConfigSig {
+module ModeLiteralFlow {
+  private module ModeLiteralConfig implements DataFlow::ConfigSig {
     predicate isSource(DataFlow::Node source) {
       source.asExpr() = any(PaddingMode mode).getAnAccess()
+      or
+      source.asExpr() = any(CipherMode mode).getAnAccess()
     }
 
-    predicate isSink(DataFlow::Node sink) { sink.asExpr() instanceof PaddingPropertyWrite }
+    predicate isSink(DataFlow::Node sink) {
+      sink.asExpr() instanceof PaddingPropertyWrite or
+      sink.asExpr() instanceof CipherModePropertyWrite
+    }
 
     // TODO: Figure out why this is needed.
     predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
@@ -171,51 +178,96 @@ module PaddingModeLiteralFlow {
     }
   }
 
-  private module PaddingModeLiteralFlow = DataFlow::Global<PaddingModeLiteralConfig>;
+  private module ModeLiteralFlow = DataFlow::Global<ModeLiteralConfig>;
 
   SymmetricAlgorithmUse getConsumer(
-    Expr mode, PaddingModeLiteralFlow::PathNode source, PaddingModeLiteralFlow::PathNode sink
+    Expr mode, ModeLiteralFlow::PathNode source, ModeLiteralFlow::PathNode sink
   ) {
     source.getNode().asExpr() = mode and
     sink.getNode().asExpr() = result and
-    PaddingModeLiteralFlow::flowPath(source, sink)
+    ModeLiteralFlow::flowPath(source, sink)
   }
 }
 
 /**
- * A flow analysis module that tracks the flow from a `MemoryStream` object
- * creation to the `stream` argument passed to a `CryptoStream` constructor
- * call.
+ * A flow analysis module that tracks the flow from an arbitrary `Stream` object
+ * creation to the creation of a second `Stream` object wrapping the first one.
  *
- * TODO: This should probably be made generic over multiple stream types.
+ * This is useful for tracking the flow of data from a buffer passed to a
+ * `MemoryStream` to a `CryptoStream` wrapping the original `MemoryStream`. It
+ * can also be used to track dataflow from a `Stream` object to a call to
+ * `ToArray()` on the stream, or a wrapped stream.
  */
-module MemoryStreamFlow {
-  private class MemoryStreamCreation extends ObjectCreation {
-    MemoryStreamCreation() {
-      this.getObjectType().hasFullyQualifiedName("System.IO", "MemoryStream")
+module StreamFlow {
+  private class Stream extends Class {
+    Stream() { this.getABaseType().hasFullyQualifiedName("System.IO", "Stream") }
+  }
+
+  /**
+   * A `Stream` object creation.
+   */
+  private class StreamCreation extends ObjectCreation {
+    StreamCreation() { this.getObjectType() instanceof Stream }
+
+    Expr getInputArg() {
+      result = this.getAnArgument() and
+      result.getType().hasFullyQualifiedName("System", "Byte[]")
+    }
+
+    Expr getStreamArg() {
+      result = this.getAnArgument() and
+      result.getType() instanceof Stream
+    }
+  }
+
+  private class StreamUse extends MethodCall {
+    StreamUse() {
+      this.getQualifier().getType() instanceof Stream and
+      this.getTarget().hasName("ToArray")
     }
 
-    Expr getBufferArg() { result = this.getArgument(0) }
+    Expr getOutput() { result = this }
   }
 
-  // (Note that we cannot use `CreationToUseFlow` here, because the use is not a
-  // `QualifiableExpr`.)
-  private module MemoryStreamConfig implements DataFlow::ConfigSig {
-    predicate isSource(DataFlow::Node source) { source.asExpr() instanceof MemoryStreamCreation }
+  private module StreamConfig implements DataFlow::ConfigSig {
+    predicate isSource(DataFlow::Node source) { source.asExpr() instanceof StreamCreation }
 
     predicate isSink(DataFlow::Node sink) {
-      exists(CryptoStreamCreation creation | sink.asExpr() = creation.getStreamArg())
+      sink.asExpr() instanceof StreamCreation
+      or
+      exists(StreamUse use | sink.asExpr() = use.getQualifier())
+    }
+
+    predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
+      // Allow flow from one stream wrapped by a second stream.
+      exists(StreamCreation creation |
+        node1.asExpr() = creation.getStreamArg() and
+        node2.asExpr() = creation
+      )
+      or
+      exists(MethodCall copy |
+        node1.asExpr() = copy.getQualifier() and
+        node2.asExpr() = copy.getAnArgument() and
+        copy.getTarget().hasName("CopyTo")
+      )
     }
   }
 
-  private module MemoryStreamFlow = DataFlow::Global<MemoryStreamConfig>;
+  private module StreamFlow = DataFlow::Global<StreamConfig>;
 
-  MemoryStreamCreation getCreationFromUse(
-    CryptoStreamCreation creation, MemoryStreamFlow::PathNode source,
-    MemoryStreamFlow::PathNode sink
+  StreamCreation getWrappedStream(
+    StreamCreation stream, StreamFlow::PathNode source, StreamFlow::PathNode sink
   ) {
     source.getNode().asExpr() = result and
-    sink.getNode().asExpr() = creation.getStreamArg() and
-    MemoryStreamFlow::flowPath(source, sink)
+    sink.getNode().asExpr() = stream and
+    StreamFlow::flowPath(source, sink)
+  }
+
+  StreamUse getStreamUse(
+    StreamCreation stream, StreamFlow::PathNode source, StreamFlow::PathNode sink
+  ) {
+    source.getNode().asExpr() = stream and
+    sink.getNode().asExpr() = result.getQualifier() and
+    StreamFlow::flowPath(source, sink)
   }
 }

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

@@ -88,7 +88,7 @@ class SymmetricAlgorithmUse extends QualifiableExpr {
   SymmetricAlgorithmUse() {
     this.getQualifier().getType() instanceof SymmetricAlgorithm and
     this.getQualifiedDeclaration()
-        .hasName(["CreateEncryptor", "CreateDecryptor", "Key", "IV", "Padding"])
+        .hasName(["CreateEncryptor", "CreateDecryptor", "Key", "IV", "Padding", "Mode"])
   }
 
   Expr getSymmetricAlgorithm() { result = this.getQualifier() }
@@ -111,6 +111,11 @@ class SymmetricAlgorithmUse extends QualifiableExpr {
   predicate isPaddingConsumer() {
     this instanceof PropertyWrite and this.getQualifiedDeclaration().getName() = "Padding"
   }
+
+  // The cipher mode may be set by assigning it to the `Mode` property of the symmetric algorithm.
+  predicate isModeConsumer() {
+    this instanceof PropertyWrite and this.getQualifiedDeclaration().getName() = "Mode"
+  }
 }
 
 module SymmetricAlgorithmFlow =
@@ -162,6 +167,12 @@ class PaddingMode extends MemberConstant {
   }
 }
 
+class CipherMode extends MemberConstant {
+  CipherMode() {
+    this.getDeclaringType().hasFullyQualifiedName("System.Security.Cryptography", "CipherMode")
+  }
+}
+
 class CryptoStreamCreation extends ObjectCreation {
   CryptoStreamCreation() { this.getObjectType() instanceof CryptoStream }
 
@@ -211,7 +222,7 @@ class CryptoStreamOperationInstance extends Crypto::KeyOperationInstance instanc
     (transform.isEncryptor() or transform.isDecryptor())
   }
 
-  override Crypto::AlgorithmValueConsumer getAnAlgorithmValueConsumer() { none() }
+  override Crypto::AlgorithmValueConsumer getAnAlgorithmValueConsumer() { result = transform }
 
   override Crypto::KeyOperationSubtype getKeyOperationSubtype() {
     if transform.isEncryptor()
@@ -246,14 +257,21 @@ class CryptoStreamOperationInstance extends Crypto::KeyOperationInstance instanc
     )
   }
 
-  // Inputs to the operation can be passed either through the stream argument
-  // when the `CryptoStream` is created, or through calls to
-  // `CryptoStream.Write()`. If the input is passed through the stream argument,
-  // it is wrapped using a `MemoryStream` object.
+  // Inputs can be passed either through the `stream` argument when the
+  // `CryptoStream` is created, or through calls to `Write()` on the
+  // `CryptoStream` object.
   override Crypto::ConsumerInputDataFlowNode getInputConsumer() {
-    result.asExpr() = MemoryStreamFlow::getCreationFromUse(this, _, _).getBufferArg() or
+    result.asExpr() = StreamFlow::getWrappedStream(this, _, _).getInputArg() or
     result.asExpr() = CryptoStreamFlow::getUseFromCreation(this, _, _).getInputArg()
   }
 
-  override Crypto::ArtifactOutputDataFlowNode getOutputArtifact() { none() }
+  // The output is obtained by calling `ToArray()` on a `Stream` either wrapped
+  // by the `CryptoStream` object, or copied from the `CryptoStream` object.
+  override Crypto::ArtifactOutputDataFlowNode getOutputArtifact() {
+    // We perform backwards dataflow to identify stream objects that are wrapped
+    // by the `CryptoStream` object, and then we look for calls to `ToArray()`
+    // on those streams.
+    result.asExpr() =
+      StreamFlow::getStreamUse(any(StreamFlow::getWrappedStream(this, _, _)), _, _).getOutput()
+  }
 }