Deal with build constraints

Note that build constraints can be explicit (comments at the top of the
file) or implicit (part of the file name)

Author: owen-mc

Makefile

@@ -107,6 +107,7 @@ ql/src/go.dbscheme.stats: ql/src/go.dbscheme build/stats/src.stamp extractor
 
 test: all build/testdb/check-upgrade-path
 	codeql test run ql/test --search-path .
+	env GOARCH=386 codeql$(EXE) test run ql/test/query-tests/Security/CWE-681 --search-path .
 	cd extractor; go test -mod=vendor ./... | grep -vF "[no test files]"
 
 .PHONY: build/testdb/check-upgrade-path

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

@@ -28,6 +28,19 @@ float getMaxIntValue(int bitSize, boolean isSigned) {
   )
 }
 
+/**
+ * Get the size of `int` or `uint` in `file`, or 0 if it is
+ * architecture-specific.
+ */
+int getIntTypeBitSize(File file) {
+  if file.hasConstrainedIntBitSize(32)
+  then result = 32
+  else
+    if file.hasConstrainedIntBitSize(64)
+    then result = 64
+    else result = 0
+}
+
 /**
  * Holds if converting from an integer types with size `sourceBitSize` to
  * one with size `sinkBitSize` can produce unexpected values, where 0 means
@@ -74,7 +87,9 @@ class ConversionWithoutBoundsCheckConfig extends TaintTracking::Configuration {
   int getSourceBitSize() { result = sourceBitSize }
 
   override predicate isSource(DataFlow::Node source) {
-    exists(DataFlow::CallNode c, IntegerParser::Range ip, int bitSize |
+    exists(
+      DataFlow::CallNode c, IntegerParser::Range ip, int apparentBitSize, int effectiveBitSize
+    |
       c.getTarget() = ip and source = c.getResult(0)
     |
       (
@@ -83,20 +98,25 @@ class ConversionWithoutBoundsCheckConfig extends TaintTracking::Configuration {
         else sourceIsSigned = false
       ) and
       (
-        bitSize = ip.getTargetBitSize()
+        apparentBitSize = ip.getTargetBitSize()
         or
         // If we are reading a variable, check if it is
         // `strconv.IntSize`, and use 0 if it is.
         exists(DataFlow::Node rawBitSize | rawBitSize = ip.getTargetBitSizeInput().getNode(c) |
           if rawBitSize = any(StrConv::IntSize intSize).getARead()
-          then bitSize = 0
-          else bitSize = rawBitSize.getIntValue()
+          then apparentBitSize = 0
+          else apparentBitSize = rawBitSize.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.
-      sourceBitSize = min(int b | b in [0, 8, 16, 32, 64] and b >= bitSize)
+      (
+        if apparentBitSize = 0
+        then effectiveBitSize = getIntTypeBitSize(source.getFile())
+        else effectiveBitSize = apparentBitSize
+      ) and
+      // `effectiveBitSize` 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.
+      sourceBitSize = min(int b | b in [0, 8, 16, 32, 64] and b >= effectiveBitSize)
     )
   }
 
@@ -111,7 +131,7 @@ class ConversionWithoutBoundsCheckConfig extends TaintTracking::Configuration {
       bitSize = integerType.getSize()
       or
       not exists(integerType.getSize()) and
-      bitSize = 0
+      bitSize = getIntTypeBitSize(sink.getFile())
     ) and
     not exists(ShrExpr shrExpr |
       shrExpr.getLeftOperand().getGlobalValueNumber() =
@@ -161,17 +181,25 @@ class UpperBoundCheckGuard extends DataFlow::BarrierGuard, DataFlow::RelationalC
 }
 
 /** Gets a string describing the size of the integer parsed. */
-string describeBitSize(int bitSize) {
+string describeBitSize(int bitSize, int intTypeBitSize) {
+  intTypeBitSize in [0, 32, 64] and
   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"
+  else
+    if intTypeBitSize = 0
+    then result = "an integer with architecture-dependent bit size"
+    else
+      result =
+        "a number with architecture-dependent bit-width, which is constrained to be " +
+          intTypeBitSize + "-bit by build constraints,"
 }
 
 from
   DataFlow::PathNode source, DataFlow::PathNode sink, ConversionWithoutBoundsCheckConfig cfg,
   DataFlow::CallNode call
 where cfg.hasFlowPath(source, sink) and call.getResult(0) = source.getNode()
 select source.getNode(), source, sink,
-  "Incorrect conversion of " + describeBitSize(cfg.getSourceBitSize()) + " from " +
-    call.getTarget().getQualifiedName() + " to a lower bit size type " +
+  "Incorrect conversion of " +
+    describeBitSize(cfg.getSourceBitSize(), getIntTypeBitSize(source.getNode().getFile())) +
+    " from " + call.getTarget().getQualifiedName() + " to a lower bit size type " +
     sink.getNode().getType().getUnderlyingType().getName() + " without an upper bound check."

ql/src/semmle/go/Files.qll

@@ -203,6 +203,57 @@ class File extends Container, @file, Documentable, ExprParent, GoModExprParent,
   pragma[noinline]
   predicate hasBuildConstraints() { exists(BuildConstraintComment bc | this = bc.getFile()) }
 
+  /**
+   * Gets an architecture that is valid in a build constraint with bit
+   * size `bitSize`.
+   *
+   * Information obtained from
+   * https://github.com/golang/go/blob/98cbf45cfc6a5a50cc6ac2367f9572cb198b57c7/src/go/types/gccgosizes.go
+   * where the first field of the struct is 4 for 32-bit architectures
+   * and 8 for 64-bit architectures.
+   */
+  private string getAnArchitecture(int bitSize) {
+    bitSize = 32 and
+    result in ["386", "amd64p32", "arm", "armbe", "mips", "mipsle", "mips64p32", "mips64p32le",
+          "ppc", "s390", "sparc"]
+    or
+    bitSize = 64 and
+    result in ["amd64", "arm64", "arm64be", "ppc64", "ppc64le", "mips64", "mips64le", "s390x",
+          "sparc64"]
+  }
+
+  /**
+   * Holds if this file contains build constraints that ensure that it
+   * is only built on architectures of bit size `bitSize`.
+   */
+  predicate hasConstrainedIntBitSize(int bitSize) {
+    hasExplicitBuildConstraintsForArchitectures(bitSize) or
+    hasImplicitBuildConstraintForAnArchitecture(bitSize)
+  }
+
+  /**
+   * Holds if this file contains explicit build constraints that ensure
+   * that it is only built on an architecture of bit size `bitSize`.
+   */
+  predicate hasExplicitBuildConstraintsForArchitectures(int bitSize) {
+    exists(BuildConstraintComment bcc, string bc |
+      this = bcc.getFile() and bc = bcc.getText().splitAt("+build ", 1)
+    |
+      forex(string disjunct | disjunct = bc.splitAt(" ") |
+        disjunct.splitAt(",").matches(getAnArchitecture(bitSize))
+      )
+    )
+  }
+
+  /**
+   * Holds if this file has a name which acts as an implicit build
+   * constraint that ensures that it is only built on an
+   * architecture of bit size `bitSize`.
+   */
+  predicate hasImplicitBuildConstraintForAnArchitecture(int bitSize) {
+    this.getStem().regexpMatch(".*_" + getAnArchitecture(bitSize) + "(_test)?")
+  }
+
   override string toString() { result = Container.super.toString() }
 
   /** Gets the URL of this file. */

ql/test/query-tests/Security/CWE-681/IncorrectIntegerConversion.expected

@@ -65,6 +65,10 @@ edges
 | IncorrectIntegerConversion.go:367:2:367:60 | ... := ...[0] : int64 | IncorrectIntegerConversion.go:374:6:374:19 | type conversion |
 | IncorrectIntegerConversion.go:367:2:367:60 | ... := ...[0] : int64 | IncorrectIntegerConversion.go:375:6:375:18 | type conversion |
 | IncorrectIntegerConversion.go:367:2:367:60 | ... := ...[0] : int64 | IncorrectIntegerConversion.go:376:6:376:19 | type conversion |
+| TestNoArchitectureBuildConstraints.go:12:3:12:48 | ... := ...[0] : int64 | TestNoArchitectureBuildConstraints.go:16:7:16:19 | type conversion |
+| TestNoArchitectureBuildConstraints.go:12:3:12:48 | ... := ...[0] : int64 | TestNoArchitectureBuildConstraints.go:17:7:17:20 | type conversion |
+| TestNoArchitectureBuildConstraints.go:20:3:20:49 | ... := ...[0] : int64 | TestNoArchitectureBuildConstraints.go:24:7:24:17 | type conversion |
+| TestNoArchitectureBuildConstraints.go:20:3:20:49 | ... := ...[0] : int64 | TestNoArchitectureBuildConstraints.go:25:7:25:18 | type conversion |
 nodes
 | IncorrectIntegerConversion.go:26:2:26:28 | ... := ...[0] : int | semmle.label | ... := ...[0] : int |
 | IncorrectIntegerConversion.go:35:41:35:50 | type conversion | semmle.label | type conversion |
@@ -168,6 +172,12 @@ nodes
 | IncorrectIntegerConversion.go:374:6:374:19 | type conversion | semmle.label | type conversion |
 | IncorrectIntegerConversion.go:375:6:375:18 | type conversion | semmle.label | type conversion |
 | IncorrectIntegerConversion.go:376:6:376:19 | type conversion | semmle.label | type conversion |
+| TestNoArchitectureBuildConstraints.go:12:3:12:48 | ... := ...[0] : int64 | semmle.label | ... := ...[0] : int64 |
+| TestNoArchitectureBuildConstraints.go:16:7:16:19 | type conversion | semmle.label | type conversion |
+| TestNoArchitectureBuildConstraints.go:17:7:17:20 | type conversion | semmle.label | type conversion |
+| TestNoArchitectureBuildConstraints.go:20:3:20:49 | ... := ...[0] : int64 | semmle.label | ... := ...[0] : int64 |
+| TestNoArchitectureBuildConstraints.go:24:7:24:17 | type conversion | semmle.label | type conversion |
+| TestNoArchitectureBuildConstraints.go:25:7:25:18 | type conversion | semmle.label | type conversion |
 #select
 | IncorrectIntegerConversion.go:26:2:26:28 | ... := ...[0] | IncorrectIntegerConversion.go:26:2:26:28 | ... := ...[0] : int | IncorrectIntegerConversion.go:35:41:35:50 | type conversion | Incorrect conversion of an integer with architecture-dependent bit size from strconv.Atoi to a lower bit size type int32 without an upper bound check. |
 | IncorrectIntegerConversion.go:65:3:65:49 | ... := ...[0] | IncorrectIntegerConversion.go:65:3:65:49 | ... := ...[0] : int64 | IncorrectIntegerConversion.go:69:7:69:18 | type conversion | Incorrect conversion of a 16-bit integer from strconv.ParseInt to a lower bit size type int8 without an upper bound check. |
@@ -231,3 +241,7 @@ nodes
 | IncorrectIntegerConversion.go:367:2:367:60 | ... := ...[0] | IncorrectIntegerConversion.go:367:2:367:60 | ... := ...[0] : int64 | IncorrectIntegerConversion.go:374:6:374:19 | type conversion | Incorrect conversion of an integer with architecture-dependent bit size from strconv.ParseInt to a lower bit size type uint16 without an upper bound check. |
 | IncorrectIntegerConversion.go:367:2:367:60 | ... := ...[0] | IncorrectIntegerConversion.go:367:2:367:60 | ... := ...[0] : int64 | IncorrectIntegerConversion.go:375:6:375:18 | type conversion | Incorrect conversion of an integer with architecture-dependent bit size from strconv.ParseInt to a lower bit size type int32 without an upper bound check. |
 | IncorrectIntegerConversion.go:367:2:367:60 | ... := ...[0] | IncorrectIntegerConversion.go:367:2:367:60 | ... := ...[0] : int64 | IncorrectIntegerConversion.go:376:6:376:19 | type conversion | Incorrect conversion of an integer with architecture-dependent bit size from strconv.ParseInt to a lower bit size type uint32 without an upper bound check. |
+| TestNoArchitectureBuildConstraints.go:12:3:12:48 | ... := ...[0] | TestNoArchitectureBuildConstraints.go:12:3:12:48 | ... := ...[0] : int64 | TestNoArchitectureBuildConstraints.go:16:7:16:19 | type conversion | Incorrect conversion of an integer with architecture-dependent bit size from strconv.ParseInt to a lower bit size type int32 without an upper bound check. |
+| TestNoArchitectureBuildConstraints.go:12:3:12:48 | ... := ...[0] | TestNoArchitectureBuildConstraints.go:12:3:12:48 | ... := ...[0] : int64 | TestNoArchitectureBuildConstraints.go:17:7:17:20 | type conversion | Incorrect conversion of an integer with architecture-dependent bit size from strconv.ParseInt to a lower bit size type uint32 without an upper bound check. |
+| TestNoArchitectureBuildConstraints.go:20:3:20:49 | ... := ...[0] | TestNoArchitectureBuildConstraints.go:20:3:20:49 | ... := ...[0] : int64 | TestNoArchitectureBuildConstraints.go:24:7:24:17 | type conversion | Incorrect conversion of a 64-bit integer from strconv.ParseInt to a lower bit size type int without an upper bound check. |
+| TestNoArchitectureBuildConstraints.go:20:3:20:49 | ... := ...[0] | TestNoArchitectureBuildConstraints.go:20:3:20:49 | ... := ...[0] : int64 | TestNoArchitectureBuildConstraints.go:25:7:25:18 | type conversion | Incorrect conversion of a 64-bit integer from strconv.ParseInt to a lower bit size type uint without an upper bound check. |

ql/test/query-tests/Security/CWE-681/Test32BitArchitectureBuildConstraintInFileName_386.go

@@ -0,0 +1,34 @@
+// Note that the filename acts as an implicit build constraint
+
+package main
+
+import (
+	"strconv"
+)
+
+func testIntSource386() {
+	{
+		parsed, err := strconv.ParseInt("3456", 10, 0)
+		if err != nil {
+			panic(err)
+		}
+		_ = int32(parsed)  // OK
+		_ = uint32(parsed) // OK
+	}
+	{
+		parsed, err := strconv.ParseUint("3456", 10, 0)
+		if err != nil {
+			panic(err)
+		}
+		_ = int32(parsed)  // OK
+		_ = uint32(parsed) // OK
+	}
+	{
+		parsed, err := strconv.Atoi("3456")
+		if err != nil {
+			panic(err)
+		}
+		_ = int32(parsed)  // OK
+		_ = uint32(parsed) // OK
+	}
+}

ql/test/query-tests/Security/CWE-681/Test32BitArchitectureBuildConstraints.go

@@ -0,0 +1,36 @@
+// +build 386 amd64p32 arm armbe mips mipsle mips64p32 mips64p32le ppc s390 sparc
+// +build gc
+// +build go1.4
+
+package main
+
+import (
+	"strconv"
+)
+
+func testIntSource32() {
+	{
+		parsed, err := strconv.ParseInt("3456", 10, 0)
+		if err != nil {
+			panic(err)
+		}
+		_ = int32(parsed)  // OK
+		_ = uint32(parsed) // OK
+	}
+	{
+		parsed, err := strconv.ParseUint("3456", 10, 0)
+		if err != nil {
+			panic(err)
+		}
+		_ = int32(parsed)  // OK
+		_ = uint32(parsed) // OK
+	}
+	{
+		parsed, err := strconv.Atoi("3456")
+		if err != nil {
+			panic(err)
+		}
+		_ = int32(parsed)  // OK
+		_ = uint32(parsed) // OK
+	}
+}

ql/test/query-tests/Security/CWE-681/Test64BitArchitectureBuildConstraintInFileName_amd64.go

@@ -0,0 +1,26 @@
+// Note that the filename acts as an implicit build constraint
+
+package main
+
+import (
+	"strconv"
+)
+
+func testIntSinkAmd64() {
+	{
+		parsed, err := strconv.ParseInt("3456", 10, 64)
+		if err != nil {
+			panic(err)
+		}
+		_ = int(parsed)  // OK
+		_ = uint(parsed) // OK
+	}
+	{
+		parsed, err := strconv.ParseUint("3456", 10, 64)
+		if err != nil {
+			panic(err)
+		}
+		_ = int(parsed)  // OK
+		_ = uint(parsed) // OK
+	}
+}

ql/test/query-tests/Security/CWE-681/Test64BitArchitectureBuildConstraints.go

@@ -0,0 +1,28 @@
+// +build amd64 arm64 arm64be ppc64 ppc64le mips64 mips64le s390x sparc64
+// +build gc
+// +build go1.4
+
+package main
+
+import (
+	"strconv"
+)
+
+func testIntSink64() {
+	{
+		parsed, err := strconv.ParseInt("3456", 10, 64)
+		if err != nil {
+			panic(err)
+		}
+		_ = int(parsed)  // OK
+		_ = uint(parsed) // OK
+	}
+	{
+		parsed, err := strconv.ParseUint("3456", 10, 64)
+		if err != nil {
+			panic(err)
+		}
+		_ = int(parsed)  // OK
+		_ = uint(parsed) // OK
+	}
+}

ql/test/query-tests/Security/CWE-681/TestNoArchitectureBuildConstraints.go

@@ -0,0 +1,27 @@
+// +build gc
+// +build go1.4
+
+package main
+
+import (
+	"strconv"
+)
+
+func testIntSizeIsArchicturallyDependent1() {
+	{
+		parsed, err := strconv.ParseInt("3456", 10, 0)
+		if err != nil {
+			panic(err)
+		}
+		_ = int32(parsed)  // NOT OK
+		_ = uint32(parsed) // NOT OK
+	}
+	{
+		parsed, err := strconv.ParseInt("3456", 10, 64)
+		if err != nil {
+			panic(err)
+		}
+		_ = int(parsed)  // NOT OK
+		_ = uint(parsed) // NOT OK
+	}
+}