Address review comments 2

Author: owen-mc

ql/src/Security/CWE-681/IncorrectIntegerConversion.ql

@@ -1,7 +1,8 @@
 /**
  * @name Incorrect conversion between integer types
- * @description Converting the result of strconv.Atoi, strconv.ParseInt and strconv.ParseUint
- *              to integer types of smaller bit size can produce unexpected values.
+ * @description Converting the result of `strconv.Atoi`, `strconv.ParseInt`,
+ *              and `strconv.ParseUint` to integer types of smaller bit size
+ *              can produce unexpected values.
  * @kind path-problem
  * @problem.severity warning
  * @id go/incorrect-integer-conversion
@@ -19,7 +20,7 @@ import DataFlow::PathGraph
  * is true, unsigned otherwise) with `bitSize` bits.
  */
 float getMaxIntValue(int bitSize, boolean isSigned) {
-  bitSize in [8, 16, 32, 64] and
+  bitSize in [8, 16, 32] and
   (
     isSigned = true and result = 2.pow(bitSize - 1) - 1
     or
@@ -33,15 +34,15 @@ float getMaxIntValue(int bitSize, boolean isSigned) {
  * architecture-dependent.
  */
 private predicate isIncorrectIntegerConversion(int sourceBitSize, int sinkBitSize) {
-  sourceBitSize in [0, 16, 32, 64] and
-  sinkBitSize in [0, 8, 16, 32] and
-  not (sourceBitSize = 0 and sinkBitSize = 0) and
-  exists(int source, int sink |
-    (if sourceBitSize = 0 then source = 64 else source = sourceBitSize) and
-    if sinkBitSize = 0 then sink = 32 else sink = sinkBitSize
-  |
-    source > sink
-  )
+  sourceBitSize in [16, 32, 64] and
+  sinkBitSize in [8, 16, 32] and
+  sourceBitSize > sinkBitSize
+  or
+  sourceBitSize = 0 and
+  sinkBitSize in [8, 16, 32]
+  or
+  sourceBitSize = 64 and
+  sinkBitSize = 0
 }
 
 /**
@@ -57,15 +58,24 @@ class ConversionWithoutBoundsCheckConfig extends TaintTracking::Configuration {
   ConversionWithoutBoundsCheckConfig() {
     sourceIsSigned in [true, false] and
     isIncorrectIntegerConversion(sourceBitSize, sinkBitSize) and
-    this =
-      sourceBitSize.toString() + sourceIsSigned.toString() + sinkBitSize.toString() +
-        "ConversionWithoutBoundsCheckConfig"
+    this = "ConversionWithoutBoundsCheckConfig" + sourceBitSize + sourceIsSigned + sinkBitSize
   }
 
+  int getSourceBitSize() { result = sourceBitSize }
+
   override predicate isSource(DataFlow::Node source) {
-    exists(ParserCall pc, int bitSize | source = pc.getResult(0) |
-      (if pc.targetIsSigned() then sourceIsSigned = true else sourceIsSigned = false) and
-      (if pc.getTargetBitSize() = 0 then bitSize = 0 else bitSize = pc.getTargetBitSize()) and
+    exists(DataFlow::CallNode c, IntegerParser::Range ip, int bitSize |
+      c.getTarget() = ip and source = c.getResult(0)
+    |
+      (
+        if ip.getResultType(0) instanceof SignedIntegerType
+        then sourceIsSigned = true
+        else sourceIsSigned = false
+      ) and
+      (
+        bitSize = ip.getTargetBitSize() or
+        bitSize = ip.getTargetBitSizeInput().getNode(c).getIntValue()
+      ) and
       // `bitSize` could be any value between 0 and 64, but we can round
       // it up to the nearest size of an integer type without changing
       // behaviour.
@@ -76,16 +86,14 @@ class ConversionWithoutBoundsCheckConfig extends TaintTracking::Configuration {
   /**
    * Holds if `sink` is a typecast to an integer type with size `bitSize` (where
    * 0 represents architecture-dependent) and the expression being typecast is
-   * not also in a right-shift expression.
+   * not also in a right-shift expression. We allow this case because it is
+   * a common pattern to serialise `byte(v)`, `byte(v >> 8)`, and so on.
    */
   predicate isSink(DataFlow::TypeCastNode sink, int bitSize) {
     exists(IntegerType integerType | sink.getType().getUnderlyingType() = integerType |
       bitSize = integerType.getSize()
       or
-      (
-        integerType instanceof IntType or
-        integerType instanceof UintType
-      ) and
+      not exists(integerType.getSize()) and
       bitSize = 0
     ) and
     not exists(ShrExpr shrExpr |
@@ -131,12 +139,18 @@ class UpperBoundCheckGuard extends DataFlow::BarrierGuard, DataFlow::RelationalC
   }
 }
 
+/** Gets a string describing the size of the integer parsed. */
+string describeBitSize(int bitSize) {
+  if bitSize != 0
+  then bitSize in [8, 16, 32, 64] and result = "a " + bitSize + "-bit integer"
+  else result = "an integer with architecture-dependent bit size"
+}
+
 from
   DataFlow::PathNode source, DataFlow::PathNode sink, ConversionWithoutBoundsCheckConfig cfg,
-  ParserCall pc
-where cfg.hasFlowPath(source, sink) and pc.getResult(0) = source.getNode()
+  DataFlow::CallNode call
+where cfg.hasFlowPath(source, sink) and call.getResult(0) = source.getNode()
 select source.getNode(), source, sink,
-  "Incorrect conversion of " + pc.getBitSizeString() + " from " + pc.getParserName() +
-    " to a lower bit size type " +
-    sink.getNode().(DataFlow::TypeCastNode).getType().getUnderlyingType().getName() +
-    " without an upper bound check."
+  "Incorrect conversion of " + describeBitSize(cfg.getSourceBitSize()) + " from " +
+    call.getTarget().getQualifiedName() + " to a lower bit size type " +
+    sink.getNode().getType().getUnderlyingType().getName() + " without an upper bound check."

ql/src/semmle/go/frameworks/Stdlib.qll

@@ -507,119 +507,65 @@ module Path {
   }
 }
 
+/** Provides a class for modeling functions which convert strings into integers. */
+module IntegerParser {
+  /**
+   * A function that converts strings into integers.
+   *
+   * Extend this class to model new APIs. If you want to refine existing API models,
+   * extend `IntegerParser` instead.
+   */
+  abstract class Range extends Function {
+    /**
+     * Gets the maximum bit size of the return value, if this makes
+     * sense, where 0 represents the bit size of `int` and `uint`.
+     */
+    int getTargetBitSize() { none() }
+
+    /**
+     * Gets the `FunctionInput` containing the maximum bit size of the
+     * return value, if this makes sense, where 0 represents the bit
+     * size of `int` and `uint`.
+     */
+    FunctionInput getTargetBitSizeInput() { none() }
+  }
+}
+
 /**
  * Provides classes for some functions in the `strconv` package for
  * converting strings to numbers.
  */
 module StrConv {
-  /** A function that parses integers. */
-  class Atoi extends Function {
+  /** The `Atoi` function. */
+  class Atoi extends IntegerParser::Range {
     Atoi() { this.hasQualifiedName("strconv", "Atoi") }
-  }
 
-  /** A function that parses floating-point numbers. */
-  class ParseFloat extends Function {
-    ParseFloat() { this.hasQualifiedName("strconv", "ParseFloat") }
+    override int getTargetBitSize() { result = 0 }
   }
 
-  /** A function that parses integers with a specifiable bit size. */
-  class ParseInt extends Function {
+  /** The `ParseInt` function. */
+  class ParseInt extends IntegerParser::Range {
     ParseInt() { this.hasQualifiedName("strconv", "ParseInt") }
+
+    override FunctionInput getTargetBitSizeInput() { result.isParameter(2) }
   }
 
-  /** A function that parses unsigned integers with a specifiable bit size. */
-  class ParseUint extends Function {
+  /** The `ParseUint` function. */
+  class ParseUint extends IntegerParser::Range {
     ParseUint() { this.hasQualifiedName("strconv", "ParseUint") }
+
+    override FunctionInput getTargetBitSizeInput() { result.isParameter(2) }
   }
 
   /**
-   * A constant that gives the size in bits of an `int` or `uint`
-   * value on the current architecture (32 or 64).
+   * The `IntSize` constant, that gives the size in bits of an `int` or
+   * `uint` value on the current architecture (32 or 64).
    */
   class IntSize extends DeclaredConstant {
     IntSize() { this.hasQualifiedName("strconv", "IntSize") }
   }
 }
 
-/** Provides a class for modeling calls to number-parsing functions. */
-module ParserCall {
-  /** A data-flow call node that parses a number. */
-  abstract class Range extends DataFlow::CallNode {
-    /** Gets the bit size of the type of the result number. */
-    abstract int getTargetBitSize();
-
-    /** Holds if the type of the result number is signed. */
-    abstract predicate targetIsSigned();
-
-    /** Gets the name of the parser function. */
-    abstract string getParserName();
-  }
-}
-
-/** A call to a number-parsing function. */
-class ParserCall extends DataFlow::CallNode {
-  ParserCall::Range self;
-
-  ParserCall() { this = self }
-
-  /** Gets the bit size of the type of the result number. */
-  int getTargetBitSize() { result = self.getTargetBitSize() }
-
-  /** Holds if the type of the result number is signed. */
-  predicate targetIsSigned() { self.targetIsSigned() }
-
-  /** Gets the name of the parser function. */
-  string getParserName() { result = self.getParserName() }
-
-  /** Gets a string describing the size of the integer parsed. */
-  string getBitSizeString() {
-    if getTargetBitSize() != 0
-    then result = "a " + getTargetBitSize() + "-bit integer"
-    else result = "an integer with architecture-dependent bit-width"
-  }
-}
-
-/** A call to `strconv.Atoi`. */
-class AtoiCall extends DataFlow::CallNode, ParserCall::Range {
-  AtoiCall() { exists(StrConv::Atoi atoi | this = atoi.getACall()) }
-
-  override int getTargetBitSize() { result = 0 }
-
-  override predicate targetIsSigned() { any() }
-
-  override string getParserName() { result = "strconv.Atoi" }
-}
-
-/** A call to `strconv.ParseInt`. */
-class ParseIntCall extends DataFlow::CallNode, ParserCall::Range {
-  ParseIntCall() { exists(StrConv::ParseInt parseInt | this = parseInt.getACall()) }
-
-  override int getTargetBitSize() {
-    if exists(StrConv::IntSize intSize | this.getArgument(2).(DataFlow::ReadNode).reads(intSize))
-    then result = 0
-    else result = this.getArgument(2).getIntValue()
-  }
-
-  override predicate targetIsSigned() { any() }
-
-  override string getParserName() { result = "strconv.ParseInt" }
-}
-
-/** A call to `strconv.ParseUint`. */
-class ParseUintCall extends DataFlow::CallNode, ParserCall::Range {
-  ParseUintCall() { exists(StrConv::ParseUint parseUint | this = parseUint.getACall()) }
-
-  override int getTargetBitSize() {
-    if exists(StrConv::IntSize intSize | this.getArgument(2).(DataFlow::ReadNode).reads(intSize))
-    then result = 0
-    else result = this.getArgument(2).getIntValue()
-  }
-
-  override predicate targetIsSigned() { none() }
-
-  override string getParserName() { result = "strconv.ParseUint" }
-}
-
 /** Provides models of commonly used functions in the `strings` package. */
 module Strings {
   /** The `Join` function. */