From 4d1608aafa13b44aff11d4694326925a63a5785d Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Fri, 17 Feb 2023 18:53:18 +0000 Subject: [PATCH 1/4] Go integer conversion: check against sink, not source signedness --- .../IncorrectIntegerConversionLib.qll | 24 +++++++++---------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/go/ql/lib/semmle/go/security/IncorrectIntegerConversionLib.qll b/go/ql/lib/semmle/go/security/IncorrectIntegerConversionLib.qll index 060832cfffc..1c0b647e91f 100644 --- a/go/ql/lib/semmle/go/security/IncorrectIntegerConversionLib.qll +++ b/go/ql/lib/semmle/go/security/IncorrectIntegerConversionLib.qll @@ -56,14 +56,14 @@ private predicate isIncorrectIntegerConversion(int sourceBitSize, int sinkBitSiz * integer types, which could cause unexpected values. */ class ConversionWithoutBoundsCheckConfig extends TaintTracking::Configuration { - boolean sourceIsSigned; + boolean sinkIsSigned; int sourceBitSize; int sinkBitSize; ConversionWithoutBoundsCheckConfig() { - sourceIsSigned in [true, false] and + sinkIsSigned in [true, false] and isIncorrectIntegerConversion(sourceBitSize, sinkBitSize) and - this = "ConversionWithoutBoundsCheckConfig" + sourceBitSize + sourceIsSigned + sinkBitSize + this = "ConversionWithoutBoundsCheckConfig" + sourceBitSize + sinkIsSigned + sinkBitSize } /** Gets the bit size of the source. */ @@ -75,11 +75,6 @@ class ConversionWithoutBoundsCheckConfig extends TaintTracking::Configuration { | c.getTarget() = ip and source = c.getResult(0) | - ( - if ip.getResultType(0) instanceof SignedIntegerType - then sourceIsSigned = true - else sourceIsSigned = false - ) and ( apparentBitSize = ip.getTargetBitSize() or @@ -112,10 +107,13 @@ class ConversionWithoutBoundsCheckConfig extends TaintTracking::Configuration { predicate isSinkWithBitSize(DataFlow::TypeCastNode sink, int bitSize) { sink.asExpr() instanceof ConversionExpr and exists(IntegerType integerType | sink.getResultType().getUnderlyingType() = integerType | - bitSize = integerType.getSize() - or - not exists(integerType.getSize()) and - bitSize = getIntTypeBitSize(sink.getFile()) + ( + bitSize = integerType.getSize() + or + not exists(integerType.getSize()) and + bitSize = getIntTypeBitSize(sink.getFile()) + ) and + if integerType instanceof SignedIntegerType then sinkIsSigned = true else sinkIsSigned = false ) and not exists(ShrExpr shrExpr | shrExpr.getLeftOperand().getGlobalValueNumber() = @@ -134,7 +132,7 @@ class ConversionWithoutBoundsCheckConfig extends TaintTracking::Configuration { if sinkBitSize != 0 then bitSize = sinkBitSize else bitSize = 32 | node = DataFlow::BarrierGuard::getABarrierNodeForGuard(g) and - g.isBoundFor(bitSize, sourceIsSigned) + g.isBoundFor(bitSize, sinkIsSigned) ) } From 4e86edf4fe7dea14faddc7b8e35095ee87be3d4a Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Fri, 17 Feb 2023 19:16:36 +0000 Subject: [PATCH 2/4] Add test case --- .../query-tests/Security/CWE-681/IncorrectIntegerConversion.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/go/ql/test/query-tests/Security/CWE-681/IncorrectIntegerConversion.go b/go/ql/test/query-tests/Security/CWE-681/IncorrectIntegerConversion.go index 92782b7ad67..e5298cc5644 100644 --- a/go/ql/test/query-tests/Security/CWE-681/IncorrectIntegerConversion.go +++ b/go/ql/test/query-tests/Security/CWE-681/IncorrectIntegerConversion.go @@ -264,6 +264,9 @@ func testBoundsChecking(input string) { _ = int16(parsed) } } + if parsed <= math.MaxUint32 { + _ = uint32(parsed) + } } { parsed, err := strconv.ParseUint(input, 10, 32) From be468fe122424b146b855ae911d8ee869b0c1e21 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Fri, 17 Feb 2023 19:21:15 +0000 Subject: [PATCH 3/4] Change note --- go/ql/src/change-notes/2023-02-17-integer-conversion-fix.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 go/ql/src/change-notes/2023-02-17-integer-conversion-fix.md diff --git a/go/ql/src/change-notes/2023-02-17-integer-conversion-fix.md b/go/ql/src/change-notes/2023-02-17-integer-conversion-fix.md new file mode 100644 index 00000000000..9fb36271a32 --- /dev/null +++ b/go/ql/src/change-notes/2023-02-17-integer-conversion-fix.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* The query `go/incorrect-integer-conversion` now correctly recognises guards of the form `if val <= x` to protect a conversion `uintX(val)` when `x` is in the range `(math.MaxIntX, math.MaxUintX]`. From c7da1c9e0d571a1844a2cb9cc64afcf8cf591a07 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Fri, 17 Feb 2023 19:35:04 +0000 Subject: [PATCH 4/4] Use example that compiles on 32-bit arch --- .../Security/CWE-681/IncorrectIntegerConversion.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/go/ql/test/query-tests/Security/CWE-681/IncorrectIntegerConversion.go b/go/ql/test/query-tests/Security/CWE-681/IncorrectIntegerConversion.go index e5298cc5644..d10d1e47a3b 100644 --- a/go/ql/test/query-tests/Security/CWE-681/IncorrectIntegerConversion.go +++ b/go/ql/test/query-tests/Security/CWE-681/IncorrectIntegerConversion.go @@ -264,8 +264,8 @@ func testBoundsChecking(input string) { _ = int16(parsed) } } - if parsed <= math.MaxUint32 { - _ = uint32(parsed) + if parsed <= math.MaxUint16 { + _ = uint16(parsed) } } {