WIP refactor #2

owen-mc · owen-mc · commit 38c9c184e0e4 · 2023-08-11T10:52:27.000+01:00
diff --git a/go/ql/lib/semmle/go/security/IncorrectIntegerConversionLib.qll b/go/ql/lib/semmle/go/security/IncorrectIntegerConversionLib.qll
@@ -151,12 +151,31 @@ deprecated class ConversionWithoutBoundsCheckConfig extends TaintTracking::Confi
   }
 }
 
+private int validBitsize() { result = [7, 8, 15, 16, 31, 32, 63, 64] }
+
+private newtype TMaxValueState =
+  TMkConcrete(int bitSize) { bitSize = validBitsize() } or
+  TMkArchitectureDependent(boolean isSigned) { isSigned = [true, false] }
+
 /** Flow state for ConversionWithoutBoundsCheckConfig. */
-newtype IntegerConversionFlowState =
-  TFlowstate(int bitSize, boolean isSigned) {
-    bitSize = [0, 8, 16, 32, 64] and
-    isSigned = [true, false]
+private class MaxValueState extends TMaxValueState {
+  /**
+   * Gets the smallest valid bitsize `result` where the maximum value that
+   * could have been parsed fits into an integer type whose maximum value is
+   * 2^(result) - 1.
+   */
+  int getConcreteBitsize() { this = TMkConcrete(result) }
+
+  /** Gets the signedness of an architecture-dependent flow state. */
+  boolean getArchictureDependentSignedness() { this = TMkArchitectureDependent(result) }
+
+  string toString() {
+    result = "Concrete MaxValueState(max value <= 2^(" + this.getConcreteBitsize() + ")-1)" or
+    result =
+      "Architecture-dependent MaxValueState(signed = " + this.getArchictureDependentSignedness() +
+        ")"
   }
+}
 
 /**
  * The integer type with bit size `bitSize` and signedness `isSigned` has
@@ -172,21 +191,13 @@ private int f(int bitSize, boolean isSigned, int bitSizeForZero) {
 }
 
 private module ConversionWithoutBoundsCheckConfig implements DataFlow::StateConfigSig {
-  class FlowState extends IntegerConversionFlowState {
-    /** Gets the signedness of the flow state. */
-    boolean getIsSigned() { this = TFlowstate(_, result) }
-
-    /** Gets the bit size of the flow state. */
-    int getBitSize() { this = TFlowstate(result, _) }
-
-    string toString() { result = "FlowState(" + this.getBitSize() + "," + this.getIsSigned() + ")" }
-  }
+  class FlowState = MaxValueState;
 
   predicate isSource(DataFlow::Node source, FlowState state) {
     exists(
       DataFlow::CallNode c, IntegerParser::Range ip, int apparentBitSize, int effectiveBitSize
     |
-      c.getTarget() = ip and
+      c = ip.getACall() and
       source = c.getResult(0) and
       (
         apparentBitSize = ip.getTargetBitSize()
@@ -208,8 +219,8 @@ private module ConversionWithoutBoundsCheckConfig implements DataFlow::StateConf
       // if assigned the maximum value that can be parsed by `ip`. If both the
       // source and the sink are `int` or `uint` then overflow can only happen
       // if we are converting from `uint` to `int`.
-      if state.getBitSize() = 0 and effectiveBitSize = 0
-      then ip.isSigned() = false and state.getIsSigned() = true
+      if effectiveBitSize = 0
+      then state.getArchictureDependentSignedness() = ip.isSigned()
       else
         // The maximum value for a signed type with n bits is 2^(n - 1) - 1.
         // The maximum value for an unsigned type with n bits is 2^n - 1. To
@@ -219,7 +230,13 @@ private module ConversionWithoutBoundsCheckConfig implements DataFlow::StateConf
         // catch that flow from `int64` to `int` is incorrect on a 32-bit
         // architecture), and we have already dealt with the case that both
         // source and sink are `int` or `uint`.
-        f(state.getBitSize(), state.getIsSigned(), 32) < f(effectiveBitSize, ip.isSigned(), 64)
+        state.getConcreteBitsize() =
+          min(int bitsize |
+            bitsize = validBitsize() and
+            f(effectiveBitSize, ip.isSigned(), 64) <= state.getConcreteBitsize()
+          |
+            bitsize
+          )
     )
   }
 
