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) ) } 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]`. 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..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,6 +264,9 @@ func testBoundsChecking(input string) { _ = int16(parsed) } } + if parsed <= math.MaxUint16 { + _ = uint16(parsed) + } } { parsed, err := strconv.ParseUint(input, 10, 32)