Merge pull request #9 from trailofbits/go-fix-dataflows-new

Go fix dataflows new
trailofbits · Jul 8, 2024 · ff37b35 · ff37b35
2 parents 23da6f6 + 37062f5
commit ff37b35
Show file tree

Hide file tree

Showing 4 changed files with 42 additions and 45 deletions.
diff --git a/go/src/crypto/MsgNotHashedBeforeSigVerfication/MsgNotHashedBeforeSigVerfication.ql b/go/src/crypto/MsgNotHashedBeforeSigVerfication/MsgNotHashedBeforeSigVerfication.ql
@@ -11,8 +11,6 @@
  */
 
 import go
-import DataFlow::PathGraph
-import semmle.go.dataflow.DataFlow2
 
 /**
  *  Function that performs signing or signature verification on a hash of a message
@@ -90,27 +88,25 @@ class HashFunction extends Function {
     }
 }
 
-class LongestFlowConfig extends DataFlow2::Configuration {
-    LongestFlowConfig() { this = "LongestFlowConfig" }
-    override predicate isSource(DataFlow::Node source) { source = source }
-    override predicate isSink(DataFlow::Node sink) { sink = sink }
+private module LongestFlowConfig implements DataFlow::ConfigSig {
+    predicate isSource(DataFlow::Node source) { source = source }
+    predicate isSink(DataFlow::Node sink) { sink = sink }
 }
+module LongestFlowFlow = DataFlow::Global<LongestFlowConfig>;
 
 /**
  *  Flows from anything to SignatureMsgTruncationFunction
  *  that do not cross a hash function or slicing expression
  */
-class AnythingToSignatureMsgTrunFuncFlow extends DataFlow::Configuration {
-    AnythingToSignatureMsgTrunFuncFlow() { this = "AnythingToSignatureMsgTrunFuncFlow" }
-
+module AnythingToSignatureMsgTrunFuncConfig implements DataFlow::ConfigSig {    
     // anything that is not a function's argument
     // TODO: alternatively, set sources to be ExternalInputs
-    override predicate isSource(DataFlow::Node source) {
-        not this.isSink(source, _)
+    predicate isSource(DataFlow::Node source) {
+        not isSink(source)
         and not source.asInstruction() instanceof IR::ReadArgumentInstruction
     }
 
-    override predicate isSink(DataFlow::Node sink) {
+    predicate isSink(DataFlow::Node sink) {
         exists(SignatureMsgTruncationFunction sigUseF, CallExpr sigUseCall, int position | 
             sigUseCall.getTarget() = sigUseF
             and sigUseF.hashArgPosition(position)
@@ -122,7 +118,7 @@ class AnythingToSignatureMsgTrunFuncFlow extends DataFlow::Configuration {
     // * data goes through a hash function
     // * data is truncated with a hardcoded value
     // * TODO: data is of type Hash
-    override predicate isBarrier(DataFlow::Node node) {
+    predicate isBarrier(DataFlow::Node node) {
         // direct hash function call
         exists(HashFunction hf | hf.getACall().getResult(_) = node or hf.getACall().getArgument(_) = node)
         or
@@ -142,14 +138,17 @@ class AnythingToSignatureMsgTrunFuncFlow extends DataFlow::Configuration {
         node.asExpr().getType().getUnderlyingType().(ArrayType).getLength() <= 66
     }
 }
+module AnythingToSignatureMsgTrunFuncFlow = DataFlow::Global<AnythingToSignatureMsgTrunFuncConfig>;
+import AnythingToSignatureMsgTrunFuncFlow::PathGraph
 
-from AnythingToSignatureMsgTrunFuncFlow config, DataFlow::PathNode source, DataFlow::PathNode sink
+from AnythingToSignatureMsgTrunFuncFlow::PathNode source, AnythingToSignatureMsgTrunFuncFlow::PathNode sink
 where
-    config.hasFlowPath(source, sink)
+    AnythingToSignatureMsgTrunFuncFlow::flowPath(source, sink)
 
     // only the longest flow
-    and not exists(LongestFlowConfig config2, DataFlow::Node source2 |
-        config2.hasFlow(source2, source.getNode())
+    // TODO: find only flows originating from user input
+    and not exists(DataFlow::Node source2 |
+        LongestFlowFlow::flow(source2, source.getNode())
         and source2 != source.getNode()
     )
 

diff --git a/go/src/security/MissingMinVersionTLS/MissingMinVersionTLS.ql b/go/src/security/MissingMinVersionTLS/MissingMinVersionTLS.ql
@@ -15,13 +15,11 @@ import go
 /**
  * Flow of a `tls.Config` to a write to the `MinVersion` field.
  */
-class TlsVersionFlowConfig extends TaintTracking::Configuration {
-    TlsVersionFlowConfig() { this = "TlsVersionFlowConfig" }
-
+module TlsVersionConfig implements DataFlow::ConfigSig {
     /**
      * Holds if `source` is a TLS.Config instance.
      */
-    override predicate isSource(DataFlow::Node source) {
+    predicate isSource(DataFlow::Node source) {
         exists(Variable v | 
             configOrConfigPointer(v.getType()) and 
             source.asExpr() = v.getAReference()
@@ -31,21 +29,21 @@ class TlsVersionFlowConfig extends TaintTracking::Configuration {
     /**
      * Holds if a write to `sink`.MinVersion exists.
      */
-    override predicate isSink(DataFlow::Node sink) {
+    predicate isSink(DataFlow::Node sink) {
       exists(Write fieldWrite, Field fld |
         fld.hasQualifiedName( "crypto/tls", "Config", "MinVersion") and
         fieldWrite.writesField(sink, fld, _)
       )
     }
 }
+module TlsVersionFlow = TaintTracking::Global<TlsVersionConfig>;
+
 
 /**
  * Flow of a `tls.Config` with `MinVersion` to a variable.
  */
-class TlsConfigCreation extends TaintTracking::Configuration {
-    TlsConfigCreation() { this = "TlsConfigCreation" }
-
-    predicate isSecure(DataFlow::Node source) {
+module TlsConfigCreationConfig implements DataFlow::ConfigSig {
+    additional predicate isSecure(DataFlow::Node source) {
         exists(StructLit lit, Field fld |
             lit.getType().hasQualifiedName("crypto/tls", "Config") and
             fld.hasQualifiedName("crypto/tls", "Config", "MinVersion") and
@@ -58,18 +56,19 @@ class TlsConfigCreation extends TaintTracking::Configuration {
     /**
      * Holds if `source` is a TLS.Config literal.
      */
-    override predicate isSource(DataFlow::Node source) {
+    predicate isSource(DataFlow::Node source) {
         exists(StructLit lit, Field fld |
             lit.getType().hasQualifiedName("crypto/tls", "Config") and
             fld.hasQualifiedName("crypto/tls", "Config", "MinVersion") and
             source.asExpr() = lit
         )
+        and not isSecure(source)
     }
 
     /**
      * Holds if it is TLS.Config instance (a Variable).
      */
-    override predicate isSink(DataFlow::Node sink) {
+    predicate isSink(DataFlow::Node sink) {
         exists(Variable v |
             sink.asExpr() = v.getAReference()
         )
@@ -78,10 +77,11 @@ class TlsConfigCreation extends TaintTracking::Configuration {
     /**
      * Holds if TLS.Config literal is saved in a structure's field
      */
-    override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
+    predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
           exists(Write w | w.writesField(succ, _, pred))
     }
 }
+module TlsConfigCreationFlow = TaintTracking::Global<TlsConfigCreationConfig>;
 
 /**
  * Holds if `t` is a TLS.Config type or a pointer to it (or ptr to ptr...) or a struct containing it.
@@ -104,14 +104,13 @@ predicate configOrConfigPointer(Type t) {
 }
 
 // v - a variable holding any structure which is or contains the tls.Config
-from StructLit configStruct, Variable v, TlsConfigCreation cfg, DataFlow::Node source, DataFlow::Node sink
+from StructLit configStruct, Variable v, DataFlow::Node source, DataFlow::Node sink
 where
     // find tls.Config structures with MinVersion not set on the structure initialization
     (
-        cfg.hasFlow(source, sink) and
+        TlsConfigCreationFlow::flow(source, sink) and
         sink.asExpr() = v.getAReference() and
-        source.asExpr() = configStruct and
-        not cfg.isSecure(source)
+        source.asExpr() = configStruct
     )
 
     // exclude if tls.Config is used as TLSClientConfig, as default for clients is TLS 1.2
@@ -143,8 +142,8 @@ where
     and if configOrConfigPointer(v.getType()) then
     (   
         // exclude if there is a later write to MinVersion
-        not exists(TlsVersionFlowConfig cfg2, DataFlow::Node source2, DataFlow::Node sink2 | 
-            cfg2.hasFlow(source2, sink2) and
+        not exists(DataFlow::Node source2, DataFlow::Node sink2 | 
+            TlsVersionFlow::flow(source2, sink2) and
             source2.asExpr() = v.getAReference()
         )
     ) else

diff --git a/go/src/security/TrimMisuse/TrimMisuse.ql b/go/src/security/TrimMisuse/TrimMisuse.ql
@@ -11,24 +11,23 @@
  */
 
 import go
-import DataFlow
+import DataFlow2
 
 /*
  * Flows from a string to TrimFamilyCall cutSet argument
  */
-class Trim2ndArg extends DataFlow::Configuration {
-  Trim2ndArg() { this = "Trim2ndArg" }
-
-  override predicate isSource(DataFlow::Node source) {
+module Trim2ndArgConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) {
     source.asExpr() instanceof StringLit
   }
 
-  override predicate isSink(DataFlow::Node sink) {
+  predicate isSink(DataFlow::Node sink) {
     exists(TrimFamilyCall trimCall | 
       sink.asExpr() = trimCall.getCutSetArg()
     )
   }
 }
+module Trim2ndArgFlow = DataFlow::Global<Trim2ndArgConfig>;
 
 /*
  * Calls to Trim methods that we are interested in
@@ -49,8 +48,8 @@ class TrimFamilyCall extends CallNode {
 from TrimFamilyCall trimCall, StringLit cutset
 where
   // get 2nd argument value, if possible
-  exists(Trim2ndArg config, DataFlow::Node source, DataFlow::Node sink |
-    config.hasFlow(source, sink)
+  exists(DataFlow::Node source, DataFlow::Node sink |
+    Trim2ndArgFlow::flow(source, sink)
     and source.asExpr() = cutset
     and sink.asExpr() = trimCall.getCutSetArg()
   )

diff --git a/...y-tests/crypto/MsgNotHashedBeforeSigVerfication/MsgNotHashedBeforeSigVerfication.expected b/...y-tests/crypto/MsgNotHashedBeforeSigVerfication/MsgNotHashedBeforeSigVerfication.expected
@@ -1,6 +1,6 @@
 edges
-| MsgNotHashedBeforeSigVerfication.go:78:33:78:46 | "test message" | MsgNotHashedBeforeSigVerfication.go:78:26:78:47 | type conversion |
-| MsgNotHashedBeforeSigVerfication.go:86:31:86:44 | "test message" | MsgNotHashedBeforeSigVerfication.go:86:24:86:45 | type conversion |
+| MsgNotHashedBeforeSigVerfication.go:78:33:78:46 | "test message" | MsgNotHashedBeforeSigVerfication.go:78:26:78:47 | type conversion | provenance |  |
+| MsgNotHashedBeforeSigVerfication.go:86:31:86:44 | "test message" | MsgNotHashedBeforeSigVerfication.go:86:24:86:45 | type conversion | provenance |  |
 nodes
 | MsgNotHashedBeforeSigVerfication.go:78:26:78:47 | type conversion | semmle.label | type conversion |
 | MsgNotHashedBeforeSigVerfication.go:78:33:78:46 | "test message" | semmle.label | "test message" |