From 74481c4bad66c7fd72584c2243eab9c82ca057de Mon Sep 17 00:00:00 2001 From: Slavomir Date: Mon, 6 Apr 2020 18:03:26 +0300 Subject: [PATCH 01/22] CWE-681: initial commit --- .../CWE-681/IncorrectNumericConversion.go | 13 ++ .../CWE-681/IncorrectNumericConversion.qhelp | 48 +++++ .../CWE-681/IncorrectNumericConversion.ql | 141 ++++++++++++ .../CWE-681/IncorrectNumericConversionGood.go | 32 +++ .../IncorrectNumericConversion.expected | 43 ++++ .../CWE-681/IncorrectNumericConversion.go | 201 ++++++++++++++++++ .../CWE-681/IncorrectNumericConversion.qlref | 1 + 7 files changed, 479 insertions(+) create mode 100644 ql/src/Security/CWE-681/IncorrectNumericConversion.go create mode 100644 ql/src/Security/CWE-681/IncorrectNumericConversion.qhelp create mode 100644 ql/src/Security/CWE-681/IncorrectNumericConversion.ql create mode 100644 ql/src/Security/CWE-681/IncorrectNumericConversionGood.go create mode 100644 ql/test/query-tests/Security/CWE-681/IncorrectNumericConversion.expected create mode 100644 ql/test/query-tests/Security/CWE-681/IncorrectNumericConversion.go create mode 100644 ql/test/query-tests/Security/CWE-681/IncorrectNumericConversion.qlref diff --git a/ql/src/Security/CWE-681/IncorrectNumericConversion.go b/ql/src/Security/CWE-681/IncorrectNumericConversion.go new file mode 100644 index 00000000000..11c343550ab --- /dev/null +++ b/ql/src/Security/CWE-681/IncorrectNumericConversion.go @@ -0,0 +1,13 @@ +package main + +import ( + "strconv" +) + +func parseAllocate(wanted string) int32 { + parsed, err := strconv.Atoi(wanted) + if err != nil { + panic(err) + } + return int32(parsed) +} diff --git a/ql/src/Security/CWE-681/IncorrectNumericConversion.qhelp b/ql/src/Security/CWE-681/IncorrectNumericConversion.qhelp new file mode 100644 index 00000000000..77dd3f1cfac --- /dev/null +++ b/ql/src/Security/CWE-681/IncorrectNumericConversion.qhelp @@ -0,0 +1,48 @@ + + + + +

+If a numeric value string is parsed using strconv.Atoi into an int, and then that int +is converted into another type of a lower bit size, the result can produce unexpected values. +

+ + + +

+If you need to parse numeric values with specific bit sizes, use the functions specific to each +type (strconv.ParseFloat, strconv.ParseInt, strconv.ParseUint) +that also allow to specify the wanted bit size. +

+

+If this is not possible, then add upper (and lower) bound checks specific to each type and +bit size (you can find the min and max value for each type in the `math` package). +

+ + + +

+In the following example, assume that an input string is passed to parseAllocate function, +parsed by strconv.Atoi, and then converted into an int32 type: +

+ +

+The bounds are not checked, so this means that if the provided number is greater than max int32, +the resulting value from the conversion will be different from the provided value. +

+

+To avoid unexpected values, you should either use the other functions provided by the strconv +package to parse the specific types and bit sizes; in this case, strconv.ParseInt as you +can see in parseAllocateGood2 function; or check bounds as in parseAllocateGood1 +function. +

+ + + +
  • +mitre.org: CWE-681: Incorrect Conversion between Numeric Types. +
    • +     +     diff --git a/ql/src/Security/CWE-681/IncorrectNumericConversion.ql b/ql/src/Security/CWE-681/IncorrectNumericConversion.ql new file mode 100644 index 00000000000..90b74791d09 --- /dev/null +++ b/ql/src/Security/CWE-681/IncorrectNumericConversion.ql @@ -0,0 +1,141 @@ +/** + * @name Incorrect Conversion between Numeric Types + * @description Converting the result of strconv.Atoi (which is of type `int`) to + * numeric types of lower bit size can produce unexpected values. + * @kind path-problem + * @id go/incorrect-numeric-conversion + * @tags security + * external/cwe/cwe-681 + */ + +import go +import DataFlow::PathGraph + +class IntParser extends Function { + IntParser() { this.hasQualifiedName("strconv", "Atoi") } +} + +class OverflowingConversionExpr extends ConversionExpr { + string conversionTypeName; + + OverflowingConversionExpr() { + exists(ConversionExpr conv | + conversionTypeName = conv.getTypeExpr().getType().getUnderlyingType*().getName() and + ( + // anything lower than int64: + conversionTypeName = "int8" or + conversionTypeName = "int16" or + conversionTypeName = "int32" or + // anything lower than uint64: + conversionTypeName = "uint8" or + conversionTypeName = "uint16" or + conversionTypeName = "uint32" or + // anything lower than float64: + conversionTypeName = "float32" + ) + | + this = conv + ) + } + + string getTypeName() { result = conversionTypeName } +} + +class IfRelationalComparison extends IfStmt { + IfRelationalComparison() { + this.getCond() instanceof RelationalComparisonExpr or this.getCond() instanceof LandExpr + } + + RelationalComparisonExpr getComparison() { result = this.getCond().(RelationalComparisonExpr) } + + LandExpr getLandExpr() { result = this.getCond().(LandExpr) } +} + +class FlowConfig extends TaintTracking::Configuration, DataFlow::Configuration { + FlowConfig() { this = "FlowConfig" } + + override predicate isSource(DataFlow::Node source) { + exists(IntParser atoi | source = atoi.getACall().getResult(0)) + } + + override predicate isSink(DataFlow::Node sink) { + exists(OverflowingConversionExpr conv | sink.asExpr() = conv) + } + + override predicate isSanitizerIn(DataFlow::Node node) { + // If the conversion is inside an `if` block that compares the + // source as `source > 0`, then that sanitizes conversion of int64 to int32; + exists(IfRelationalComparison san, OverflowingConversionExpr conv | + conv = node.asExpr().(OverflowingConversionExpr) and + san.getThen().getAChild*() = conv and + ( + conv.getTypeName() = "int32" and + san.getComparison().getLesserOperand().getNumericValue() = 0 and + san.getComparison().getGreaterOperand().getGlobalValueNumber() = + conv.getOperand().getGlobalValueNumber() + or + comparisonGreaterOperandIsEqualOrLess("int8", san, conv, getMaxInt8()) + or + comparisonGreaterOperandIsEqualOrLess("int16", san, conv, getMaxInt16()) + or + comparisonGreaterOperandIsEqualOrLess("int32", san, conv, getMaxInt32()) + or + comparisonGreaterOperandIsEqualOrLess("uint8", san, conv, getMaxUint8()) + or + comparisonGreaterOperandIsEqualOrLess("uint16", san, conv, getMaxUint16()) + ) + ) + } +} + +int getMaxInt8() { + result = 2.pow(7) - 1 + // = 1<<7 - 1 +} + +int getMaxInt16() { + result = 2.pow(15) - 1 + // = 1<<15 - 1 +} + +int getMaxInt32() { + result = 2.pow(31) - 1 + // = 1<<31 - 1 +} + +int getMaxUint8() { + result = 2.pow(8) - 1 + // = 1<<8 - 1 +} + +int getMaxUint16() { + result = 2.pow(16) - 1 + // = 1<<16 - 1 +} + +predicate comparisonGreaterOperandIsEqualOrLess( + string typeName, IfRelationalComparison ifExpr, OverflowingConversionExpr conv, int value +) { + conv.getTypeName() = typeName and + ( + // exclude cases like: if parsed < math.MaxInt8 { int8(parsed)} + ifExpr.getComparison().getGreaterOperand().getNumericValue() = value and + // and lesser is the conversion operand: + ifExpr.getComparison().getLesserOperand().getGlobalValueNumber() = + conv.getOperand().getGlobalValueNumber() + or + // exclude cases like: if err == nil && parsed < math.MaxInt8 { int8(parsed)} + exists(RelationalComparisonExpr andExpr | + andExpr = ifExpr.getLandExpr().getAnOperand().(RelationalComparisonExpr) + | + andExpr.getGreaterOperand().getNumericValue() = value and + // and lesser is the conversion operand: + andExpr.getLesserOperand().getGlobalValueNumber() = conv.getOperand().getGlobalValueNumber() + ) + ) +} + +from FlowConfig cfg, DataFlow::PathNode source, DataFlow::PathNode sink +where cfg.hasFlowPath(source, sink) +select source, source, sink, + "Incorrect type conversion of int from strconv.Atoi result to another numeric type" diff --git a/ql/src/Security/CWE-681/IncorrectNumericConversionGood.go b/ql/src/Security/CWE-681/IncorrectNumericConversionGood.go new file mode 100644 index 00000000000..003f82cd2a9 --- /dev/null +++ b/ql/src/Security/CWE-681/IncorrectNumericConversionGood.go @@ -0,0 +1,32 @@ +package main + +import ( + "math" + "strconv" +) + +func main() { + +} + +const DefaultAllocate int32 = 256 + +func parseAllocateGood1(desired string) int32 { + parsed, err := strconv.Atoi(desired) + if err != nil { + return DefaultAllocate + } + // GOOD: check for lower and uppper bounds + if parsed > 0 && parsed <= math.MaxInt32 { + return int32(parsed) + } + return DefaultAllocate +} +func parseAllocateGood2(desired string) int32 { + // GOOD: parse specifying the bit size + parsed, err := strconv.ParseInt(desired, 10, 32) + if err != nil { + return DefaultAllocate + } + return int32(parsed) +} diff --git a/ql/test/query-tests/Security/CWE-681/IncorrectNumericConversion.expected b/ql/test/query-tests/Security/CWE-681/IncorrectNumericConversion.expected new file mode 100644 index 00000000000..03a09e927fa --- /dev/null +++ b/ql/test/query-tests/Security/CWE-681/IncorrectNumericConversion.expected @@ -0,0 +1,43 @@ +edges +| IncorrectNumericConversion.go:26:2:26:28 | ... := ...[0] : int | IncorrectNumericConversion.go:35:41:35:50 | type conversion | +| IncorrectNumericConversion.go:54:3:54:36 | ... := ...[0] : int | IncorrectNumericConversion.go:58:7:58:18 | type conversion | +| IncorrectNumericConversion.go:61:3:61:36 | ... := ...[0] : int | IncorrectNumericConversion.go:65:7:65:19 | type conversion | +| IncorrectNumericConversion.go:68:3:68:36 | ... := ...[0] : int | IncorrectNumericConversion.go:72:7:72:19 | type conversion | +| IncorrectNumericConversion.go:75:3:75:36 | ... := ...[0] : int | IncorrectNumericConversion.go:79:7:79:19 | type conversion | +| IncorrectNumericConversion.go:82:3:82:36 | ... := ...[0] : int | IncorrectNumericConversion.go:86:7:86:20 | type conversion | +| IncorrectNumericConversion.go:89:3:89:36 | ... := ...[0] : int | IncorrectNumericConversion.go:93:7:93:20 | type conversion | +| IncorrectNumericConversion.go:96:3:96:36 | ... := ...[0] : int | IncorrectNumericConversion.go:100:7:100:21 | type conversion | +| IncorrectNumericConversion.go:103:3:103:36 | ... := ...[0] : int | IncorrectNumericConversion.go:108:7:108:18 | type conversion | +| IncorrectNumericConversion.go:112:3:112:36 | ... := ...[0] : int | IncorrectNumericConversion.go:116:7:116:23 | type conversion | +nodes +| IncorrectNumericConversion.go:26:2:26:28 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | +| IncorrectNumericConversion.go:35:41:35:50 | type conversion | semmle.label | type conversion | +| IncorrectNumericConversion.go:54:3:54:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | +| IncorrectNumericConversion.go:58:7:58:18 | type conversion | semmle.label | type conversion | +| IncorrectNumericConversion.go:61:3:61:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | +| IncorrectNumericConversion.go:65:7:65:19 | type conversion | semmle.label | type conversion | +| IncorrectNumericConversion.go:68:3:68:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | +| IncorrectNumericConversion.go:72:7:72:19 | type conversion | semmle.label | type conversion | +| IncorrectNumericConversion.go:75:3:75:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | +| IncorrectNumericConversion.go:79:7:79:19 | type conversion | semmle.label | type conversion | +| IncorrectNumericConversion.go:82:3:82:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | +| IncorrectNumericConversion.go:86:7:86:20 | type conversion | semmle.label | type conversion | +| IncorrectNumericConversion.go:89:3:89:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | +| IncorrectNumericConversion.go:93:7:93:20 | type conversion | semmle.label | type conversion | +| IncorrectNumericConversion.go:96:3:96:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | +| IncorrectNumericConversion.go:100:7:100:21 | type conversion | semmle.label | type conversion | +| IncorrectNumericConversion.go:103:3:103:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | +| IncorrectNumericConversion.go:108:7:108:18 | type conversion | semmle.label | type conversion | +| IncorrectNumericConversion.go:112:3:112:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | +| IncorrectNumericConversion.go:116:7:116:23 | type conversion | semmle.label | type conversion | +#select +| IncorrectNumericConversion.go:26:2:26:28 | ... := ...[0] : int | IncorrectNumericConversion.go:26:2:26:28 | ... := ...[0] : int | IncorrectNumericConversion.go:35:41:35:50 | type conversion | Incorrect type conversion of int from strconv.Atoi result to another numeric type | +| IncorrectNumericConversion.go:54:3:54:36 | ... := ...[0] : int | IncorrectNumericConversion.go:54:3:54:36 | ... := ...[0] : int | IncorrectNumericConversion.go:58:7:58:18 | type conversion | Incorrect type conversion of int from strconv.Atoi result to another numeric type | +| IncorrectNumericConversion.go:61:3:61:36 | ... := ...[0] : int | IncorrectNumericConversion.go:61:3:61:36 | ... := ...[0] : int | IncorrectNumericConversion.go:65:7:65:19 | type conversion | Incorrect type conversion of int from strconv.Atoi result to another numeric type | +| IncorrectNumericConversion.go:68:3:68:36 | ... := ...[0] : int | IncorrectNumericConversion.go:68:3:68:36 | ... := ...[0] : int | IncorrectNumericConversion.go:72:7:72:19 | type conversion | Incorrect type conversion of int from strconv.Atoi result to another numeric type | +| IncorrectNumericConversion.go:75:3:75:36 | ... := ...[0] : int | IncorrectNumericConversion.go:75:3:75:36 | ... := ...[0] : int | IncorrectNumericConversion.go:79:7:79:19 | type conversion | Incorrect type conversion of int from strconv.Atoi result to another numeric type | +| IncorrectNumericConversion.go:82:3:82:36 | ... := ...[0] : int | IncorrectNumericConversion.go:82:3:82:36 | ... := ...[0] : int | IncorrectNumericConversion.go:86:7:86:20 | type conversion | Incorrect type conversion of int from strconv.Atoi result to another numeric type | +| IncorrectNumericConversion.go:89:3:89:36 | ... := ...[0] : int | IncorrectNumericConversion.go:89:3:89:36 | ... := ...[0] : int | IncorrectNumericConversion.go:93:7:93:20 | type conversion | Incorrect type conversion of int from strconv.Atoi result to another numeric type | +| IncorrectNumericConversion.go:96:3:96:36 | ... := ...[0] : int | IncorrectNumericConversion.go:96:3:96:36 | ... := ...[0] : int | IncorrectNumericConversion.go:100:7:100:21 | type conversion | Incorrect type conversion of int from strconv.Atoi result to another numeric type | +| IncorrectNumericConversion.go:103:3:103:36 | ... := ...[0] : int | IncorrectNumericConversion.go:103:3:103:36 | ... := ...[0] : int | IncorrectNumericConversion.go:108:7:108:18 | type conversion | Incorrect type conversion of int from strconv.Atoi result to another numeric type | +| IncorrectNumericConversion.go:112:3:112:36 | ... := ...[0] : int | IncorrectNumericConversion.go:112:3:112:36 | ... := ...[0] : int | IncorrectNumericConversion.go:116:7:116:23 | type conversion | Incorrect type conversion of int from strconv.Atoi result to another numeric type | diff --git a/ql/test/query-tests/Security/CWE-681/IncorrectNumericConversion.go b/ql/test/query-tests/Security/CWE-681/IncorrectNumericConversion.go new file mode 100644 index 00000000000..a437bd70924 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-681/IncorrectNumericConversion.go @@ -0,0 +1,201 @@ +package main + +import ( + "math" + "strconv" +) + +func main() { + +} + +type Something struct { +} +type Config struct { +} +type Registry struct { +} + +func LookupTarget(conf *Config, num int32) (int32, error) { + return 567, nil +} +func LookupNumberByName(reg *Registry, name string) (int32, error) { + return 567, nil +} +func lab(s string) (*Something, error) { + num, err := strconv.Atoi(s) + + if err != nil { + number, err := LookupNumberByName(&Registry{}, s) + if err != nil { + return nil, err + } + num = int(number) + } + target, err := LookupTarget(&Config{}, int32(num)) + if err != nil { + return nil, err + } + + // convert the resolved target number back to a string + + s = strconv.Itoa(int(target)) + + return nil, nil +} + +const CustomMaxInt16 = 1<<15 - 1 + +type CustomInt int16 + +// these should be caught: +func upperBoundIsNOTChecked(input string) { + { + parsed, err := strconv.Atoi(input) + if err != nil { + panic(err) + } + _ = int8(parsed) + } + { + parsed, err := strconv.Atoi(input) + if err != nil { + panic(err) + } + _ = int16(parsed) + } + { + parsed, err := strconv.Atoi(input) + if err != nil { + panic(err) + } + _ = int32(parsed) + } + { + parsed, err := strconv.Atoi(input) + if err != nil { + panic(err) + } + _ = uint8(parsed) + } + { + parsed, err := strconv.Atoi(input) + if err != nil { + panic(err) + } + _ = uint16(parsed) + } + { + parsed, err := strconv.Atoi(input) + if err != nil { + panic(err) + } + _ = uint32(parsed) + } + { + parsed, err := strconv.Atoi(input) + if err != nil { + panic(err) + } + _ = float32(parsed) + } + { + parsed, err := strconv.Atoi(input) + if err != nil { + panic(err) + } + // NOTE: byte is uint8 + _ = byte(parsed) + } + { + // using custom type: + parsed, err := strconv.Atoi(input) + if err != nil { + panic(err) + } + _ = CustomInt(parsed) + } + +} + +// these should NOT be caught: +func upperBoundIsChecked(input string) { + { + parsed, err := strconv.Atoi(input) + if err != nil { + panic(err) + } + if parsed < math.MaxInt8 { + _ = int8(parsed) + } + } + { + parsed, err := strconv.Atoi(input) + if err != nil { + panic(err) + } + if parsed < math.MaxInt16 { + _ = int16(parsed) + } + } + { + parsed, err := strconv.Atoi(input) + if err != nil { + panic(err) + } + if parsed > 0 { + _ = int32(parsed) + } + } + { + parsed, err := strconv.Atoi(input) + if err != nil { + panic(err) + } + if parsed < math.MaxInt32 { + _ = int32(parsed) + } + } + { + parsed, err := strconv.Atoi(input) + if err != nil { + panic(err) + } + if parsed < math.MaxUint8 { + _ = uint8(parsed) + } + } + { + parsed, err := strconv.Atoi(input) + if err != nil { + panic(err) + } + if parsed < math.MaxUint16 { + _ = uint16(parsed) + } + } + { + parsed, err := strconv.Atoi(input) + if err != nil { + panic(err) + } + if parsed < math.MaxUint8 { + _ = byte(parsed) + } + } + { // multiple `and` conditions + parsed, err := strconv.Atoi(input) + if err == nil && 1 == 1 && parsed < math.MaxInt8 { + _ = int8(parsed) + } + } + { // custom maxInt16 + parsed, err := strconv.Atoi(input) + if err != nil { + panic(err) + } + if parsed < CustomMaxInt16 { + _ = int16(parsed) + } + } +} diff --git a/ql/test/query-tests/Security/CWE-681/IncorrectNumericConversion.qlref b/ql/test/query-tests/Security/CWE-681/IncorrectNumericConversion.qlref new file mode 100644 index 00000000000..7e678611eb2 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-681/IncorrectNumericConversion.qlref @@ -0,0 +1 @@ +Security/CWE-681/IncorrectNumericConversion.ql \ No newline at end of file From 6d2c5be196b2409950cf05be3dd84b50b21a1741 Mon Sep 17 00:00:00 2001 From: Slavomir Date: Mon, 6 Apr 2020 18:08:52 +0300 Subject: [PATCH 02/22] rename OverflowingConversionExpr to NumericConversionExpr --- .../Security/CWE-681/IncorrectNumericConversion.ql | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/ql/src/Security/CWE-681/IncorrectNumericConversion.ql b/ql/src/Security/CWE-681/IncorrectNumericConversion.ql index 90b74791d09..4e7f714f201 100644 --- a/ql/src/Security/CWE-681/IncorrectNumericConversion.ql +++ b/ql/src/Security/CWE-681/IncorrectNumericConversion.ql @@ -15,10 +15,10 @@ class IntParser extends Function { IntParser() { this.hasQualifiedName("strconv", "Atoi") } } -class OverflowingConversionExpr extends ConversionExpr { +class NumericConversionExpr extends ConversionExpr { string conversionTypeName; - OverflowingConversionExpr() { + NumericConversionExpr() { exists(ConversionExpr conv | conversionTypeName = conv.getTypeExpr().getType().getUnderlyingType*().getName() and ( @@ -59,14 +59,14 @@ class FlowConfig extends TaintTracking::Configuration, DataFlow::Configuration { } override predicate isSink(DataFlow::Node sink) { - exists(OverflowingConversionExpr conv | sink.asExpr() = conv) + exists(NumericConversionExpr conv | sink.asExpr() = conv) } override predicate isSanitizerIn(DataFlow::Node node) { // If the conversion is inside an `if` block that compares the // source as `source > 0`, then that sanitizes conversion of int64 to int32; - exists(IfRelationalComparison san, OverflowingConversionExpr conv | - conv = node.asExpr().(OverflowingConversionExpr) and + exists(IfRelationalComparison san, NumericConversionExpr conv | + conv = node.asExpr().(NumericConversionExpr) and san.getThen().getAChild*() = conv and ( conv.getTypeName() = "int32" and @@ -114,7 +114,7 @@ int getMaxUint16() { } predicate comparisonGreaterOperandIsEqualOrLess( - string typeName, IfRelationalComparison ifExpr, OverflowingConversionExpr conv, int value + string typeName, IfRelationalComparison ifExpr, NumericConversionExpr conv, int value ) { conv.getTypeName() = typeName and ( From fd9e3a005e82911c629898a09aa1cc8e15bc75de Mon Sep 17 00:00:00 2001 From: Slavomir Date: Mon, 6 Apr 2020 21:40:29 +0300 Subject: [PATCH 03/22] fix comments --- ql/src/Security/CWE-681/IncorrectNumericConversion.ql | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ql/src/Security/CWE-681/IncorrectNumericConversion.ql b/ql/src/Security/CWE-681/IncorrectNumericConversion.ql index 4e7f714f201..ffb2e0431b4 100644 --- a/ql/src/Security/CWE-681/IncorrectNumericConversion.ql +++ b/ql/src/Security/CWE-681/IncorrectNumericConversion.ql @@ -118,13 +118,13 @@ predicate comparisonGreaterOperandIsEqualOrLess( ) { conv.getTypeName() = typeName and ( - // exclude cases like: if parsed < math.MaxInt8 { int8(parsed)} + // exclude cases like: if parsed < math.MaxInt8 {return int8(parsed)} ifExpr.getComparison().getGreaterOperand().getNumericValue() = value and // and lesser is the conversion operand: ifExpr.getComparison().getLesserOperand().getGlobalValueNumber() = conv.getOperand().getGlobalValueNumber() or - // exclude cases like: if err == nil && parsed < math.MaxInt8 { int8(parsed)} + // exclude cases like: if err == nil && parsed < math.MaxInt8 {return int8(parsed)} exists(RelationalComparisonExpr andExpr | andExpr = ifExpr.getLandExpr().getAnOperand().(RelationalComparisonExpr) | From 8ecc2b95237e16b1d739bebc147538f69415bec3 Mon Sep 17 00:00:00 2001 From: Slavomir Date: Mon, 6 Apr 2020 21:48:34 +0300 Subject: [PATCH 04/22] add comments, improve naming, refactor --- .../CWE-681/IncorrectNumericConversion.ql | 27 ++++++++++--------- 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/ql/src/Security/CWE-681/IncorrectNumericConversion.ql b/ql/src/Security/CWE-681/IncorrectNumericConversion.ql index ffb2e0431b4..ba7bb5b76c9 100644 --- a/ql/src/Security/CWE-681/IncorrectNumericConversion.ql +++ b/ql/src/Security/CWE-681/IncorrectNumericConversion.ql @@ -63,26 +63,26 @@ class FlowConfig extends TaintTracking::Configuration, DataFlow::Configuration { } override predicate isSanitizerIn(DataFlow::Node node) { - // If the conversion is inside an `if` block that compares the - // source as `source > 0`, then that sanitizes conversion of int64 to int32; exists(IfRelationalComparison san, NumericConversionExpr conv | conv = node.asExpr().(NumericConversionExpr) and san.getThen().getAChild*() = conv and ( + // If the conversion is inside an `if` block that compares the source as + // `source > 0` or `source >= 0`, then that sanitizes conversion of int to int32; conv.getTypeName() = "int32" and san.getComparison().getLesserOperand().getNumericValue() = 0 and san.getComparison().getGreaterOperand().getGlobalValueNumber() = conv.getOperand().getGlobalValueNumber() or - comparisonGreaterOperandIsEqualOrLess("int8", san, conv, getMaxInt8()) + comparisonGreaterOperandValueIsEqualOrLess("int8", san, conv, getMaxInt8()) or - comparisonGreaterOperandIsEqualOrLess("int16", san, conv, getMaxInt16()) + comparisonGreaterOperandValueIsEqualOrLess("int16", san, conv, getMaxInt16()) or - comparisonGreaterOperandIsEqualOrLess("int32", san, conv, getMaxInt32()) + comparisonGreaterOperandValueIsEqualOrLess("int32", san, conv, getMaxInt32()) or - comparisonGreaterOperandIsEqualOrLess("uint8", san, conv, getMaxUint8()) + comparisonGreaterOperandValueIsEqualOrLess("uint8", san, conv, getMaxUint8()) or - comparisonGreaterOperandIsEqualOrLess("uint16", san, conv, getMaxUint16()) + comparisonGreaterOperandValueIsEqualOrLess("uint16", san, conv, getMaxUint16()) ) ) } @@ -113,21 +113,24 @@ int getMaxUint16() { // = 1<<16 - 1 } -predicate comparisonGreaterOperandIsEqualOrLess( +predicate comparisonGreaterOperandValueIsEqualOrLess( string typeName, IfRelationalComparison ifExpr, NumericConversionExpr conv, int value ) { conv.getTypeName() = typeName and ( // exclude cases like: if parsed < math.MaxInt8 {return int8(parsed)} - ifExpr.getComparison().getGreaterOperand().getNumericValue() = value and - // and lesser is the conversion operand: - ifExpr.getComparison().getLesserOperand().getGlobalValueNumber() = - conv.getOperand().getGlobalValueNumber() + exists(RelationalComparisonExpr comp | comp = ifExpr.getComparison() | + // greater operand is equal to value: + comp.getGreaterOperand().getNumericValue() = value and + // and lesser is the conversion operand: + comp.getLesserOperand().getGlobalValueNumber() = conv.getOperand().getGlobalValueNumber() + ) or // exclude cases like: if err == nil && parsed < math.MaxInt8 {return int8(parsed)} exists(RelationalComparisonExpr andExpr | andExpr = ifExpr.getLandExpr().getAnOperand().(RelationalComparisonExpr) | + // greater operand is equal to value: andExpr.getGreaterOperand().getNumericValue() = value and // and lesser is the conversion operand: andExpr.getLesserOperand().getGlobalValueNumber() = conv.getOperand().getGlobalValueNumber() From b176c4ad19b1843d57d173b2d6b7df837112ef9b Mon Sep 17 00:00:00 2001 From: Slavomir Date: Tue, 7 Apr 2020 13:45:13 +0300 Subject: [PATCH 05/22] Add ParseFloat, ParseInt, ParseUint --- .../CWE-681/IncorrectNumericConversion.ql | 182 ++++++++++++++---- 1 file changed, 149 insertions(+), 33 deletions(-) diff --git a/ql/src/Security/CWE-681/IncorrectNumericConversion.ql b/ql/src/Security/CWE-681/IncorrectNumericConversion.ql index ba7bb5b76c9..d19bf2332bb 100644 --- a/ql/src/Security/CWE-681/IncorrectNumericConversion.ql +++ b/ql/src/Security/CWE-681/IncorrectNumericConversion.ql @@ -11,14 +11,26 @@ import go import DataFlow::PathGraph -class IntParser extends Function { - IntParser() { this.hasQualifiedName("strconv", "Atoi") } +class Atoi extends Function { + Atoi() { this.hasQualifiedName("strconv", "Atoi") } } -class NumericConversionExpr extends ConversionExpr { +class ParseFloat extends Function { + ParseFloat() { this.hasQualifiedName("strconv", "ParseFloat") } +} + +class ParseInt extends Function { + ParseInt() { this.hasQualifiedName("strconv", "ParseInt") } +} + +class ParseUint extends Function { + ParseUint() { this.hasQualifiedName("strconv", "ParseUint") } +} + +class Lte32BitNumericConversionExpr extends ConversionExpr { string conversionTypeName; - NumericConversionExpr() { + Lte32BitNumericConversionExpr() { exists(ConversionExpr conv | conversionTypeName = conv.getTypeExpr().getType().getUnderlyingType*().getName() and ( @@ -41,6 +53,33 @@ class NumericConversionExpr extends ConversionExpr { string getTypeName() { result = conversionTypeName } } +class Lte16BitNumericConversionExpr extends Lte32BitNumericConversionExpr { + Lte16BitNumericConversionExpr() { + conversionTypeName = this.getTypeName() and + ( + // anything lower than int32: + conversionTypeName = "int8" or + conversionTypeName = "int16" or + // anything lower than uint32: + conversionTypeName = "uint8" or + conversionTypeName = "uint16" + ) + } +} + +class Lte8BitNumericConversionExpr extends Lte16BitNumericConversionExpr { + Lte8BitNumericConversionExpr() { + conversionTypeName = this.getTypeName() and + ( + // anything lower than int16: + conversionTypeName = "int8" + or + // anything lower than uint16: + conversionTypeName = "uint8" + ) + } +} + class IfRelationalComparison extends IfStmt { IfRelationalComparison() { this.getCond() instanceof RelationalComparisonExpr or this.getCond() instanceof LandExpr @@ -51,41 +90,115 @@ class IfRelationalComparison extends IfStmt { LandExpr getLandExpr() { result = this.getCond().(LandExpr) } } -class FlowConfig extends TaintTracking::Configuration, DataFlow::Configuration { - FlowConfig() { this = "FlowConfig" } +class Lte64FlowConfig extends TaintTracking::Configuration, DataFlow::Configuration { + Lte64FlowConfig() { this = "Lte64FlowConfig" } override predicate isSource(DataFlow::Node source) { - exists(IntParser atoi | source = atoi.getACall().getResult(0)) + exists(Atoi atoi | source = atoi.getACall().getResult(0)) + or + exists(ParseFloat parseFloat, DataFlow::CallNode call | + call = parseFloat.getACall() and call.getArgument(1).getIntValue() = [64] + | + source = call.getResult(0) + ) + or + exists(ParseInt parseInt, DataFlow::CallNode call | + call = parseInt.getACall() and call.getArgument(2).getIntValue() = [0, 64] + | + source = call.getResult(0) + ) + or + exists(ParseUint parseUint, DataFlow::CallNode call | + call = parseUint.getACall() and call.getArgument(2).getIntValue() = [0, 64] + | + source = call.getResult(0) + ) } override predicate isSink(DataFlow::Node sink) { - exists(NumericConversionExpr conv | sink.asExpr() = conv) + exists(Lte32BitNumericConversionExpr conv | sink.asExpr() = conv) } - override predicate isSanitizerIn(DataFlow::Node node) { - exists(IfRelationalComparison san, NumericConversionExpr conv | - conv = node.asExpr().(NumericConversionExpr) and - san.getThen().getAChild*() = conv and - ( - // If the conversion is inside an `if` block that compares the source as - // `source > 0` or `source >= 0`, then that sanitizes conversion of int to int32; - conv.getTypeName() = "int32" and - san.getComparison().getLesserOperand().getNumericValue() = 0 and - san.getComparison().getGreaterOperand().getGlobalValueNumber() = - conv.getOperand().getGlobalValueNumber() - or - comparisonGreaterOperandValueIsEqualOrLess("int8", san, conv, getMaxInt8()) - or - comparisonGreaterOperandValueIsEqualOrLess("int16", san, conv, getMaxInt16()) - or - comparisonGreaterOperandValueIsEqualOrLess("int32", san, conv, getMaxInt32()) - or - comparisonGreaterOperandValueIsEqualOrLess("uint8", san, conv, getMaxUint8()) - or - comparisonGreaterOperandValueIsEqualOrLess("uint16", san, conv, getMaxUint16()) - ) + override predicate isSanitizerIn(DataFlow::Node node) { isSanitizedInsideAnIfBoundCheck(node) } +} + +class Lte32FlowConfig extends TaintTracking::Configuration, DataFlow::Configuration { + Lte32FlowConfig() { this = "Lte32FlowConfig" } + + override predicate isSource(DataFlow::Node source) { + exists(ParseFloat parseFloat, DataFlow::CallNode call | + call = parseFloat.getACall() and call.getArgument(1).getIntValue() = [32] + | + source = call.getResult(0) + ) + or + exists(ParseInt parseInt, DataFlow::CallNode call | + call = parseInt.getACall() and call.getArgument(2).getIntValue() = [32] + | + source = call.getResult(0) + ) + or + exists(ParseUint parseUint, DataFlow::CallNode call | + call = parseUint.getACall() and call.getArgument(2).getIntValue() = [32] + | + source = call.getResult(0) ) } + + override predicate isSink(DataFlow::Node sink) { + exists(Lte16BitNumericConversionExpr conv | sink.asExpr() = conv) + } + + override predicate isSanitizerIn(DataFlow::Node node) { isSanitizedInsideAnIfBoundCheck(node) } +} + +class Lte16FlowConfig extends TaintTracking::Configuration, DataFlow::Configuration { + Lte16FlowConfig() { this = "Lte16FlowConfig" } + + override predicate isSource(DataFlow::Node source) { + exists(ParseInt parseInt, DataFlow::CallNode call | + call = parseInt.getACall() and call.getArgument(2).getIntValue() = [16] + | + source = call.getResult(0) + ) + or + exists(ParseUint parseUint, DataFlow::CallNode call | + call = parseUint.getACall() and call.getArgument(2).getIntValue() = [16] + | + source = call.getResult(0) + ) + } + + override predicate isSink(DataFlow::Node sink) { + exists(Lte8BitNumericConversionExpr conv | sink.asExpr() = conv) + } + + override predicate isSanitizerIn(DataFlow::Node node) { isSanitizedInsideAnIfBoundCheck(node) } +} + +predicate isSanitizedInsideAnIfBoundCheck(DataFlow::Node node) { + exists(IfRelationalComparison san, Lte32BitNumericConversionExpr conv | + conv = node.asExpr().(Lte32BitNumericConversionExpr) and + san.getThen().getAChild*() = conv and + ( + // If the conversion is inside an `if` block that compares the source as + // `source > 0` or `source >= 0`, then that sanitizes conversion of int to int32; + conv.getTypeName() = "int32" and + san.getComparison().getLesserOperand().getNumericValue() = 0 and + san.getComparison().getGreaterOperand().getGlobalValueNumber() = + conv.getOperand().getGlobalValueNumber() + or + comparisonGreaterOperandValueIsEqualOrLess("int8", san, conv, getMaxInt8()) + or + comparisonGreaterOperandValueIsEqualOrLess("int16", san, conv, getMaxInt16()) + or + comparisonGreaterOperandValueIsEqualOrLess("int32", san, conv, getMaxInt32()) + or + comparisonGreaterOperandValueIsEqualOrLess("uint8", san, conv, getMaxUint8()) + or + comparisonGreaterOperandValueIsEqualOrLess("uint16", san, conv, getMaxUint16()) + ) + ) } int getMaxInt8() { @@ -114,7 +227,7 @@ int getMaxUint16() { } predicate comparisonGreaterOperandValueIsEqualOrLess( - string typeName, IfRelationalComparison ifExpr, NumericConversionExpr conv, int value + string typeName, IfRelationalComparison ifExpr, Lte32BitNumericConversionExpr conv, int value ) { conv.getTypeName() = typeName and ( @@ -138,7 +251,10 @@ predicate comparisonGreaterOperandValueIsEqualOrLess( ) } -from FlowConfig cfg, DataFlow::PathNode source, DataFlow::PathNode sink -where cfg.hasFlowPath(source, sink) +from DataFlow::PathNode source, DataFlow::PathNode sink +where + exists(Lte64FlowConfig cfg | cfg.hasFlowPath(source, sink)) or + exists(Lte32FlowConfig cfg | cfg.hasFlowPath(source, sink)) or + exists(Lte16FlowConfig cfg | cfg.hasFlowPath(source, sink)) select source, source, sink, "Incorrect type conversion of int from strconv.Atoi result to another numeric type" From 05314a19eee9070fb22638213fdfdc1618258c89 Mon Sep 17 00:00:00 2001 From: Slavomir Date: Tue, 7 Apr 2020 16:01:31 +0300 Subject: [PATCH 06/22] Add comments, improve naming --- .../CWE-681/IncorrectNumericConversion.ql | 113 ++++++++++-------- 1 file changed, 65 insertions(+), 48 deletions(-) diff --git a/ql/src/Security/CWE-681/IncorrectNumericConversion.ql b/ql/src/Security/CWE-681/IncorrectNumericConversion.ql index d19bf2332bb..25b7ca43799 100644 --- a/ql/src/Security/CWE-681/IncorrectNumericConversion.ql +++ b/ql/src/Security/CWE-681/IncorrectNumericConversion.ql @@ -1,7 +1,7 @@ /** * @name Incorrect Conversion between Numeric Types - * @description Converting the result of strconv.Atoi (which is of type `int`) to - * numeric types of lower bit size can produce unexpected values. + * @description Converting the result of strconv.Atoi (and other parsers from strconv package) + * to numeric types of lower bit size can produce unexpected values. * @kind path-problem * @id go/incorrect-numeric-conversion * @tags security @@ -27,6 +27,10 @@ class ParseUint extends Function { ParseUint() { this.hasQualifiedName("strconv", "ParseUint") } } +/** + * A type conversion expression towards a numeric type that has + * a bit size equal to or lower than 32 bits. + */ class Lte32BitNumericConversionExpr extends ConversionExpr { string conversionTypeName; @@ -35,13 +39,11 @@ class Lte32BitNumericConversionExpr extends ConversionExpr { conversionTypeName = conv.getTypeExpr().getType().getUnderlyingType*().getName() and ( // anything lower than int64: - conversionTypeName = "int8" or - conversionTypeName = "int16" or - conversionTypeName = "int32" or + conversionTypeName = ["int8", "int16", "int32"] + or // anything lower than uint64: - conversionTypeName = "uint8" or - conversionTypeName = "uint16" or - conversionTypeName = "uint32" or + conversionTypeName = ["uint8", "uint16", "uint32"] + or // anything lower than float64: conversionTypeName = "float32" ) @@ -53,20 +55,27 @@ class Lte32BitNumericConversionExpr extends ConversionExpr { string getTypeName() { result = conversionTypeName } } +/** + * A type conversion expression towards a numeric type that has + * a bit size equal to or lower than 16 bits. + */ class Lte16BitNumericConversionExpr extends Lte32BitNumericConversionExpr { Lte16BitNumericConversionExpr() { conversionTypeName = this.getTypeName() and ( // anything lower than int32: - conversionTypeName = "int8" or - conversionTypeName = "int16" or + conversionTypeName = ["int8", "int16"] + or // anything lower than uint32: - conversionTypeName = "uint8" or - conversionTypeName = "uint16" + conversionTypeName = ["uint8", "uint16"] ) } } +/** + * A type conversion expression towards a numeric type that has + * a bit size equal to 8 bits. + */ class Lte8BitNumericConversionExpr extends Lte16BitNumericConversionExpr { Lte8BitNumericConversionExpr() { conversionTypeName = this.getTypeName() and @@ -80,6 +89,10 @@ class Lte8BitNumericConversionExpr extends Lte16BitNumericConversionExpr { } } +/** + * An `if` statement with the condition being either a relational comparison, + * or one or more `&&`. + */ class IfRelationalComparison extends IfStmt { IfRelationalComparison() { this.getCond() instanceof RelationalComparisonExpr or this.getCond() instanceof LandExpr @@ -90,6 +103,9 @@ class IfRelationalComparison extends IfStmt { LandExpr getLandExpr() { result = this.getCond().(LandExpr) } } +/** + * Flow of result of parsing a 64 bit number, to conversion to lower bit numbers. + */ class Lte64FlowConfig extends TaintTracking::Configuration, DataFlow::Configuration { Lte64FlowConfig() { this = "Lte64FlowConfig" } @@ -97,7 +113,7 @@ class Lte64FlowConfig extends TaintTracking::Configuration, DataFlow::Configurat exists(Atoi atoi | source = atoi.getACall().getResult(0)) or exists(ParseFloat parseFloat, DataFlow::CallNode call | - call = parseFloat.getACall() and call.getArgument(1).getIntValue() = [64] + call = parseFloat.getACall() and call.getArgument(1).getIntValue() = 64 | source = call.getResult(0) ) @@ -122,24 +138,27 @@ class Lte64FlowConfig extends TaintTracking::Configuration, DataFlow::Configurat override predicate isSanitizerIn(DataFlow::Node node) { isSanitizedInsideAnIfBoundCheck(node) } } +/** + * Flow of result of parsing a 32 bit number, to conversion to lower bit numbers. + */ class Lte32FlowConfig extends TaintTracking::Configuration, DataFlow::Configuration { Lte32FlowConfig() { this = "Lte32FlowConfig" } override predicate isSource(DataFlow::Node source) { exists(ParseFloat parseFloat, DataFlow::CallNode call | - call = parseFloat.getACall() and call.getArgument(1).getIntValue() = [32] + call = parseFloat.getACall() and call.getArgument(1).getIntValue() = 32 | source = call.getResult(0) ) or exists(ParseInt parseInt, DataFlow::CallNode call | - call = parseInt.getACall() and call.getArgument(2).getIntValue() = [32] + call = parseInt.getACall() and call.getArgument(2).getIntValue() = 32 | source = call.getResult(0) ) or exists(ParseUint parseUint, DataFlow::CallNode call | - call = parseUint.getACall() and call.getArgument(2).getIntValue() = [32] + call = parseUint.getACall() and call.getArgument(2).getIntValue() = 32 | source = call.getResult(0) ) @@ -152,18 +171,21 @@ class Lte32FlowConfig extends TaintTracking::Configuration, DataFlow::Configurat override predicate isSanitizerIn(DataFlow::Node node) { isSanitizedInsideAnIfBoundCheck(node) } } +/** + * Flow of result of parsing a 16 bit number, to conversion to lower bit numbers. + */ class Lte16FlowConfig extends TaintTracking::Configuration, DataFlow::Configuration { Lte16FlowConfig() { this = "Lte16FlowConfig" } override predicate isSource(DataFlow::Node source) { exists(ParseInt parseInt, DataFlow::CallNode call | - call = parseInt.getACall() and call.getArgument(2).getIntValue() = [16] + call = parseInt.getACall() and call.getArgument(2).getIntValue() = 16 | source = call.getResult(0) ) or exists(ParseUint parseUint, DataFlow::CallNode call | - call = parseUint.getACall() and call.getArgument(2).getIntValue() = [16] + call = parseUint.getACall() and call.getArgument(2).getIntValue() = 16 | source = call.getResult(0) ) @@ -176,57 +198,52 @@ class Lte16FlowConfig extends TaintTracking::Configuration, DataFlow::Configurat override predicate isSanitizerIn(DataFlow::Node node) { isSanitizedInsideAnIfBoundCheck(node) } } +/** + * Check if the node is a numeric conversion inside an `if` body, where + * the `if` condition contains an upper bound check on the conversion operand. + */ predicate isSanitizedInsideAnIfBoundCheck(DataFlow::Node node) { - exists(IfRelationalComparison san, Lte32BitNumericConversionExpr conv | + exists(IfRelationalComparison comp, Lte32BitNumericConversionExpr conv | + // NOTE: using Lte32BitNumericConversionExpr because it also catches + // any lower bit conversions. conv = node.asExpr().(Lte32BitNumericConversionExpr) and - san.getThen().getAChild*() = conv and + comp.getThen().getAChild*() = conv and ( // If the conversion is inside an `if` block that compares the source as // `source > 0` or `source >= 0`, then that sanitizes conversion of int to int32; conv.getTypeName() = "int32" and - san.getComparison().getLesserOperand().getNumericValue() = 0 and - san.getComparison().getGreaterOperand().getGlobalValueNumber() = + comp.getComparison().getLesserOperand().getNumericValue() = 0 and + comp.getComparison().getGreaterOperand().getGlobalValueNumber() = conv.getOperand().getGlobalValueNumber() or - comparisonGreaterOperandValueIsEqualOrLess("int8", san, conv, getMaxInt8()) + comparisonGreaterOperandValueIsEqual("int8", comp, conv, getMaxInt8()) or - comparisonGreaterOperandValueIsEqualOrLess("int16", san, conv, getMaxInt16()) + comparisonGreaterOperandValueIsEqual("int16", comp, conv, getMaxInt16()) or - comparisonGreaterOperandValueIsEqualOrLess("int32", san, conv, getMaxInt32()) + comparisonGreaterOperandValueIsEqual("int32", comp, conv, getMaxInt32()) or - comparisonGreaterOperandValueIsEqualOrLess("uint8", san, conv, getMaxUint8()) + comparisonGreaterOperandValueIsEqual("uint8", comp, conv, getMaxUint8()) or - comparisonGreaterOperandValueIsEqualOrLess("uint16", san, conv, getMaxUint16()) + comparisonGreaterOperandValueIsEqual("uint16", comp, conv, getMaxUint16()) ) ) } -int getMaxInt8() { - result = 2.pow(7) - 1 - // = 1<<7 - 1 -} +int getMaxInt8() { result = 2.pow(7) - 1 } -int getMaxInt16() { - result = 2.pow(15) - 1 - // = 1<<15 - 1 -} +int getMaxInt16() { result = 2.pow(15) - 1 } -int getMaxInt32() { - result = 2.pow(31) - 1 - // = 1<<31 - 1 -} +int getMaxInt32() { result = 2.pow(31) - 1 } -int getMaxUint8() { - result = 2.pow(8) - 1 - // = 1<<8 - 1 -} +int getMaxUint8() { result = 2.pow(8) - 1 } -int getMaxUint16() { - result = 2.pow(16) - 1 - // = 1<<16 - 1 -} +int getMaxUint16() { result = 2.pow(16) - 1 } -predicate comparisonGreaterOperandValueIsEqualOrLess( +/** + * The `if` relational comparison (which can also be inside a `LandExpr`) stating that + * the greater operand is equal to `value`, and the lesses operand is the conversion operand. + */ +predicate comparisonGreaterOperandValueIsEqual( string typeName, IfRelationalComparison ifExpr, Lte32BitNumericConversionExpr conv, int value ) { conv.getTypeName() = typeName and From fe661b227c128b3e135e6f790710925f97c63a3a Mon Sep 17 00:00:00 2001 From: Slavomir Date: Tue, 7 Apr 2020 16:45:52 +0300 Subject: [PATCH 07/22] Improve alert message inside select statement --- .../Security/CWE-681/IncorrectNumericConversion.ql | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/ql/src/Security/CWE-681/IncorrectNumericConversion.ql b/ql/src/Security/CWE-681/IncorrectNumericConversion.ql index 25b7ca43799..129e6a60c5e 100644 --- a/ql/src/Security/CWE-681/IncorrectNumericConversion.ql +++ b/ql/src/Security/CWE-681/IncorrectNumericConversion.ql @@ -268,10 +268,22 @@ predicate comparisonGreaterOperandValueIsEqual( ) } +string getParserQualifiedNameFromResultType(string resultTypeName) { + resultTypeName = "int" and result = "strconv.Atoi" + or + resultTypeName = "int64" and result = "strconv.ParseInt" + or + resultTypeName = "uint64" and result = "strconv.ParseUint" + or + resultTypeName = "float64" and result = "strconv.ParseFloat" +} + from DataFlow::PathNode source, DataFlow::PathNode sink where exists(Lte64FlowConfig cfg | cfg.hasFlowPath(source, sink)) or exists(Lte32FlowConfig cfg | cfg.hasFlowPath(source, sink)) or exists(Lte16FlowConfig cfg | cfg.hasFlowPath(source, sink)) select source, source, sink, - "Incorrect type conversion of int from strconv.Atoi result to another numeric type" + "Incorrect type conversion of " + source.getNode().getType() + " from " + + getParserQualifiedNameFromResultType(source.getNode().getType().toString()) + " result to " + + sink.getNode().asExpr().(Lte32BitNumericConversionExpr).getTypeName() From 57ac636d60830495ef45c8e8e8dc78bc3a2418db Mon Sep 17 00:00:00 2001 From: Slavomir Date: Tue, 7 Apr 2020 17:17:49 +0300 Subject: [PATCH 08/22] Change alert message --- ql/src/Security/CWE-681/IncorrectNumericConversion.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ql/src/Security/CWE-681/IncorrectNumericConversion.ql b/ql/src/Security/CWE-681/IncorrectNumericConversion.ql index 129e6a60c5e..7d0693c94f2 100644 --- a/ql/src/Security/CWE-681/IncorrectNumericConversion.ql +++ b/ql/src/Security/CWE-681/IncorrectNumericConversion.ql @@ -285,5 +285,5 @@ where exists(Lte16FlowConfig cfg | cfg.hasFlowPath(source, sink)) select source, source, sink, "Incorrect type conversion of " + source.getNode().getType() + " from " + - getParserQualifiedNameFromResultType(source.getNode().getType().toString()) + " result to " + + getParserQualifiedNameFromResultType(source.getNode().getType().toString()) + " result to a lower bit size type " + sink.getNode().asExpr().(Lte32BitNumericConversionExpr).getTypeName() From f093226dab64f281cb6f3861d8bf763ee5375aa0 Mon Sep 17 00:00:00 2001 From: Slavomir Date: Tue, 7 Apr 2020 17:25:13 +0300 Subject: [PATCH 09/22] Move query to experimental --- .../CWE-681/IncorrectNumericConversion.go | 0 .../CWE-681/IncorrectNumericConversion.qhelp | 0 .../CWE-681/IncorrectNumericConversion.ql | 0 .../CWE-681/IncorrectNumericConversionGood.go | 0 .../IncorrectNumericConversion.expected | 83 +++++++++++++++++++ .../CWE-681/IncorrectNumericConversion.go | 77 +++++++++++++++++ .../CWE-681/IncorrectNumericConversion.qlref | 1 + .../IncorrectNumericConversion.expected | 43 ---------- .../CWE-681/IncorrectNumericConversion.qlref | 1 - 9 files changed, 161 insertions(+), 44 deletions(-) rename ql/src/{Security => experimental}/CWE-681/IncorrectNumericConversion.go (100%) rename ql/src/{Security => experimental}/CWE-681/IncorrectNumericConversion.qhelp (100%) rename ql/src/{Security => experimental}/CWE-681/IncorrectNumericConversion.ql (100%) rename ql/src/{Security => experimental}/CWE-681/IncorrectNumericConversionGood.go (100%) create mode 100644 ql/test/experimental/CWE-681/IncorrectNumericConversion.expected rename ql/test/{query-tests/Security => experimental}/CWE-681/IncorrectNumericConversion.go (72%) create mode 100644 ql/test/experimental/CWE-681/IncorrectNumericConversion.qlref delete mode 100644 ql/test/query-tests/Security/CWE-681/IncorrectNumericConversion.expected delete mode 100644 ql/test/query-tests/Security/CWE-681/IncorrectNumericConversion.qlref diff --git a/ql/src/Security/CWE-681/IncorrectNumericConversion.go b/ql/src/experimental/CWE-681/IncorrectNumericConversion.go similarity index 100% rename from ql/src/Security/CWE-681/IncorrectNumericConversion.go rename to ql/src/experimental/CWE-681/IncorrectNumericConversion.go diff --git a/ql/src/Security/CWE-681/IncorrectNumericConversion.qhelp b/ql/src/experimental/CWE-681/IncorrectNumericConversion.qhelp similarity index 100% rename from ql/src/Security/CWE-681/IncorrectNumericConversion.qhelp rename to ql/src/experimental/CWE-681/IncorrectNumericConversion.qhelp diff --git a/ql/src/Security/CWE-681/IncorrectNumericConversion.ql b/ql/src/experimental/CWE-681/IncorrectNumericConversion.ql similarity index 100% rename from ql/src/Security/CWE-681/IncorrectNumericConversion.ql rename to ql/src/experimental/CWE-681/IncorrectNumericConversion.ql diff --git a/ql/src/Security/CWE-681/IncorrectNumericConversionGood.go b/ql/src/experimental/CWE-681/IncorrectNumericConversionGood.go similarity index 100% rename from ql/src/Security/CWE-681/IncorrectNumericConversionGood.go rename to ql/src/experimental/CWE-681/IncorrectNumericConversionGood.go diff --git a/ql/test/experimental/CWE-681/IncorrectNumericConversion.expected b/ql/test/experimental/CWE-681/IncorrectNumericConversion.expected new file mode 100644 index 00000000000..286b470f51d --- /dev/null +++ b/ql/test/experimental/CWE-681/IncorrectNumericConversion.expected @@ -0,0 +1,83 @@ +edges +| IncorrectNumericConversion.go:26:2:26:28 | ... := ...[0] : int | IncorrectNumericConversion.go:35:41:35:50 | type conversion | +| IncorrectNumericConversion.go:53:3:53:47 | ... := ...[0] : float64 | IncorrectNumericConversion.go:57:7:57:19 | type conversion | +| IncorrectNumericConversion.go:60:3:60:47 | ... := ...[0] : float64 | IncorrectNumericConversion.go:64:7:64:19 | type conversion | +| IncorrectNumericConversion.go:69:3:69:49 | ... := ...[0] : int64 | IncorrectNumericConversion.go:73:7:73:18 | type conversion | +| IncorrectNumericConversion.go:76:3:76:49 | ... := ...[0] : int64 | IncorrectNumericConversion.go:80:7:80:19 | type conversion | +| IncorrectNumericConversion.go:83:3:83:49 | ... := ...[0] : int64 | IncorrectNumericConversion.go:87:7:87:19 | type conversion | +| IncorrectNumericConversion.go:90:3:90:48 | ... := ...[0] : int64 | IncorrectNumericConversion.go:94:7:94:19 | type conversion | +| IncorrectNumericConversion.go:99:3:99:50 | ... := ...[0] : uint64 | IncorrectNumericConversion.go:103:7:103:18 | type conversion | +| IncorrectNumericConversion.go:106:3:106:50 | ... := ...[0] : uint64 | IncorrectNumericConversion.go:110:7:110:19 | type conversion | +| IncorrectNumericConversion.go:113:3:113:50 | ... := ...[0] : uint64 | IncorrectNumericConversion.go:117:7:117:19 | type conversion | +| IncorrectNumericConversion.go:120:3:120:49 | ... := ...[0] : uint64 | IncorrectNumericConversion.go:124:7:124:19 | type conversion | +| IncorrectNumericConversion.go:131:3:131:36 | ... := ...[0] : int | IncorrectNumericConversion.go:135:7:135:18 | type conversion | +| IncorrectNumericConversion.go:138:3:138:36 | ... := ...[0] : int | IncorrectNumericConversion.go:142:7:142:19 | type conversion | +| IncorrectNumericConversion.go:145:3:145:36 | ... := ...[0] : int | IncorrectNumericConversion.go:149:7:149:19 | type conversion | +| IncorrectNumericConversion.go:152:3:152:36 | ... := ...[0] : int | IncorrectNumericConversion.go:156:7:156:19 | type conversion | +| IncorrectNumericConversion.go:159:3:159:36 | ... := ...[0] : int | IncorrectNumericConversion.go:163:7:163:20 | type conversion | +| IncorrectNumericConversion.go:166:3:166:36 | ... := ...[0] : int | IncorrectNumericConversion.go:170:7:170:20 | type conversion | +| IncorrectNumericConversion.go:173:3:173:36 | ... := ...[0] : int | IncorrectNumericConversion.go:177:7:177:21 | type conversion | +| IncorrectNumericConversion.go:180:3:180:36 | ... := ...[0] : int | IncorrectNumericConversion.go:185:7:185:18 | type conversion | +| IncorrectNumericConversion.go:189:3:189:36 | ... := ...[0] : int | IncorrectNumericConversion.go:193:7:193:23 | type conversion | +nodes +| IncorrectNumericConversion.go:26:2:26:28 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | +| IncorrectNumericConversion.go:35:41:35:50 | type conversion | semmle.label | type conversion | +| IncorrectNumericConversion.go:53:3:53:47 | ... := ...[0] : float64 | semmle.label | ... := ...[0] : float64 | +| IncorrectNumericConversion.go:57:7:57:19 | type conversion | semmle.label | type conversion | +| IncorrectNumericConversion.go:60:3:60:47 | ... := ...[0] : float64 | semmle.label | ... := ...[0] : float64 | +| IncorrectNumericConversion.go:64:7:64:19 | type conversion | semmle.label | type conversion | +| IncorrectNumericConversion.go:69:3:69:49 | ... := ...[0] : int64 | semmle.label | ... := ...[0] : int64 | +| IncorrectNumericConversion.go:73:7:73:18 | type conversion | semmle.label | type conversion | +| IncorrectNumericConversion.go:76:3:76:49 | ... := ...[0] : int64 | semmle.label | ... := ...[0] : int64 | +| IncorrectNumericConversion.go:80:7:80:19 | type conversion | semmle.label | type conversion | +| IncorrectNumericConversion.go:83:3:83:49 | ... := ...[0] : int64 | semmle.label | ... := ...[0] : int64 | +| IncorrectNumericConversion.go:87:7:87:19 | type conversion | semmle.label | type conversion | +| IncorrectNumericConversion.go:90:3:90:48 | ... := ...[0] : int64 | semmle.label | ... := ...[0] : int64 | +| IncorrectNumericConversion.go:94:7:94:19 | type conversion | semmle.label | type conversion | +| IncorrectNumericConversion.go:99:3:99:50 | ... := ...[0] : uint64 | semmle.label | ... := ...[0] : uint64 | +| IncorrectNumericConversion.go:103:7:103:18 | type conversion | semmle.label | type conversion | +| IncorrectNumericConversion.go:106:3:106:50 | ... := ...[0] : uint64 | semmle.label | ... := ...[0] : uint64 | +| IncorrectNumericConversion.go:110:7:110:19 | type conversion | semmle.label | type conversion | +| IncorrectNumericConversion.go:113:3:113:50 | ... := ...[0] : uint64 | semmle.label | ... := ...[0] : uint64 | +| IncorrectNumericConversion.go:117:7:117:19 | type conversion | semmle.label | type conversion | +| IncorrectNumericConversion.go:120:3:120:49 | ... := ...[0] : uint64 | semmle.label | ... := ...[0] : uint64 | +| IncorrectNumericConversion.go:124:7:124:19 | type conversion | semmle.label | type conversion | +| IncorrectNumericConversion.go:131:3:131:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | +| IncorrectNumericConversion.go:135:7:135:18 | type conversion | semmle.label | type conversion | +| IncorrectNumericConversion.go:138:3:138:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | +| IncorrectNumericConversion.go:142:7:142:19 | type conversion | semmle.label | type conversion | +| IncorrectNumericConversion.go:145:3:145:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | +| IncorrectNumericConversion.go:149:7:149:19 | type conversion | semmle.label | type conversion | +| IncorrectNumericConversion.go:152:3:152:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | +| IncorrectNumericConversion.go:156:7:156:19 | type conversion | semmle.label | type conversion | +| IncorrectNumericConversion.go:159:3:159:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | +| IncorrectNumericConversion.go:163:7:163:20 | type conversion | semmle.label | type conversion | +| IncorrectNumericConversion.go:166:3:166:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | +| IncorrectNumericConversion.go:170:7:170:20 | type conversion | semmle.label | type conversion | +| IncorrectNumericConversion.go:173:3:173:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | +| IncorrectNumericConversion.go:177:7:177:21 | type conversion | semmle.label | type conversion | +| IncorrectNumericConversion.go:180:3:180:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | +| IncorrectNumericConversion.go:185:7:185:18 | type conversion | semmle.label | type conversion | +| IncorrectNumericConversion.go:189:3:189:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | +| IncorrectNumericConversion.go:193:7:193:23 | type conversion | semmle.label | type conversion | +#select +| IncorrectNumericConversion.go:26:2:26:28 | ... := ...[0] : int | IncorrectNumericConversion.go:26:2:26:28 | ... := ...[0] : int | IncorrectNumericConversion.go:35:41:35:50 | type conversion | Incorrect type conversion of int from strconv.Atoi result to a lower bit size type int32 | +| IncorrectNumericConversion.go:53:3:53:47 | ... := ...[0] : float64 | IncorrectNumericConversion.go:53:3:53:47 | ... := ...[0] : float64 | IncorrectNumericConversion.go:57:7:57:19 | type conversion | Incorrect type conversion of float64 from strconv.ParseFloat result to a lower bit size type int16 | +| IncorrectNumericConversion.go:60:3:60:47 | ... := ...[0] : float64 | IncorrectNumericConversion.go:60:3:60:47 | ... := ...[0] : float64 | IncorrectNumericConversion.go:64:7:64:19 | type conversion | Incorrect type conversion of float64 from strconv.ParseFloat result to a lower bit size type int32 | +| IncorrectNumericConversion.go:69:3:69:49 | ... := ...[0] : int64 | IncorrectNumericConversion.go:69:3:69:49 | ... := ...[0] : int64 | IncorrectNumericConversion.go:73:7:73:18 | type conversion | Incorrect type conversion of int64 from strconv.ParseInt result to a lower bit size type int8 | +| IncorrectNumericConversion.go:76:3:76:49 | ... := ...[0] : int64 | IncorrectNumericConversion.go:76:3:76:49 | ... := ...[0] : int64 | IncorrectNumericConversion.go:80:7:80:19 | type conversion | Incorrect type conversion of int64 from strconv.ParseInt result to a lower bit size type int16 | +| IncorrectNumericConversion.go:83:3:83:49 | ... := ...[0] : int64 | IncorrectNumericConversion.go:83:3:83:49 | ... := ...[0] : int64 | IncorrectNumericConversion.go:87:7:87:19 | type conversion | Incorrect type conversion of int64 from strconv.ParseInt result to a lower bit size type int32 | +| IncorrectNumericConversion.go:90:3:90:48 | ... := ...[0] : int64 | IncorrectNumericConversion.go:90:3:90:48 | ... := ...[0] : int64 | IncorrectNumericConversion.go:94:7:94:19 | type conversion | Incorrect type conversion of int64 from strconv.ParseInt result to a lower bit size type int32 | +| IncorrectNumericConversion.go:99:3:99:50 | ... := ...[0] : uint64 | IncorrectNumericConversion.go:99:3:99:50 | ... := ...[0] : uint64 | IncorrectNumericConversion.go:103:7:103:18 | type conversion | Incorrect type conversion of uint64 from strconv.ParseUint result to a lower bit size type int8 | +| IncorrectNumericConversion.go:106:3:106:50 | ... := ...[0] : uint64 | IncorrectNumericConversion.go:106:3:106:50 | ... := ...[0] : uint64 | IncorrectNumericConversion.go:110:7:110:19 | type conversion | Incorrect type conversion of uint64 from strconv.ParseUint result to a lower bit size type int16 | +| IncorrectNumericConversion.go:113:3:113:50 | ... := ...[0] : uint64 | IncorrectNumericConversion.go:113:3:113:50 | ... := ...[0] : uint64 | IncorrectNumericConversion.go:117:7:117:19 | type conversion | Incorrect type conversion of uint64 from strconv.ParseUint result to a lower bit size type int32 | +| IncorrectNumericConversion.go:120:3:120:49 | ... := ...[0] : uint64 | IncorrectNumericConversion.go:120:3:120:49 | ... := ...[0] : uint64 | IncorrectNumericConversion.go:124:7:124:19 | type conversion | Incorrect type conversion of uint64 from strconv.ParseUint result to a lower bit size type int32 | +| IncorrectNumericConversion.go:131:3:131:36 | ... := ...[0] : int | IncorrectNumericConversion.go:131:3:131:36 | ... := ...[0] : int | IncorrectNumericConversion.go:135:7:135:18 | type conversion | Incorrect type conversion of int from strconv.Atoi result to a lower bit size type int8 | +| IncorrectNumericConversion.go:138:3:138:36 | ... := ...[0] : int | IncorrectNumericConversion.go:138:3:138:36 | ... := ...[0] : int | IncorrectNumericConversion.go:142:7:142:19 | type conversion | Incorrect type conversion of int from strconv.Atoi result to a lower bit size type int16 | +| IncorrectNumericConversion.go:145:3:145:36 | ... := ...[0] : int | IncorrectNumericConversion.go:145:3:145:36 | ... := ...[0] : int | IncorrectNumericConversion.go:149:7:149:19 | type conversion | Incorrect type conversion of int from strconv.Atoi result to a lower bit size type int32 | +| IncorrectNumericConversion.go:152:3:152:36 | ... := ...[0] : int | IncorrectNumericConversion.go:152:3:152:36 | ... := ...[0] : int | IncorrectNumericConversion.go:156:7:156:19 | type conversion | Incorrect type conversion of int from strconv.Atoi result to a lower bit size type uint8 | +| IncorrectNumericConversion.go:159:3:159:36 | ... := ...[0] : int | IncorrectNumericConversion.go:159:3:159:36 | ... := ...[0] : int | IncorrectNumericConversion.go:163:7:163:20 | type conversion | Incorrect type conversion of int from strconv.Atoi result to a lower bit size type uint16 | +| IncorrectNumericConversion.go:166:3:166:36 | ... := ...[0] : int | IncorrectNumericConversion.go:166:3:166:36 | ... := ...[0] : int | IncorrectNumericConversion.go:170:7:170:20 | type conversion | Incorrect type conversion of int from strconv.Atoi result to a lower bit size type uint32 | +| IncorrectNumericConversion.go:173:3:173:36 | ... := ...[0] : int | IncorrectNumericConversion.go:173:3:173:36 | ... := ...[0] : int | IncorrectNumericConversion.go:177:7:177:21 | type conversion | Incorrect type conversion of int from strconv.Atoi result to a lower bit size type float32 | +| IncorrectNumericConversion.go:180:3:180:36 | ... := ...[0] : int | IncorrectNumericConversion.go:180:3:180:36 | ... := ...[0] : int | IncorrectNumericConversion.go:185:7:185:18 | type conversion | Incorrect type conversion of int from strconv.Atoi result to a lower bit size type uint8 | +| IncorrectNumericConversion.go:189:3:189:36 | ... := ...[0] : int | IncorrectNumericConversion.go:189:3:189:36 | ... := ...[0] : int | IncorrectNumericConversion.go:193:7:193:23 | type conversion | Incorrect type conversion of int from strconv.Atoi result to a lower bit size type int16 | diff --git a/ql/test/query-tests/Security/CWE-681/IncorrectNumericConversion.go b/ql/test/experimental/CWE-681/IncorrectNumericConversion.go similarity index 72% rename from ql/test/query-tests/Security/CWE-681/IncorrectNumericConversion.go rename to ql/test/experimental/CWE-681/IncorrectNumericConversion.go index a437bd70924..3aade6f98df 100644 --- a/ql/test/query-tests/Security/CWE-681/IncorrectNumericConversion.go +++ b/ql/test/experimental/CWE-681/IncorrectNumericConversion.go @@ -48,6 +48,83 @@ const CustomMaxInt16 = 1<<15 - 1 type CustomInt int16 +func badParseFloat() { + { + parsed, err := strconv.ParseFloat("1.32", 32) + if err != nil { + panic(err) + } + _ = int16(parsed) + } + { + parsed, err := strconv.ParseFloat("1.32", 64) + if err != nil { + panic(err) + } + _ = int32(parsed) + } +} +func badParseInt() { + { + parsed, err := strconv.ParseInt("3456", 10, 16) + if err != nil { + panic(err) + } + _ = int8(parsed) + } + { + parsed, err := strconv.ParseInt("3456", 10, 32) + if err != nil { + panic(err) + } + _ = int16(parsed) + } + { + parsed, err := strconv.ParseInt("3456", 10, 64) + if err != nil { + panic(err) + } + _ = int32(parsed) + } + { + parsed, err := strconv.ParseInt("3456", 10, 0) + if err != nil { + panic(err) + } + _ = int32(parsed) + } +} +func badParseUint() { + { + parsed, err := strconv.ParseUint("3456", 10, 16) + if err != nil { + panic(err) + } + _ = int8(parsed) + } + { + parsed, err := strconv.ParseUint("3456", 10, 32) + if err != nil { + panic(err) + } + _ = int16(parsed) + } + { + parsed, err := strconv.ParseUint("3456", 10, 64) + if err != nil { + panic(err) + } + _ = int32(parsed) + } + { + parsed, err := strconv.ParseUint("3456", 10, 0) + if err != nil { + panic(err) + } + _ = int32(parsed) + } +} + // these should be caught: func upperBoundIsNOTChecked(input string) { { diff --git a/ql/test/experimental/CWE-681/IncorrectNumericConversion.qlref b/ql/test/experimental/CWE-681/IncorrectNumericConversion.qlref new file mode 100644 index 00000000000..81a515ce260 --- /dev/null +++ b/ql/test/experimental/CWE-681/IncorrectNumericConversion.qlref @@ -0,0 +1 @@ +experimental/CWE-681/IncorrectNumericConversion.ql \ No newline at end of file diff --git a/ql/test/query-tests/Security/CWE-681/IncorrectNumericConversion.expected b/ql/test/query-tests/Security/CWE-681/IncorrectNumericConversion.expected deleted file mode 100644 index 03a09e927fa..00000000000 --- a/ql/test/query-tests/Security/CWE-681/IncorrectNumericConversion.expected +++ /dev/null @@ -1,43 +0,0 @@ -edges -| IncorrectNumericConversion.go:26:2:26:28 | ... := ...[0] : int | IncorrectNumericConversion.go:35:41:35:50 | type conversion | -| IncorrectNumericConversion.go:54:3:54:36 | ... := ...[0] : int | IncorrectNumericConversion.go:58:7:58:18 | type conversion | -| IncorrectNumericConversion.go:61:3:61:36 | ... := ...[0] : int | IncorrectNumericConversion.go:65:7:65:19 | type conversion | -| IncorrectNumericConversion.go:68:3:68:36 | ... := ...[0] : int | IncorrectNumericConversion.go:72:7:72:19 | type conversion | -| IncorrectNumericConversion.go:75:3:75:36 | ... := ...[0] : int | IncorrectNumericConversion.go:79:7:79:19 | type conversion | -| IncorrectNumericConversion.go:82:3:82:36 | ... := ...[0] : int | IncorrectNumericConversion.go:86:7:86:20 | type conversion | -| IncorrectNumericConversion.go:89:3:89:36 | ... := ...[0] : int | IncorrectNumericConversion.go:93:7:93:20 | type conversion | -| IncorrectNumericConversion.go:96:3:96:36 | ... := ...[0] : int | IncorrectNumericConversion.go:100:7:100:21 | type conversion | -| IncorrectNumericConversion.go:103:3:103:36 | ... := ...[0] : int | IncorrectNumericConversion.go:108:7:108:18 | type conversion | -| IncorrectNumericConversion.go:112:3:112:36 | ... := ...[0] : int | IncorrectNumericConversion.go:116:7:116:23 | type conversion | -nodes -| IncorrectNumericConversion.go:26:2:26:28 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | -| IncorrectNumericConversion.go:35:41:35:50 | type conversion | semmle.label | type conversion | -| IncorrectNumericConversion.go:54:3:54:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | -| IncorrectNumericConversion.go:58:7:58:18 | type conversion | semmle.label | type conversion | -| IncorrectNumericConversion.go:61:3:61:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | -| IncorrectNumericConversion.go:65:7:65:19 | type conversion | semmle.label | type conversion | -| IncorrectNumericConversion.go:68:3:68:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | -| IncorrectNumericConversion.go:72:7:72:19 | type conversion | semmle.label | type conversion | -| IncorrectNumericConversion.go:75:3:75:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | -| IncorrectNumericConversion.go:79:7:79:19 | type conversion | semmle.label | type conversion | -| IncorrectNumericConversion.go:82:3:82:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | -| IncorrectNumericConversion.go:86:7:86:20 | type conversion | semmle.label | type conversion | -| IncorrectNumericConversion.go:89:3:89:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | -| IncorrectNumericConversion.go:93:7:93:20 | type conversion | semmle.label | type conversion | -| IncorrectNumericConversion.go:96:3:96:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | -| IncorrectNumericConversion.go:100:7:100:21 | type conversion | semmle.label | type conversion | -| IncorrectNumericConversion.go:103:3:103:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | -| IncorrectNumericConversion.go:108:7:108:18 | type conversion | semmle.label | type conversion | -| IncorrectNumericConversion.go:112:3:112:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | -| IncorrectNumericConversion.go:116:7:116:23 | type conversion | semmle.label | type conversion | -#select -| IncorrectNumericConversion.go:26:2:26:28 | ... := ...[0] : int | IncorrectNumericConversion.go:26:2:26:28 | ... := ...[0] : int | IncorrectNumericConversion.go:35:41:35:50 | type conversion | Incorrect type conversion of int from strconv.Atoi result to another numeric type | -| IncorrectNumericConversion.go:54:3:54:36 | ... := ...[0] : int | IncorrectNumericConversion.go:54:3:54:36 | ... := ...[0] : int | IncorrectNumericConversion.go:58:7:58:18 | type conversion | Incorrect type conversion of int from strconv.Atoi result to another numeric type | -| IncorrectNumericConversion.go:61:3:61:36 | ... := ...[0] : int | IncorrectNumericConversion.go:61:3:61:36 | ... := ...[0] : int | IncorrectNumericConversion.go:65:7:65:19 | type conversion | Incorrect type conversion of int from strconv.Atoi result to another numeric type | -| IncorrectNumericConversion.go:68:3:68:36 | ... := ...[0] : int | IncorrectNumericConversion.go:68:3:68:36 | ... := ...[0] : int | IncorrectNumericConversion.go:72:7:72:19 | type conversion | Incorrect type conversion of int from strconv.Atoi result to another numeric type | -| IncorrectNumericConversion.go:75:3:75:36 | ... := ...[0] : int | IncorrectNumericConversion.go:75:3:75:36 | ... := ...[0] : int | IncorrectNumericConversion.go:79:7:79:19 | type conversion | Incorrect type conversion of int from strconv.Atoi result to another numeric type | -| IncorrectNumericConversion.go:82:3:82:36 | ... := ...[0] : int | IncorrectNumericConversion.go:82:3:82:36 | ... := ...[0] : int | IncorrectNumericConversion.go:86:7:86:20 | type conversion | Incorrect type conversion of int from strconv.Atoi result to another numeric type | -| IncorrectNumericConversion.go:89:3:89:36 | ... := ...[0] : int | IncorrectNumericConversion.go:89:3:89:36 | ... := ...[0] : int | IncorrectNumericConversion.go:93:7:93:20 | type conversion | Incorrect type conversion of int from strconv.Atoi result to another numeric type | -| IncorrectNumericConversion.go:96:3:96:36 | ... := ...[0] : int | IncorrectNumericConversion.go:96:3:96:36 | ... := ...[0] : int | IncorrectNumericConversion.go:100:7:100:21 | type conversion | Incorrect type conversion of int from strconv.Atoi result to another numeric type | -| IncorrectNumericConversion.go:103:3:103:36 | ... := ...[0] : int | IncorrectNumericConversion.go:103:3:103:36 | ... := ...[0] : int | IncorrectNumericConversion.go:108:7:108:18 | type conversion | Incorrect type conversion of int from strconv.Atoi result to another numeric type | -| IncorrectNumericConversion.go:112:3:112:36 | ... := ...[0] : int | IncorrectNumericConversion.go:112:3:112:36 | ... := ...[0] : int | IncorrectNumericConversion.go:116:7:116:23 | type conversion | Incorrect type conversion of int from strconv.Atoi result to another numeric type | diff --git a/ql/test/query-tests/Security/CWE-681/IncorrectNumericConversion.qlref b/ql/test/query-tests/Security/CWE-681/IncorrectNumericConversion.qlref deleted file mode 100644 index 7e678611eb2..00000000000 --- a/ql/test/query-tests/Security/CWE-681/IncorrectNumericConversion.qlref +++ /dev/null @@ -1 +0,0 @@ -Security/CWE-681/IncorrectNumericConversion.ql \ No newline at end of file From 4517d4513fdc5fc042dcc33d3aa3b31c52166fe7 Mon Sep 17 00:00:00 2001 From: Slavomir Date: Tue, 7 Apr 2020 17:46:16 +0300 Subject: [PATCH 10/22] Update qhelp file and go examples --- .../CWE-681/IncorrectNumericConversion.go | 9 ++++- .../CWE-681/IncorrectNumericConversion.qhelp | 39 ++++++++++++++++--- .../CWE-681/IncorrectNumericConversionGood.go | 19 +++++++++ 3 files changed, 60 insertions(+), 7 deletions(-) diff --git a/ql/src/experimental/CWE-681/IncorrectNumericConversion.go b/ql/src/experimental/CWE-681/IncorrectNumericConversion.go index 11c343550ab..518ca38cee0 100644 --- a/ql/src/experimental/CWE-681/IncorrectNumericConversion.go +++ b/ql/src/experimental/CWE-681/IncorrectNumericConversion.go @@ -4,10 +4,17 @@ import ( "strconv" ) -func parseAllocate(wanted string) int32 { +func parseAllocateBad1(wanted string) int32 { parsed, err := strconv.Atoi(wanted) if err != nil { panic(err) } return int32(parsed) } +func parseAllocateBad2(wanted string) int32 { + parsed, err := strconv.ParseInt(wanted, 10, 64) + if err != nil { + panic(err) + } + return int32(parsed) +} diff --git a/ql/src/experimental/CWE-681/IncorrectNumericConversion.qhelp b/ql/src/experimental/CWE-681/IncorrectNumericConversion.qhelp index 77dd3f1cfac..61bf10d9400 100644 --- a/ql/src/experimental/CWE-681/IncorrectNumericConversion.qhelp +++ b/ql/src/experimental/CWE-681/IncorrectNumericConversion.qhelp @@ -5,16 +5,25 @@

    -If a numeric value string is parsed using strconv.Atoi into an int, and then that int +If a numeric value string is parsed using strconv.Atoi into an int, and subsequently that int is converted into another type of a lower bit size, the result can produce unexpected values.

    +

    +This also applie to the results of strconv.ParseFloat, strconv.ParseInt, +and strconv.ParseUint when the specified bit size is higher than the bit size of the +type that number is converted to. +

    -If you need to parse numeric values with specific bit sizes, use the functions specific to each -type (strconv.ParseFloat, strconv.ParseInt, strconv.ParseUint) -that also allow to specify the wanted bit size. +If you need to parse numeric values with specific bit sizes, avoid strconv.Atoi, and, instead, +use the functions specific to each type (strconv.ParseFloat, strconv.ParseInt, +strconv.ParseUint) that also allow to specify the wanted bit size. +

    +

    +When using those functions, be careful to not convert the result to another type with a lower bit size than +the bit size you specified when parsing the number.

    If this is not possible, then add upper (and lower) bound checks specific to each type and @@ -24,13 +33,13 @@ bit size (you can find the min and max value for each type in the `math` package

    -In the following example, assume that an input string is passed to parseAllocate function, +In the first example, assume that an input string is passed to parseAllocateBad1 function, parsed by strconv.Atoi, and then converted into an int32 type:

    The bounds are not checked, so this means that if the provided number is greater than max int32, -the resulting value from the conversion will be different from the provided value. +the resulting value from the conversion will be different from the actual provided value.

    To avoid unexpected values, you should either use the other functions provided by the strconv @@ -40,6 +49,24 @@ function.

    + + +

    +In the second example, assume that an input string is passed to parseAllocateBad2 function, +parsed by strconv.ParseInt with a bit size set to 64, and then converted into an int32 type: +

    + +

    +If the provided number is greater than max int32, the resulting value from the conversion will be +different from the actual provided value. +

    +

    +To avoid unexpected values, you should specify the correct bit size as in parseAllocateGood3; +or check bounds before making the conversion as in parseAllocateGood4. +

    + +     +
  • mitre.org: CWE-681: Incorrect Conversion between Numeric Types. diff --git a/ql/src/experimental/CWE-681/IncorrectNumericConversionGood.go b/ql/src/experimental/CWE-681/IncorrectNumericConversionGood.go index 003f82cd2a9..29c111cf54e 100644 --- a/ql/src/experimental/CWE-681/IncorrectNumericConversionGood.go +++ b/ql/src/experimental/CWE-681/IncorrectNumericConversionGood.go @@ -30,3 +30,22 @@ func parseAllocateGood2(desired string) int32 { } return int32(parsed) } + +func parseAllocateGood3(wanted string) int32 { + parsed, err := strconv.ParseInt(wanted, 10, 32) + if err != nil { + panic(err) + } + return int32(parsed) +} +func parseAllocateGood4(wanted string) int32 { + parsed, err := strconv.ParseInt(wanted, 10, 64) + if err != nil { + panic(err) + } + // GOOD: check for lower and uppper bounds + if parsed > 0 && parsed <= math.MaxInt32 { + return int32(parsed) + } + return DefaultAllocate +} From d7130873649d3ff3938eccca2f65007d0cf0c719 Mon Sep 17 00:00:00 2001 From: Slavomir Date: Tue, 7 Apr 2020 18:22:52 +0300 Subject: [PATCH 11/22] Mention in qhelp file: CWE-190: Integer Overflow or Wraparound --- ql/src/experimental/CWE-681/IncorrectNumericConversion.qhelp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/ql/src/experimental/CWE-681/IncorrectNumericConversion.qhelp b/ql/src/experimental/CWE-681/IncorrectNumericConversion.qhelp index 61bf10d9400..2a693d1bc57 100644 --- a/ql/src/experimental/CWE-681/IncorrectNumericConversion.qhelp +++ b/ql/src/experimental/CWE-681/IncorrectNumericConversion.qhelp @@ -71,5 +71,8 @@ or check bounds before making the conversion as in parseAllocateGood4 mitre.org: CWE-681: Incorrect Conversion between Numeric Types.
    • +
  • +mitre.org: CWE-190: Integer Overflow or Wraparound. +
    •  From c9c7e6c0a90ff8409a0b0f7863dd1742e9c9882b Mon Sep 17 00:00:00 2001 From: Slavomir Date: Tue, 7 Apr 2020 21:39:06 +0300 Subject: [PATCH 12/22] Add more test cases: add negative cases --- .../IncorrectNumericConversion.expected | 72 ++++++++--------- .../CWE-681/IncorrectNumericConversion.go | 77 +++++++++++++++++++ 2 files changed, 113 insertions(+), 36 deletions(-) diff --git a/ql/test/experimental/CWE-681/IncorrectNumericConversion.expected b/ql/test/experimental/CWE-681/IncorrectNumericConversion.expected index 286b470f51d..4cc0102be01 100644 --- a/ql/test/experimental/CWE-681/IncorrectNumericConversion.expected +++ b/ql/test/experimental/CWE-681/IncorrectNumericConversion.expected @@ -10,15 +10,15 @@ edges | IncorrectNumericConversion.go:106:3:106:50 | ... := ...[0] : uint64 | IncorrectNumericConversion.go:110:7:110:19 | type conversion | | IncorrectNumericConversion.go:113:3:113:50 | ... := ...[0] : uint64 | IncorrectNumericConversion.go:117:7:117:19 | type conversion | | IncorrectNumericConversion.go:120:3:120:49 | ... := ...[0] : uint64 | IncorrectNumericConversion.go:124:7:124:19 | type conversion | -| IncorrectNumericConversion.go:131:3:131:36 | ... := ...[0] : int | IncorrectNumericConversion.go:135:7:135:18 | type conversion | -| IncorrectNumericConversion.go:138:3:138:36 | ... := ...[0] : int | IncorrectNumericConversion.go:142:7:142:19 | type conversion | -| IncorrectNumericConversion.go:145:3:145:36 | ... := ...[0] : int | IncorrectNumericConversion.go:149:7:149:19 | type conversion | -| IncorrectNumericConversion.go:152:3:152:36 | ... := ...[0] : int | IncorrectNumericConversion.go:156:7:156:19 | type conversion | -| IncorrectNumericConversion.go:159:3:159:36 | ... := ...[0] : int | IncorrectNumericConversion.go:163:7:163:20 | type conversion | -| IncorrectNumericConversion.go:166:3:166:36 | ... := ...[0] : int | IncorrectNumericConversion.go:170:7:170:20 | type conversion | -| IncorrectNumericConversion.go:173:3:173:36 | ... := ...[0] : int | IncorrectNumericConversion.go:177:7:177:21 | type conversion | -| IncorrectNumericConversion.go:180:3:180:36 | ... := ...[0] : int | IncorrectNumericConversion.go:185:7:185:18 | type conversion | -| IncorrectNumericConversion.go:189:3:189:36 | ... := ...[0] : int | IncorrectNumericConversion.go:193:7:193:23 | type conversion | +| IncorrectNumericConversion.go:208:3:208:36 | ... := ...[0] : int | IncorrectNumericConversion.go:212:7:212:18 | type conversion | +| IncorrectNumericConversion.go:215:3:215:36 | ... := ...[0] : int | IncorrectNumericConversion.go:219:7:219:19 | type conversion | +| IncorrectNumericConversion.go:222:3:222:36 | ... := ...[0] : int | IncorrectNumericConversion.go:226:7:226:19 | type conversion | +| IncorrectNumericConversion.go:229:3:229:36 | ... := ...[0] : int | IncorrectNumericConversion.go:233:7:233:19 | type conversion | +| IncorrectNumericConversion.go:236:3:236:36 | ... := ...[0] : int | IncorrectNumericConversion.go:240:7:240:20 | type conversion | +| IncorrectNumericConversion.go:243:3:243:36 | ... := ...[0] : int | IncorrectNumericConversion.go:247:7:247:20 | type conversion | +| IncorrectNumericConversion.go:250:3:250:36 | ... := ...[0] : int | IncorrectNumericConversion.go:254:7:254:21 | type conversion | +| IncorrectNumericConversion.go:257:3:257:36 | ... := ...[0] : int | IncorrectNumericConversion.go:262:7:262:18 | type conversion | +| IncorrectNumericConversion.go:266:3:266:36 | ... := ...[0] : int | IncorrectNumericConversion.go:270:7:270:23 | type conversion | nodes | IncorrectNumericConversion.go:26:2:26:28 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | | IncorrectNumericConversion.go:35:41:35:50 | type conversion | semmle.label | type conversion | @@ -42,24 +42,24 @@ nodes | IncorrectNumericConversion.go:117:7:117:19 | type conversion | semmle.label | type conversion | | IncorrectNumericConversion.go:120:3:120:49 | ... := ...[0] : uint64 | semmle.label | ... := ...[0] : uint64 | | IncorrectNumericConversion.go:124:7:124:19 | type conversion | semmle.label | type conversion | -| IncorrectNumericConversion.go:131:3:131:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | -| IncorrectNumericConversion.go:135:7:135:18 | type conversion | semmle.label | type conversion | -| IncorrectNumericConversion.go:138:3:138:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | -| IncorrectNumericConversion.go:142:7:142:19 | type conversion | semmle.label | type conversion | -| IncorrectNumericConversion.go:145:3:145:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | -| IncorrectNumericConversion.go:149:7:149:19 | type conversion | semmle.label | type conversion | -| IncorrectNumericConversion.go:152:3:152:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | -| IncorrectNumericConversion.go:156:7:156:19 | type conversion | semmle.label | type conversion | -| IncorrectNumericConversion.go:159:3:159:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | -| IncorrectNumericConversion.go:163:7:163:20 | type conversion | semmle.label | type conversion | -| IncorrectNumericConversion.go:166:3:166:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | -| IncorrectNumericConversion.go:170:7:170:20 | type conversion | semmle.label | type conversion | -| IncorrectNumericConversion.go:173:3:173:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | -| IncorrectNumericConversion.go:177:7:177:21 | type conversion | semmle.label | type conversion | -| IncorrectNumericConversion.go:180:3:180:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | -| IncorrectNumericConversion.go:185:7:185:18 | type conversion | semmle.label | type conversion | -| IncorrectNumericConversion.go:189:3:189:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | -| IncorrectNumericConversion.go:193:7:193:23 | type conversion | semmle.label | type conversion | +| IncorrectNumericConversion.go:208:3:208:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | +| IncorrectNumericConversion.go:212:7:212:18 | type conversion | semmle.label | type conversion | +| IncorrectNumericConversion.go:215:3:215:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | +| IncorrectNumericConversion.go:219:7:219:19 | type conversion | semmle.label | type conversion | +| IncorrectNumericConversion.go:222:3:222:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | +| IncorrectNumericConversion.go:226:7:226:19 | type conversion | semmle.label | type conversion | +| IncorrectNumericConversion.go:229:3:229:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | +| IncorrectNumericConversion.go:233:7:233:19 | type conversion | semmle.label | type conversion | +| IncorrectNumericConversion.go:236:3:236:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | +| IncorrectNumericConversion.go:240:7:240:20 | type conversion | semmle.label | type conversion | +| IncorrectNumericConversion.go:243:3:243:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | +| IncorrectNumericConversion.go:247:7:247:20 | type conversion | semmle.label | type conversion | +| IncorrectNumericConversion.go:250:3:250:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | +| IncorrectNumericConversion.go:254:7:254:21 | type conversion | semmle.label | type conversion | +| IncorrectNumericConversion.go:257:3:257:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | +| IncorrectNumericConversion.go:262:7:262:18 | type conversion | semmle.label | type conversion | +| IncorrectNumericConversion.go:266:3:266:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | +| IncorrectNumericConversion.go:270:7:270:23 | type conversion | semmle.label | type conversion | #select | IncorrectNumericConversion.go:26:2:26:28 | ... := ...[0] : int | IncorrectNumericConversion.go:26:2:26:28 | ... := ...[0] : int | IncorrectNumericConversion.go:35:41:35:50 | type conversion | Incorrect type conversion of int from strconv.Atoi result to a lower bit size type int32 | | IncorrectNumericConversion.go:53:3:53:47 | ... := ...[0] : float64 | IncorrectNumericConversion.go:53:3:53:47 | ... := ...[0] : float64 | IncorrectNumericConversion.go:57:7:57:19 | type conversion | Incorrect type conversion of float64 from strconv.ParseFloat result to a lower bit size type int16 | @@ -72,12 +72,12 @@ nodes | IncorrectNumericConversion.go:106:3:106:50 | ... := ...[0] : uint64 | IncorrectNumericConversion.go:106:3:106:50 | ... := ...[0] : uint64 | IncorrectNumericConversion.go:110:7:110:19 | type conversion | Incorrect type conversion of uint64 from strconv.ParseUint result to a lower bit size type int16 | | IncorrectNumericConversion.go:113:3:113:50 | ... := ...[0] : uint64 | IncorrectNumericConversion.go:113:3:113:50 | ... := ...[0] : uint64 | IncorrectNumericConversion.go:117:7:117:19 | type conversion | Incorrect type conversion of uint64 from strconv.ParseUint result to a lower bit size type int32 | | IncorrectNumericConversion.go:120:3:120:49 | ... := ...[0] : uint64 | IncorrectNumericConversion.go:120:3:120:49 | ... := ...[0] : uint64 | IncorrectNumericConversion.go:124:7:124:19 | type conversion | Incorrect type conversion of uint64 from strconv.ParseUint result to a lower bit size type int32 | -| IncorrectNumericConversion.go:131:3:131:36 | ... := ...[0] : int | IncorrectNumericConversion.go:131:3:131:36 | ... := ...[0] : int | IncorrectNumericConversion.go:135:7:135:18 | type conversion | Incorrect type conversion of int from strconv.Atoi result to a lower bit size type int8 | -| IncorrectNumericConversion.go:138:3:138:36 | ... := ...[0] : int | IncorrectNumericConversion.go:138:3:138:36 | ... := ...[0] : int | IncorrectNumericConversion.go:142:7:142:19 | type conversion | Incorrect type conversion of int from strconv.Atoi result to a lower bit size type int16 | -| IncorrectNumericConversion.go:145:3:145:36 | ... := ...[0] : int | IncorrectNumericConversion.go:145:3:145:36 | ... := ...[0] : int | IncorrectNumericConversion.go:149:7:149:19 | type conversion | Incorrect type conversion of int from strconv.Atoi result to a lower bit size type int32 | -| IncorrectNumericConversion.go:152:3:152:36 | ... := ...[0] : int | IncorrectNumericConversion.go:152:3:152:36 | ... := ...[0] : int | IncorrectNumericConversion.go:156:7:156:19 | type conversion | Incorrect type conversion of int from strconv.Atoi result to a lower bit size type uint8 | -| IncorrectNumericConversion.go:159:3:159:36 | ... := ...[0] : int | IncorrectNumericConversion.go:159:3:159:36 | ... := ...[0] : int | IncorrectNumericConversion.go:163:7:163:20 | type conversion | Incorrect type conversion of int from strconv.Atoi result to a lower bit size type uint16 | -| IncorrectNumericConversion.go:166:3:166:36 | ... := ...[0] : int | IncorrectNumericConversion.go:166:3:166:36 | ... := ...[0] : int | IncorrectNumericConversion.go:170:7:170:20 | type conversion | Incorrect type conversion of int from strconv.Atoi result to a lower bit size type uint32 | -| IncorrectNumericConversion.go:173:3:173:36 | ... := ...[0] : int | IncorrectNumericConversion.go:173:3:173:36 | ... := ...[0] : int | IncorrectNumericConversion.go:177:7:177:21 | type conversion | Incorrect type conversion of int from strconv.Atoi result to a lower bit size type float32 | -| IncorrectNumericConversion.go:180:3:180:36 | ... := ...[0] : int | IncorrectNumericConversion.go:180:3:180:36 | ... := ...[0] : int | IncorrectNumericConversion.go:185:7:185:18 | type conversion | Incorrect type conversion of int from strconv.Atoi result to a lower bit size type uint8 | -| IncorrectNumericConversion.go:189:3:189:36 | ... := ...[0] : int | IncorrectNumericConversion.go:189:3:189:36 | ... := ...[0] : int | IncorrectNumericConversion.go:193:7:193:23 | type conversion | Incorrect type conversion of int from strconv.Atoi result to a lower bit size type int16 | +| IncorrectNumericConversion.go:208:3:208:36 | ... := ...[0] : int | IncorrectNumericConversion.go:208:3:208:36 | ... := ...[0] : int | IncorrectNumericConversion.go:212:7:212:18 | type conversion | Incorrect type conversion of int from strconv.Atoi result to a lower bit size type int8 | +| IncorrectNumericConversion.go:215:3:215:36 | ... := ...[0] : int | IncorrectNumericConversion.go:215:3:215:36 | ... := ...[0] : int | IncorrectNumericConversion.go:219:7:219:19 | type conversion | Incorrect type conversion of int from strconv.Atoi result to a lower bit size type int16 | +| IncorrectNumericConversion.go:222:3:222:36 | ... := ...[0] : int | IncorrectNumericConversion.go:222:3:222:36 | ... := ...[0] : int | IncorrectNumericConversion.go:226:7:226:19 | type conversion | Incorrect type conversion of int from strconv.Atoi result to a lower bit size type int32 | +| IncorrectNumericConversion.go:229:3:229:36 | ... := ...[0] : int | IncorrectNumericConversion.go:229:3:229:36 | ... := ...[0] : int | IncorrectNumericConversion.go:233:7:233:19 | type conversion | Incorrect type conversion of int from strconv.Atoi result to a lower bit size type uint8 | +| IncorrectNumericConversion.go:236:3:236:36 | ... := ...[0] : int | IncorrectNumericConversion.go:236:3:236:36 | ... := ...[0] : int | IncorrectNumericConversion.go:240:7:240:20 | type conversion | Incorrect type conversion of int from strconv.Atoi result to a lower bit size type uint16 | +| IncorrectNumericConversion.go:243:3:243:36 | ... := ...[0] : int | IncorrectNumericConversion.go:243:3:243:36 | ... := ...[0] : int | IncorrectNumericConversion.go:247:7:247:20 | type conversion | Incorrect type conversion of int from strconv.Atoi result to a lower bit size type uint32 | +| IncorrectNumericConversion.go:250:3:250:36 | ... := ...[0] : int | IncorrectNumericConversion.go:250:3:250:36 | ... := ...[0] : int | IncorrectNumericConversion.go:254:7:254:21 | type conversion | Incorrect type conversion of int from strconv.Atoi result to a lower bit size type float32 | +| IncorrectNumericConversion.go:257:3:257:36 | ... := ...[0] : int | IncorrectNumericConversion.go:257:3:257:36 | ... := ...[0] : int | IncorrectNumericConversion.go:262:7:262:18 | type conversion | Incorrect type conversion of int from strconv.Atoi result to a lower bit size type uint8 | +| IncorrectNumericConversion.go:266:3:266:36 | ... := ...[0] : int | IncorrectNumericConversion.go:266:3:266:36 | ... := ...[0] : int | IncorrectNumericConversion.go:270:7:270:23 | type conversion | Incorrect type conversion of int from strconv.Atoi result to a lower bit size type int16 | diff --git a/ql/test/experimental/CWE-681/IncorrectNumericConversion.go b/ql/test/experimental/CWE-681/IncorrectNumericConversion.go index 3aade6f98df..cdabda91a4a 100644 --- a/ql/test/experimental/CWE-681/IncorrectNumericConversion.go +++ b/ql/test/experimental/CWE-681/IncorrectNumericConversion.go @@ -125,6 +125,83 @@ func badParseUint() { } } +func goodParseFloat() { + { + parsed, err := strconv.ParseFloat("1.32", 32) + if err != nil { + panic(err) + } + _ = int32(parsed) + } + { + parsed, err := strconv.ParseFloat("1.32", 64) + if err != nil { + panic(err) + } + _ = int64(parsed) + } +} +func goodParseInt() { + { + parsed, err := strconv.ParseInt("3456", 10, 16) + if err != nil { + panic(err) + } + _ = int16(parsed) + } + { + parsed, err := strconv.ParseInt("3456", 10, 32) + if err != nil { + panic(err) + } + _ = int32(parsed) + } + { + parsed, err := strconv.ParseInt("3456", 10, 64) + if err != nil { + panic(err) + } + _ = int64(parsed) + } + { + parsed, err := strconv.ParseInt("3456", 10, 0) + if err != nil { + panic(err) + } + _ = int64(parsed) + } +} +func goodParseUint() { + { + parsed, err := strconv.ParseUint("3456", 10, 16) + if err != nil { + panic(err) + } + _ = int16(parsed) + } + { + parsed, err := strconv.ParseUint("3456", 10, 32) + if err != nil { + panic(err) + } + _ = int32(parsed) + } + { + parsed, err := strconv.ParseUint("3456", 10, 64) + if err != nil { + panic(err) + } + _ = int64(parsed) + } + { + parsed, err := strconv.ParseUint("3456", 10, 0) + if err != nil { + panic(err) + } + _ = int64(parsed) + } +} + // these should be caught: func upperBoundIsNOTChecked(input string) { { From 6f1f60896a2edba9aed3693b62b0388b398ecf66 Mon Sep 17 00:00:00 2001 From: Slavomir Date: Mon, 27 Apr 2020 11:33:21 +0300 Subject: [PATCH 13/22] autoformat --- ql/src/experimental/CWE-681/IncorrectNumericConversion.ql | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ql/src/experimental/CWE-681/IncorrectNumericConversion.ql b/ql/src/experimental/CWE-681/IncorrectNumericConversion.ql index 7d0693c94f2..d2c9b7f590c 100644 --- a/ql/src/experimental/CWE-681/IncorrectNumericConversion.ql +++ b/ql/src/experimental/CWE-681/IncorrectNumericConversion.ql @@ -285,5 +285,6 @@ where exists(Lte16FlowConfig cfg | cfg.hasFlowPath(source, sink)) select source, source, sink, "Incorrect type conversion of " + source.getNode().getType() + " from " + - getParserQualifiedNameFromResultType(source.getNode().getType().toString()) + " result to a lower bit size type " + + getParserQualifiedNameFromResultType(source.getNode().getType().toString()) + + " result to a lower bit size type " + sink.getNode().asExpr().(Lte32BitNumericConversionExpr).getTypeName() From b9fae2e5d0276926a16d74333a77b156fc9010a0 Mon Sep 17 00:00:00 2001 From: Slavomir Date: Mon, 27 Apr 2020 13:09:23 +0300 Subject: [PATCH 14/22] Add newline --- ql/test/experimental/CWE-681/IncorrectNumericConversion.qlref | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ql/test/experimental/CWE-681/IncorrectNumericConversion.qlref b/ql/test/experimental/CWE-681/IncorrectNumericConversion.qlref index 81a515ce260..884cf918f1c 100644 --- a/ql/test/experimental/CWE-681/IncorrectNumericConversion.qlref +++ b/ql/test/experimental/CWE-681/IncorrectNumericConversion.qlref @@ -1 +1 @@ -experimental/CWE-681/IncorrectNumericConversion.ql \ No newline at end of file +experimental/CWE-681/IncorrectNumericConversion.ql From 127cd3d003e9ea1d5b3cd8dbd944038fd2835164 Mon Sep 17 00:00:00 2001 From: Slavomir Date: Thu, 30 Apr 2020 16:45:34 +0300 Subject: [PATCH 15/22] Refactor query --- .../CWE-681/IncorrectNumericConversion.ql | 231 ++++++++---------- .../IncorrectNumericConversion.expected | 120 ++++----- 2 files changed, 165 insertions(+), 186 deletions(-) diff --git a/ql/src/experimental/CWE-681/IncorrectNumericConversion.ql b/ql/src/experimental/CWE-681/IncorrectNumericConversion.ql index d2c9b7f590c..1746f70701a 100644 --- a/ql/src/experimental/CWE-681/IncorrectNumericConversion.ql +++ b/ql/src/experimental/CWE-681/IncorrectNumericConversion.ql @@ -27,66 +27,95 @@ class ParseUint extends Function { ParseUint() { this.hasQualifiedName("strconv", "ParseUint") } } -/** - * A type conversion expression towards a numeric type that has - * a bit size equal to or lower than 32 bits. - */ -class Lte32BitNumericConversionExpr extends ConversionExpr { - string conversionTypeName; +module ParserCall { + /** + * A data-flow call node that parses a number. + */ + abstract class Range extends DataFlow::CallNode { + /** Gets the bit size of the result number. */ + abstract int getTargetBitSize(); - Lte32BitNumericConversionExpr() { + /** Gets the name of the parser function. */ + abstract string getParserName(); + } +} + +class ParserCall extends DataFlow::CallNode { + ParserCall::Range self; + + ParserCall() { this = self } + + int getTargetBitSize() { result = self.getTargetBitSize() } + + string getParserName() { result = self.getParserName() } +} + +int archBasedBitSize() { result = 0 } + +class AtoiCall extends DataFlow::CallNode, ParserCall::Range { + AtoiCall() { exists(Atoi atoi | this = atoi.getACall()) } + + override int getTargetBitSize() { result = archBasedBitSize() } + + override string getParserName() { result = "strconv.Atoi" } +} + +class ParseIntCall extends DataFlow::CallNode, ParserCall::Range { + ParseIntCall() { exists(ParseInt parseInt | this = parseInt.getACall()) } + + override int getTargetBitSize() { result = this.getArgument(2).getIntValue() } + + override string getParserName() { result = "strconv.ParseInt" } +} + +class ParseUintCall extends DataFlow::CallNode, ParserCall::Range { + ParseUintCall() { exists(ParseUint parseUint | this = parseUint.getACall()) } + + override int getTargetBitSize() { result = this.getArgument(2).getIntValue() } + + override string getParserName() { result = "strconv.ParseUint" } +} + +class ParseFloatCall extends DataFlow::CallNode, ParserCall::Range { + ParseFloatCall() { exists(ParseFloat parseFloat | this = parseFloat.getACall()) } + + override int getTargetBitSize() { result = this.getArgument(1).getIntValue() } + + override string getParserName() { result = "strconv.ParseFloat" } +} + +class NumericConversionExpr extends ConversionExpr { + string fullTypeName; + int bitSize; + + NumericConversionExpr() { exists(ConversionExpr conv | - conversionTypeName = conv.getTypeExpr().getType().getUnderlyingType*().getName() and + fullTypeName = conv.getTypeExpr().getType().getUnderlyingType*().getName() and ( - // anything lower than int64: - conversionTypeName = ["int8", "int16", "int32"] + // 8 bit + fullTypeName = ["int8", "uint8"] and + bitSize = 8 or - // anything lower than uint64: - conversionTypeName = ["uint8", "uint16", "uint32"] + // 16 bit + fullTypeName = ["int16", "uint16"] and + bitSize = 16 or - // anything lower than float64: - conversionTypeName = "float32" + // 32 bit + fullTypeName = ["int32", "uint32", "float32"] and + bitSize = 32 + or + // 64 bit + fullTypeName = ["int64", "uint64", "float64"] and + bitSize = 64 ) | this = conv ) } - string getTypeName() { result = conversionTypeName } -} + string getFullTypeName() { result = fullTypeName } -/** - * A type conversion expression towards a numeric type that has - * a bit size equal to or lower than 16 bits. - */ -class Lte16BitNumericConversionExpr extends Lte32BitNumericConversionExpr { - Lte16BitNumericConversionExpr() { - conversionTypeName = this.getTypeName() and - ( - // anything lower than int32: - conversionTypeName = ["int8", "int16"] - or - // anything lower than uint32: - conversionTypeName = ["uint8", "uint16"] - ) - } -} - -/** - * A type conversion expression towards a numeric type that has - * a bit size equal to 8 bits. - */ -class Lte8BitNumericConversionExpr extends Lte16BitNumericConversionExpr { - Lte8BitNumericConversionExpr() { - conversionTypeName = this.getTypeName() and - ( - // anything lower than int16: - conversionTypeName = "int8" - or - // anything lower than uint16: - conversionTypeName = "uint8" - ) - } + int getBitSize() { result = bitSize } } /** @@ -106,33 +135,15 @@ class IfRelationalComparison extends IfStmt { /** * Flow of result of parsing a 64 bit number, to conversion to lower bit numbers. */ -class Lte64FlowConfig extends TaintTracking::Configuration, DataFlow::Configuration { - Lte64FlowConfig() { this = "Lte64FlowConfig" } +class Lt64BitFlowConfig extends TaintTracking::Configuration, DataFlow::Configuration { + Lt64BitFlowConfig() { this = "Lt64BitFlowConfig" } override predicate isSource(DataFlow::Node source) { - exists(Atoi atoi | source = atoi.getACall().getResult(0)) - or - exists(ParseFloat parseFloat, DataFlow::CallNode call | - call = parseFloat.getACall() and call.getArgument(1).getIntValue() = 64 - | - source = call.getResult(0) - ) - or - exists(ParseInt parseInt, DataFlow::CallNode call | - call = parseInt.getACall() and call.getArgument(2).getIntValue() = [0, 64] - | - source = call.getResult(0) - ) - or - exists(ParseUint parseUint, DataFlow::CallNode call | - call = parseUint.getACall() and call.getArgument(2).getIntValue() = [0, 64] - | - source = call.getResult(0) - ) + exists(ParserCall call | call.getTargetBitSize() = [0, 64] | source = call) } override predicate isSink(DataFlow::Node sink) { - exists(Lte32BitNumericConversionExpr conv | sink.asExpr() = conv) + exists(NumericConversionExpr conv | conv.getBitSize() = [32, 16, 8] | sink.asExpr() = conv) } override predicate isSanitizerIn(DataFlow::Node node) { isSanitizedInsideAnIfBoundCheck(node) } @@ -141,31 +152,15 @@ class Lte64FlowConfig extends TaintTracking::Configuration, DataFlow::Configurat /** * Flow of result of parsing a 32 bit number, to conversion to lower bit numbers. */ -class Lte32FlowConfig extends TaintTracking::Configuration, DataFlow::Configuration { - Lte32FlowConfig() { this = "Lte32FlowConfig" } +class Lt32BitFlowConfig extends TaintTracking::Configuration, DataFlow::Configuration { + Lt32BitFlowConfig() { this = "Lt32BitFlowConfig" } override predicate isSource(DataFlow::Node source) { - exists(ParseFloat parseFloat, DataFlow::CallNode call | - call = parseFloat.getACall() and call.getArgument(1).getIntValue() = 32 - | - source = call.getResult(0) - ) - or - exists(ParseInt parseInt, DataFlow::CallNode call | - call = parseInt.getACall() and call.getArgument(2).getIntValue() = 32 - | - source = call.getResult(0) - ) - or - exists(ParseUint parseUint, DataFlow::CallNode call | - call = parseUint.getACall() and call.getArgument(2).getIntValue() = 32 - | - source = call.getResult(0) - ) + exists(ParserCall call | call.getTargetBitSize() = [/*0,*/ 32] | source = call) } override predicate isSink(DataFlow::Node sink) { - exists(Lte16BitNumericConversionExpr conv | sink.asExpr() = conv) + exists(NumericConversionExpr conv | conv.getBitSize() = [16, 8] | sink.asExpr() = conv) } override predicate isSanitizerIn(DataFlow::Node node) { isSanitizedInsideAnIfBoundCheck(node) } @@ -174,25 +169,15 @@ class Lte32FlowConfig extends TaintTracking::Configuration, DataFlow::Configurat /** * Flow of result of parsing a 16 bit number, to conversion to lower bit numbers. */ -class Lte16FlowConfig extends TaintTracking::Configuration, DataFlow::Configuration { - Lte16FlowConfig() { this = "Lte16FlowConfig" } +class Lt16BitFlowConfig extends TaintTracking::Configuration, DataFlow::Configuration { + Lt16BitFlowConfig() { this = "Lt16BitFlowConfig" } override predicate isSource(DataFlow::Node source) { - exists(ParseInt parseInt, DataFlow::CallNode call | - call = parseInt.getACall() and call.getArgument(2).getIntValue() = 16 - | - source = call.getResult(0) - ) - or - exists(ParseUint parseUint, DataFlow::CallNode call | - call = parseUint.getACall() and call.getArgument(2).getIntValue() = 16 - | - source = call.getResult(0) - ) + exists(ParserCall call | call.getTargetBitSize() = 16 | source = call) } override predicate isSink(DataFlow::Node sink) { - exists(Lte8BitNumericConversionExpr conv | sink.asExpr() = conv) + exists(NumericConversionExpr conv | conv.getBitSize() = 8 | sink.asExpr() = conv) } override predicate isSanitizerIn(DataFlow::Node node) { isSanitizedInsideAnIfBoundCheck(node) } @@ -203,15 +188,14 @@ class Lte16FlowConfig extends TaintTracking::Configuration, DataFlow::Configurat * the `if` condition contains an upper bound check on the conversion operand. */ predicate isSanitizedInsideAnIfBoundCheck(DataFlow::Node node) { - exists(IfRelationalComparison comp, Lte32BitNumericConversionExpr conv | - // NOTE: using Lte32BitNumericConversionExpr because it also catches - // any lower bit conversions. - conv = node.asExpr().(Lte32BitNumericConversionExpr) and + exists(IfRelationalComparison comp, NumericConversionExpr conv | + conv = node.asExpr().(NumericConversionExpr) and + conv.getBitSize() = [8, 16, 32] and comp.getThen().getAChild*() = conv and ( // If the conversion is inside an `if` block that compares the source as // `source > 0` or `source >= 0`, then that sanitizes conversion of int to int32; - conv.getTypeName() = "int32" and + conv.getFullTypeName() = "int32" and comp.getComparison().getLesserOperand().getNumericValue() = 0 and comp.getComparison().getGreaterOperand().getGlobalValueNumber() = conv.getOperand().getGlobalValueNumber() @@ -244,9 +228,9 @@ int getMaxUint16() { result = 2.pow(16) - 1 } * the greater operand is equal to `value`, and the lesses operand is the conversion operand. */ predicate comparisonGreaterOperandValueIsEqual( - string typeName, IfRelationalComparison ifExpr, Lte32BitNumericConversionExpr conv, int value + string typeName, IfRelationalComparison ifExpr, NumericConversionExpr conv, int value ) { - conv.getTypeName() = typeName and + conv.getFullTypeName() = typeName and ( // exclude cases like: if parsed < math.MaxInt8 {return int8(parsed)} exists(RelationalComparisonExpr comp | comp = ifExpr.getComparison() | @@ -268,23 +252,18 @@ predicate comparisonGreaterOperandValueIsEqual( ) } -string getParserQualifiedNameFromResultType(string resultTypeName) { - resultTypeName = "int" and result = "strconv.Atoi" +string formatBitSize(ParserCall call) { + call.getTargetBitSize() = 0 and result = "(arch-dependent)" or - resultTypeName = "int64" and result = "strconv.ParseInt" - or - resultTypeName = "uint64" and result = "strconv.ParseUint" - or - resultTypeName = "float64" and result = "strconv.ParseFloat" + call.getTargetBitSize() > 0 and result = call.getTargetBitSize().toString() } from DataFlow::PathNode source, DataFlow::PathNode sink where - exists(Lte64FlowConfig cfg | cfg.hasFlowPath(source, sink)) or - exists(Lte32FlowConfig cfg | cfg.hasFlowPath(source, sink)) or - exists(Lte16FlowConfig cfg | cfg.hasFlowPath(source, sink)) -select source, source, sink, - "Incorrect type conversion of " + source.getNode().getType() + " from " + - getParserQualifiedNameFromResultType(source.getNode().getType().toString()) + - " result to a lower bit size type " + - sink.getNode().asExpr().(Lte32BitNumericConversionExpr).getTypeName() + exists(Lt64BitFlowConfig cfg | cfg.hasFlowPath(source, sink)) or + exists(Lt32BitFlowConfig cfg | cfg.hasFlowPath(source, sink)) or + exists(Lt16BitFlowConfig cfg | cfg.hasFlowPath(source, sink)) +select source.getNode(), source, sink, + "Incorrect conversion of a " + formatBitSize(source.getNode().(ParserCall)) + "-bit number from " + + source.getNode().(ParserCall).getParserName() + " result to a lower bit size type " + + sink.getNode().asExpr().(NumericConversionExpr).getFullTypeName() diff --git a/ql/test/experimental/CWE-681/IncorrectNumericConversion.expected b/ql/test/experimental/CWE-681/IncorrectNumericConversion.expected index 4cc0102be01..64aa194012e 100644 --- a/ql/test/experimental/CWE-681/IncorrectNumericConversion.expected +++ b/ql/test/experimental/CWE-681/IncorrectNumericConversion.expected @@ -1,83 +1,83 @@ edges -| IncorrectNumericConversion.go:26:2:26:28 | ... := ...[0] : int | IncorrectNumericConversion.go:35:41:35:50 | type conversion | -| IncorrectNumericConversion.go:53:3:53:47 | ... := ...[0] : float64 | IncorrectNumericConversion.go:57:7:57:19 | type conversion | -| IncorrectNumericConversion.go:60:3:60:47 | ... := ...[0] : float64 | IncorrectNumericConversion.go:64:7:64:19 | type conversion | -| IncorrectNumericConversion.go:69:3:69:49 | ... := ...[0] : int64 | IncorrectNumericConversion.go:73:7:73:18 | type conversion | -| IncorrectNumericConversion.go:76:3:76:49 | ... := ...[0] : int64 | IncorrectNumericConversion.go:80:7:80:19 | type conversion | -| IncorrectNumericConversion.go:83:3:83:49 | ... := ...[0] : int64 | IncorrectNumericConversion.go:87:7:87:19 | type conversion | -| IncorrectNumericConversion.go:90:3:90:48 | ... := ...[0] : int64 | IncorrectNumericConversion.go:94:7:94:19 | type conversion | -| IncorrectNumericConversion.go:99:3:99:50 | ... := ...[0] : uint64 | IncorrectNumericConversion.go:103:7:103:18 | type conversion | -| IncorrectNumericConversion.go:106:3:106:50 | ... := ...[0] : uint64 | IncorrectNumericConversion.go:110:7:110:19 | type conversion | -| IncorrectNumericConversion.go:113:3:113:50 | ... := ...[0] : uint64 | IncorrectNumericConversion.go:117:7:117:19 | type conversion | -| IncorrectNumericConversion.go:120:3:120:49 | ... := ...[0] : uint64 | IncorrectNumericConversion.go:124:7:124:19 | type conversion | -| IncorrectNumericConversion.go:208:3:208:36 | ... := ...[0] : int | IncorrectNumericConversion.go:212:7:212:18 | type conversion | -| IncorrectNumericConversion.go:215:3:215:36 | ... := ...[0] : int | IncorrectNumericConversion.go:219:7:219:19 | type conversion | -| IncorrectNumericConversion.go:222:3:222:36 | ... := ...[0] : int | IncorrectNumericConversion.go:226:7:226:19 | type conversion | -| IncorrectNumericConversion.go:229:3:229:36 | ... := ...[0] : int | IncorrectNumericConversion.go:233:7:233:19 | type conversion | -| IncorrectNumericConversion.go:236:3:236:36 | ... := ...[0] : int | IncorrectNumericConversion.go:240:7:240:20 | type conversion | -| IncorrectNumericConversion.go:243:3:243:36 | ... := ...[0] : int | IncorrectNumericConversion.go:247:7:247:20 | type conversion | -| IncorrectNumericConversion.go:250:3:250:36 | ... := ...[0] : int | IncorrectNumericConversion.go:254:7:254:21 | type conversion | -| IncorrectNumericConversion.go:257:3:257:36 | ... := ...[0] : int | IncorrectNumericConversion.go:262:7:262:18 | type conversion | -| IncorrectNumericConversion.go:266:3:266:36 | ... := ...[0] : int | IncorrectNumericConversion.go:270:7:270:23 | type conversion | +| IncorrectNumericConversion.go:26:14:26:28 | call to Atoi : tuple type | IncorrectNumericConversion.go:35:41:35:50 | type conversion | +| IncorrectNumericConversion.go:53:18:53:47 | call to ParseFloat : tuple type | IncorrectNumericConversion.go:57:7:57:19 | type conversion | +| IncorrectNumericConversion.go:60:18:60:47 | call to ParseFloat : tuple type | IncorrectNumericConversion.go:64:7:64:19 | type conversion | +| IncorrectNumericConversion.go:69:18:69:49 | call to ParseInt : tuple type | IncorrectNumericConversion.go:73:7:73:18 | type conversion | +| IncorrectNumericConversion.go:76:18:76:49 | call to ParseInt : tuple type | IncorrectNumericConversion.go:80:7:80:19 | type conversion | +| IncorrectNumericConversion.go:83:18:83:49 | call to ParseInt : tuple type | IncorrectNumericConversion.go:87:7:87:19 | type conversion | +| IncorrectNumericConversion.go:90:18:90:48 | call to ParseInt : tuple type | IncorrectNumericConversion.go:94:7:94:19 | type conversion | +| IncorrectNumericConversion.go:99:18:99:50 | call to ParseUint : tuple type | IncorrectNumericConversion.go:103:7:103:18 | type conversion | +| IncorrectNumericConversion.go:106:18:106:50 | call to ParseUint : tuple type | IncorrectNumericConversion.go:110:7:110:19 | type conversion | +| IncorrectNumericConversion.go:113:18:113:50 | call to ParseUint : tuple type | IncorrectNumericConversion.go:117:7:117:19 | type conversion | +| IncorrectNumericConversion.go:120:18:120:49 | call to ParseUint : tuple type | IncorrectNumericConversion.go:124:7:124:19 | type conversion | +| IncorrectNumericConversion.go:208:18:208:36 | call to Atoi : tuple type | IncorrectNumericConversion.go:212:7:212:18 | type conversion | +| IncorrectNumericConversion.go:215:18:215:36 | call to Atoi : tuple type | IncorrectNumericConversion.go:219:7:219:19 | type conversion | +| IncorrectNumericConversion.go:222:18:222:36 | call to Atoi : tuple type | IncorrectNumericConversion.go:226:7:226:19 | type conversion | +| IncorrectNumericConversion.go:229:18:229:36 | call to Atoi : tuple type | IncorrectNumericConversion.go:233:7:233:19 | type conversion | +| IncorrectNumericConversion.go:236:18:236:36 | call to Atoi : tuple type | IncorrectNumericConversion.go:240:7:240:20 | type conversion | +| IncorrectNumericConversion.go:243:18:243:36 | call to Atoi : tuple type | IncorrectNumericConversion.go:247:7:247:20 | type conversion | +| IncorrectNumericConversion.go:250:18:250:36 | call to Atoi : tuple type | IncorrectNumericConversion.go:254:7:254:21 | type conversion | +| IncorrectNumericConversion.go:257:18:257:36 | call to Atoi : tuple type | IncorrectNumericConversion.go:262:7:262:18 | type conversion | +| IncorrectNumericConversion.go:266:18:266:36 | call to Atoi : tuple type | IncorrectNumericConversion.go:270:7:270:23 | type conversion | nodes -| IncorrectNumericConversion.go:26:2:26:28 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | +| IncorrectNumericConversion.go:26:14:26:28 | call to Atoi : tuple type | semmle.label | call to Atoi : tuple type | | IncorrectNumericConversion.go:35:41:35:50 | type conversion | semmle.label | type conversion | -| IncorrectNumericConversion.go:53:3:53:47 | ... := ...[0] : float64 | semmle.label | ... := ...[0] : float64 | +| IncorrectNumericConversion.go:53:18:53:47 | call to ParseFloat : tuple type | semmle.label | call to ParseFloat : tuple type | | IncorrectNumericConversion.go:57:7:57:19 | type conversion | semmle.label | type conversion | -| IncorrectNumericConversion.go:60:3:60:47 | ... := ...[0] : float64 | semmle.label | ... := ...[0] : float64 | +| IncorrectNumericConversion.go:60:18:60:47 | call to ParseFloat : tuple type | semmle.label | call to ParseFloat : tuple type | | IncorrectNumericConversion.go:64:7:64:19 | type conversion | semmle.label | type conversion | -| IncorrectNumericConversion.go:69:3:69:49 | ... := ...[0] : int64 | semmle.label | ... := ...[0] : int64 | +| IncorrectNumericConversion.go:69:18:69:49 | call to ParseInt : tuple type | semmle.label | call to ParseInt : tuple type | | IncorrectNumericConversion.go:73:7:73:18 | type conversion | semmle.label | type conversion | -| IncorrectNumericConversion.go:76:3:76:49 | ... := ...[0] : int64 | semmle.label | ... := ...[0] : int64 | +| IncorrectNumericConversion.go:76:18:76:49 | call to ParseInt : tuple type | semmle.label | call to ParseInt : tuple type | | IncorrectNumericConversion.go:80:7:80:19 | type conversion | semmle.label | type conversion | -| IncorrectNumericConversion.go:83:3:83:49 | ... := ...[0] : int64 | semmle.label | ... := ...[0] : int64 | +| IncorrectNumericConversion.go:83:18:83:49 | call to ParseInt : tuple type | semmle.label | call to ParseInt : tuple type | | IncorrectNumericConversion.go:87:7:87:19 | type conversion | semmle.label | type conversion | -| IncorrectNumericConversion.go:90:3:90:48 | ... := ...[0] : int64 | semmle.label | ... := ...[0] : int64 | +| IncorrectNumericConversion.go:90:18:90:48 | call to ParseInt : tuple type | semmle.label | call to ParseInt : tuple type | | IncorrectNumericConversion.go:94:7:94:19 | type conversion | semmle.label | type conversion | -| IncorrectNumericConversion.go:99:3:99:50 | ... := ...[0] : uint64 | semmle.label | ... := ...[0] : uint64 | +| IncorrectNumericConversion.go:99:18:99:50 | call to ParseUint : tuple type | semmle.label | call to ParseUint : tuple type | | IncorrectNumericConversion.go:103:7:103:18 | type conversion | semmle.label | type conversion | -| IncorrectNumericConversion.go:106:3:106:50 | ... := ...[0] : uint64 | semmle.label | ... := ...[0] : uint64 | +| IncorrectNumericConversion.go:106:18:106:50 | call to ParseUint : tuple type | semmle.label | call to ParseUint : tuple type | | IncorrectNumericConversion.go:110:7:110:19 | type conversion | semmle.label | type conversion | -| IncorrectNumericConversion.go:113:3:113:50 | ... := ...[0] : uint64 | semmle.label | ... := ...[0] : uint64 | +| IncorrectNumericConversion.go:113:18:113:50 | call to ParseUint : tuple type | semmle.label | call to ParseUint : tuple type | | IncorrectNumericConversion.go:117:7:117:19 | type conversion | semmle.label | type conversion | -| IncorrectNumericConversion.go:120:3:120:49 | ... := ...[0] : uint64 | semmle.label | ... := ...[0] : uint64 | +| IncorrectNumericConversion.go:120:18:120:49 | call to ParseUint : tuple type | semmle.label | call to ParseUint : tuple type | | IncorrectNumericConversion.go:124:7:124:19 | type conversion | semmle.label | type conversion | -| IncorrectNumericConversion.go:208:3:208:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | +| IncorrectNumericConversion.go:208:18:208:36 | call to Atoi : tuple type | semmle.label | call to Atoi : tuple type | | IncorrectNumericConversion.go:212:7:212:18 | type conversion | semmle.label | type conversion | -| IncorrectNumericConversion.go:215:3:215:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | +| IncorrectNumericConversion.go:215:18:215:36 | call to Atoi : tuple type | semmle.label | call to Atoi : tuple type | | IncorrectNumericConversion.go:219:7:219:19 | type conversion | semmle.label | type conversion | -| IncorrectNumericConversion.go:222:3:222:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | +| IncorrectNumericConversion.go:222:18:222:36 | call to Atoi : tuple type | semmle.label | call to Atoi : tuple type | | IncorrectNumericConversion.go:226:7:226:19 | type conversion | semmle.label | type conversion | -| IncorrectNumericConversion.go:229:3:229:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | +| IncorrectNumericConversion.go:229:18:229:36 | call to Atoi : tuple type | semmle.label | call to Atoi : tuple type | | IncorrectNumericConversion.go:233:7:233:19 | type conversion | semmle.label | type conversion | -| IncorrectNumericConversion.go:236:3:236:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | +| IncorrectNumericConversion.go:236:18:236:36 | call to Atoi : tuple type | semmle.label | call to Atoi : tuple type | | IncorrectNumericConversion.go:240:7:240:20 | type conversion | semmle.label | type conversion | -| IncorrectNumericConversion.go:243:3:243:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | +| IncorrectNumericConversion.go:243:18:243:36 | call to Atoi : tuple type | semmle.label | call to Atoi : tuple type | | IncorrectNumericConversion.go:247:7:247:20 | type conversion | semmle.label | type conversion | -| IncorrectNumericConversion.go:250:3:250:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | +| IncorrectNumericConversion.go:250:18:250:36 | call to Atoi : tuple type | semmle.label | call to Atoi : tuple type | | IncorrectNumericConversion.go:254:7:254:21 | type conversion | semmle.label | type conversion | -| IncorrectNumericConversion.go:257:3:257:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | +| IncorrectNumericConversion.go:257:18:257:36 | call to Atoi : tuple type | semmle.label | call to Atoi : tuple type | | IncorrectNumericConversion.go:262:7:262:18 | type conversion | semmle.label | type conversion | -| IncorrectNumericConversion.go:266:3:266:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int | +| IncorrectNumericConversion.go:266:18:266:36 | call to Atoi : tuple type | semmle.label | call to Atoi : tuple type | | IncorrectNumericConversion.go:270:7:270:23 | type conversion | semmle.label | type conversion | #select -| IncorrectNumericConversion.go:26:2:26:28 | ... := ...[0] : int | IncorrectNumericConversion.go:26:2:26:28 | ... := ...[0] : int | IncorrectNumericConversion.go:35:41:35:50 | type conversion | Incorrect type conversion of int from strconv.Atoi result to a lower bit size type int32 | -| IncorrectNumericConversion.go:53:3:53:47 | ... := ...[0] : float64 | IncorrectNumericConversion.go:53:3:53:47 | ... := ...[0] : float64 | IncorrectNumericConversion.go:57:7:57:19 | type conversion | Incorrect type conversion of float64 from strconv.ParseFloat result to a lower bit size type int16 | -| IncorrectNumericConversion.go:60:3:60:47 | ... := ...[0] : float64 | IncorrectNumericConversion.go:60:3:60:47 | ... := ...[0] : float64 | IncorrectNumericConversion.go:64:7:64:19 | type conversion | Incorrect type conversion of float64 from strconv.ParseFloat result to a lower bit size type int32 | -| IncorrectNumericConversion.go:69:3:69:49 | ... := ...[0] : int64 | IncorrectNumericConversion.go:69:3:69:49 | ... := ...[0] : int64 | IncorrectNumericConversion.go:73:7:73:18 | type conversion | Incorrect type conversion of int64 from strconv.ParseInt result to a lower bit size type int8 | -| IncorrectNumericConversion.go:76:3:76:49 | ... := ...[0] : int64 | IncorrectNumericConversion.go:76:3:76:49 | ... := ...[0] : int64 | IncorrectNumericConversion.go:80:7:80:19 | type conversion | Incorrect type conversion of int64 from strconv.ParseInt result to a lower bit size type int16 | -| IncorrectNumericConversion.go:83:3:83:49 | ... := ...[0] : int64 | IncorrectNumericConversion.go:83:3:83:49 | ... := ...[0] : int64 | IncorrectNumericConversion.go:87:7:87:19 | type conversion | Incorrect type conversion of int64 from strconv.ParseInt result to a lower bit size type int32 | -| IncorrectNumericConversion.go:90:3:90:48 | ... := ...[0] : int64 | IncorrectNumericConversion.go:90:3:90:48 | ... := ...[0] : int64 | IncorrectNumericConversion.go:94:7:94:19 | type conversion | Incorrect type conversion of int64 from strconv.ParseInt result to a lower bit size type int32 | -| IncorrectNumericConversion.go:99:3:99:50 | ... := ...[0] : uint64 | IncorrectNumericConversion.go:99:3:99:50 | ... := ...[0] : uint64 | IncorrectNumericConversion.go:103:7:103:18 | type conversion | Incorrect type conversion of uint64 from strconv.ParseUint result to a lower bit size type int8 | -| IncorrectNumericConversion.go:106:3:106:50 | ... := ...[0] : uint64 | IncorrectNumericConversion.go:106:3:106:50 | ... := ...[0] : uint64 | IncorrectNumericConversion.go:110:7:110:19 | type conversion | Incorrect type conversion of uint64 from strconv.ParseUint result to a lower bit size type int16 | -| IncorrectNumericConversion.go:113:3:113:50 | ... := ...[0] : uint64 | IncorrectNumericConversion.go:113:3:113:50 | ... := ...[0] : uint64 | IncorrectNumericConversion.go:117:7:117:19 | type conversion | Incorrect type conversion of uint64 from strconv.ParseUint result to a lower bit size type int32 | -| IncorrectNumericConversion.go:120:3:120:49 | ... := ...[0] : uint64 | IncorrectNumericConversion.go:120:3:120:49 | ... := ...[0] : uint64 | IncorrectNumericConversion.go:124:7:124:19 | type conversion | Incorrect type conversion of uint64 from strconv.ParseUint result to a lower bit size type int32 | -| IncorrectNumericConversion.go:208:3:208:36 | ... := ...[0] : int | IncorrectNumericConversion.go:208:3:208:36 | ... := ...[0] : int | IncorrectNumericConversion.go:212:7:212:18 | type conversion | Incorrect type conversion of int from strconv.Atoi result to a lower bit size type int8 | -| IncorrectNumericConversion.go:215:3:215:36 | ... := ...[0] : int | IncorrectNumericConversion.go:215:3:215:36 | ... := ...[0] : int | IncorrectNumericConversion.go:219:7:219:19 | type conversion | Incorrect type conversion of int from strconv.Atoi result to a lower bit size type int16 | -| IncorrectNumericConversion.go:222:3:222:36 | ... := ...[0] : int | IncorrectNumericConversion.go:222:3:222:36 | ... := ...[0] : int | IncorrectNumericConversion.go:226:7:226:19 | type conversion | Incorrect type conversion of int from strconv.Atoi result to a lower bit size type int32 | -| IncorrectNumericConversion.go:229:3:229:36 | ... := ...[0] : int | IncorrectNumericConversion.go:229:3:229:36 | ... := ...[0] : int | IncorrectNumericConversion.go:233:7:233:19 | type conversion | Incorrect type conversion of int from strconv.Atoi result to a lower bit size type uint8 | -| IncorrectNumericConversion.go:236:3:236:36 | ... := ...[0] : int | IncorrectNumericConversion.go:236:3:236:36 | ... := ...[0] : int | IncorrectNumericConversion.go:240:7:240:20 | type conversion | Incorrect type conversion of int from strconv.Atoi result to a lower bit size type uint16 | -| IncorrectNumericConversion.go:243:3:243:36 | ... := ...[0] : int | IncorrectNumericConversion.go:243:3:243:36 | ... := ...[0] : int | IncorrectNumericConversion.go:247:7:247:20 | type conversion | Incorrect type conversion of int from strconv.Atoi result to a lower bit size type uint32 | -| IncorrectNumericConversion.go:250:3:250:36 | ... := ...[0] : int | IncorrectNumericConversion.go:250:3:250:36 | ... := ...[0] : int | IncorrectNumericConversion.go:254:7:254:21 | type conversion | Incorrect type conversion of int from strconv.Atoi result to a lower bit size type float32 | -| IncorrectNumericConversion.go:257:3:257:36 | ... := ...[0] : int | IncorrectNumericConversion.go:257:3:257:36 | ... := ...[0] : int | IncorrectNumericConversion.go:262:7:262:18 | type conversion | Incorrect type conversion of int from strconv.Atoi result to a lower bit size type uint8 | -| IncorrectNumericConversion.go:266:3:266:36 | ... := ...[0] : int | IncorrectNumericConversion.go:266:3:266:36 | ... := ...[0] : int | IncorrectNumericConversion.go:270:7:270:23 | type conversion | Incorrect type conversion of int from strconv.Atoi result to a lower bit size type int16 | +| IncorrectNumericConversion.go:26:14:26:28 | call to Atoi | IncorrectNumericConversion.go:26:14:26:28 | call to Atoi : tuple type | IncorrectNumericConversion.go:35:41:35:50 | type conversion | Incorrect conversion of a (arch-dependent)-bit number from strconv.Atoi result to a lower bit size type int32 | +| IncorrectNumericConversion.go:53:18:53:47 | call to ParseFloat | IncorrectNumericConversion.go:53:18:53:47 | call to ParseFloat : tuple type | IncorrectNumericConversion.go:57:7:57:19 | type conversion | Incorrect conversion of a 32-bit number from strconv.ParseFloat result to a lower bit size type int16 | +| IncorrectNumericConversion.go:60:18:60:47 | call to ParseFloat | IncorrectNumericConversion.go:60:18:60:47 | call to ParseFloat : tuple type | IncorrectNumericConversion.go:64:7:64:19 | type conversion | Incorrect conversion of a 64-bit number from strconv.ParseFloat result to a lower bit size type int32 | +| IncorrectNumericConversion.go:69:18:69:49 | call to ParseInt | IncorrectNumericConversion.go:69:18:69:49 | call to ParseInt : tuple type | IncorrectNumericConversion.go:73:7:73:18 | type conversion | Incorrect conversion of a 16-bit number from strconv.ParseInt result to a lower bit size type int8 | +| IncorrectNumericConversion.go:76:18:76:49 | call to ParseInt | IncorrectNumericConversion.go:76:18:76:49 | call to ParseInt : tuple type | IncorrectNumericConversion.go:80:7:80:19 | type conversion | Incorrect conversion of a 32-bit number from strconv.ParseInt result to a lower bit size type int16 | +| IncorrectNumericConversion.go:83:18:83:49 | call to ParseInt | IncorrectNumericConversion.go:83:18:83:49 | call to ParseInt : tuple type | IncorrectNumericConversion.go:87:7:87:19 | type conversion | Incorrect conversion of a 64-bit number from strconv.ParseInt result to a lower bit size type int32 | +| IncorrectNumericConversion.go:90:18:90:48 | call to ParseInt | IncorrectNumericConversion.go:90:18:90:48 | call to ParseInt : tuple type | IncorrectNumericConversion.go:94:7:94:19 | type conversion | Incorrect conversion of a (arch-dependent)-bit number from strconv.ParseInt result to a lower bit size type int32 | +| IncorrectNumericConversion.go:99:18:99:50 | call to ParseUint | IncorrectNumericConversion.go:99:18:99:50 | call to ParseUint : tuple type | IncorrectNumericConversion.go:103:7:103:18 | type conversion | Incorrect conversion of a 16-bit number from strconv.ParseUint result to a lower bit size type int8 | +| IncorrectNumericConversion.go:106:18:106:50 | call to ParseUint | IncorrectNumericConversion.go:106:18:106:50 | call to ParseUint : tuple type | IncorrectNumericConversion.go:110:7:110:19 | type conversion | Incorrect conversion of a 32-bit number from strconv.ParseUint result to a lower bit size type int16 | +| IncorrectNumericConversion.go:113:18:113:50 | call to ParseUint | IncorrectNumericConversion.go:113:18:113:50 | call to ParseUint : tuple type | IncorrectNumericConversion.go:117:7:117:19 | type conversion | Incorrect conversion of a 64-bit number from strconv.ParseUint result to a lower bit size type int32 | +| IncorrectNumericConversion.go:120:18:120:49 | call to ParseUint | IncorrectNumericConversion.go:120:18:120:49 | call to ParseUint : tuple type | IncorrectNumericConversion.go:124:7:124:19 | type conversion | Incorrect conversion of a (arch-dependent)-bit number from strconv.ParseUint result to a lower bit size type int32 | +| IncorrectNumericConversion.go:208:18:208:36 | call to Atoi | IncorrectNumericConversion.go:208:18:208:36 | call to Atoi : tuple type | IncorrectNumericConversion.go:212:7:212:18 | type conversion | Incorrect conversion of a (arch-dependent)-bit number from strconv.Atoi result to a lower bit size type int8 | +| IncorrectNumericConversion.go:215:18:215:36 | call to Atoi | IncorrectNumericConversion.go:215:18:215:36 | call to Atoi : tuple type | IncorrectNumericConversion.go:219:7:219:19 | type conversion | Incorrect conversion of a (arch-dependent)-bit number from strconv.Atoi result to a lower bit size type int16 | +| IncorrectNumericConversion.go:222:18:222:36 | call to Atoi | IncorrectNumericConversion.go:222:18:222:36 | call to Atoi : tuple type | IncorrectNumericConversion.go:226:7:226:19 | type conversion | Incorrect conversion of a (arch-dependent)-bit number from strconv.Atoi result to a lower bit size type int32 | +| IncorrectNumericConversion.go:229:18:229:36 | call to Atoi | IncorrectNumericConversion.go:229:18:229:36 | call to Atoi : tuple type | IncorrectNumericConversion.go:233:7:233:19 | type conversion | Incorrect conversion of a (arch-dependent)-bit number from strconv.Atoi result to a lower bit size type uint8 | +| IncorrectNumericConversion.go:236:18:236:36 | call to Atoi | IncorrectNumericConversion.go:236:18:236:36 | call to Atoi : tuple type | IncorrectNumericConversion.go:240:7:240:20 | type conversion | Incorrect conversion of a (arch-dependent)-bit number from strconv.Atoi result to a lower bit size type uint16 | +| IncorrectNumericConversion.go:243:18:243:36 | call to Atoi | IncorrectNumericConversion.go:243:18:243:36 | call to Atoi : tuple type | IncorrectNumericConversion.go:247:7:247:20 | type conversion | Incorrect conversion of a (arch-dependent)-bit number from strconv.Atoi result to a lower bit size type uint32 | +| IncorrectNumericConversion.go:250:18:250:36 | call to Atoi | IncorrectNumericConversion.go:250:18:250:36 | call to Atoi : tuple type | IncorrectNumericConversion.go:254:7:254:21 | type conversion | Incorrect conversion of a (arch-dependent)-bit number from strconv.Atoi result to a lower bit size type float32 | +| IncorrectNumericConversion.go:257:18:257:36 | call to Atoi | IncorrectNumericConversion.go:257:18:257:36 | call to Atoi : tuple type | IncorrectNumericConversion.go:262:7:262:18 | type conversion | Incorrect conversion of a (arch-dependent)-bit number from strconv.Atoi result to a lower bit size type uint8 | +| IncorrectNumericConversion.go:266:18:266:36 | call to Atoi | IncorrectNumericConversion.go:266:18:266:36 | call to Atoi : tuple type | IncorrectNumericConversion.go:270:7:270:23 | type conversion | Incorrect conversion of a (arch-dependent)-bit number from strconv.Atoi result to a lower bit size type int16 | From 836b8965e21db083a9375729df607bd7da5cf192 Mon Sep 17 00:00:00 2001 From: Slavomir Date: Thu, 30 Apr 2020 16:59:30 +0300 Subject: [PATCH 16/22] Beautify .qhelp file --- .../CWE-681/IncorrectNumericConversion.qhelp | 67 +++++++++---------- 1 file changed, 31 insertions(+), 36 deletions(-) diff --git a/ql/src/experimental/CWE-681/IncorrectNumericConversion.qhelp b/ql/src/experimental/CWE-681/IncorrectNumericConversion.qhelp index 2a693d1bc57..606131ce6d9 100644 --- a/ql/src/experimental/CWE-681/IncorrectNumericConversion.qhelp +++ b/ql/src/experimental/CWE-681/IncorrectNumericConversion.qhelp @@ -1,78 +1,73 @@ +"-//Semmle//qhelp//EN" +"qhelp.dtd"> -

    -If a numeric value string is parsed using strconv.Atoi into an int, and subsequently that int -is converted into another type of a lower bit size, the result can produce unexpected values. + If a numeric value string is parsed using strconv.Atoi into an int, and subsequently that int + is converted into another type of a lower bit size, the result can produce unexpected values.

    -This also applie to the results of strconv.ParseFloat, strconv.ParseInt, -and strconv.ParseUint when the specified bit size is higher than the bit size of the -type that number is converted to. + This also applie to the results of strconv.ParseFloat, strconv.ParseInt, + and strconv.ParseUint when the specified bit size is higher than the bit size of the + type that number is converted to.

    -

    -If you need to parse numeric values with specific bit sizes, avoid strconv.Atoi, and, instead, -use the functions specific to each type (strconv.ParseFloat, strconv.ParseInt, -strconv.ParseUint) that also allow to specify the wanted bit size. + If you need to parse numeric values with specific bit sizes, avoid strconv.Atoi, and, instead, + use the functions specific to each type (strconv.ParseFloat, strconv.ParseInt, + strconv.ParseUint) that also allow to specify the wanted bit size.

    -When using those functions, be careful to not convert the result to another type with a lower bit size than -the bit size you specified when parsing the number. + When using those functions, be careful to not convert the result to another type with a lower bit size than + the bit size you specified when parsing the number.

    -If this is not possible, then add upper (and lower) bound checks specific to each type and -bit size (you can find the min and max value for each type in the `math` package). + If this is not possible, then add upper (and lower) bound checks specific to each type and + bit size (you can find the min and max value for each type in the `math` package).

    -

    -In the first example, assume that an input string is passed to parseAllocateBad1 function, -parsed by strconv.Atoi, and then converted into an int32 type: + In the first example, assume that an input string is passed to parseAllocateBad1 function, + parsed by strconv.Atoi, and then converted into an int32 type:

    -The bounds are not checked, so this means that if the provided number is greater than max int32, -the resulting value from the conversion will be different from the actual provided value. + The bounds are not checked, so this means that if the provided number is greater than max int32, + the resulting value from the conversion will be different from the actual provided value.

    -To avoid unexpected values, you should either use the other functions provided by the strconv -package to parse the specific types and bit sizes; in this case, strconv.ParseInt as you -can see in parseAllocateGood2 function; or check bounds as in parseAllocateGood1 -function. + To avoid unexpected values, you should either use the other functions provided by the strconv + package to parse the specific types and bit sizes; in this case, strconv.ParseInt as you + can see in parseAllocateGood2 function; or check bounds as in parseAllocateGood1 + function.

    -

    -In the second example, assume that an input string is passed to parseAllocateBad2 function, -parsed by strconv.ParseInt with a bit size set to 64, and then converted into an int32 type: + In the second example, assume that an input string is passed to parseAllocateBad2 function, + parsed by strconv.ParseInt with a bit size set to 64, and then converted into an int32 type:

    -If the provided number is greater than max int32, the resulting value from the conversion will be -different from the actual provided value. + If the provided number is greater than max int32, the resulting value from the conversion will be + different from the actual provided value.

    -To avoid unexpected values, you should specify the correct bit size as in parseAllocateGood3; -or check bounds before making the conversion as in parseAllocateGood4. + To avoid unexpected values, you should specify the correct bit size as in parseAllocateGood3; + or check bounds before making the conversion as in parseAllocateGood4.

    -
  • -mitre.org: CWE-681: Incorrect Conversion between Numeric Types. + mitre.org: CWE-681: Incorrect Conversion between Numeric Types.
  • -mitre.org: CWE-190: Integer Overflow or Wraparound. + mitre.org: CWE-190: Integer Overflow or Wraparound.
    • -     + \ No newline at end of file From 5df81d32103fbf6203dc6e643cd241d4994eedbc Mon Sep 17 00:00:00 2001 From: Slavomir Date: Mon, 11 May 2020 12:37:14 +0300 Subject: [PATCH 17/22] Apply suggestions from code review Co-authored-by: Max Schaefer <54907921+max-schaefer@users.noreply.github.com> --- .../CWE-681/IncorrectNumericConversion.qhelp | 22 +++++++++---------- .../CWE-681/IncorrectNumericConversion.ql | 5 +++-- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/ql/src/experimental/CWE-681/IncorrectNumericConversion.qhelp b/ql/src/experimental/CWE-681/IncorrectNumericConversion.qhelp index 606131ce6d9..4174e70cd62 100644 --- a/ql/src/experimental/CWE-681/IncorrectNumericConversion.qhelp +++ b/ql/src/experimental/CWE-681/IncorrectNumericConversion.qhelp @@ -5,27 +5,27 @@

    If a numeric value string is parsed using strconv.Atoi into an int, and subsequently that int - is converted into another type of a lower bit size, the result can produce unexpected values. + is converted into another type of a smaller size, the result can produce unexpected values.

    - This also applie to the results of strconv.ParseFloat, strconv.ParseInt, - and strconv.ParseUint when the specified bit size is higher than the bit size of the + This also applies to the results of strconv.ParseFloat, strconv.ParseInt, + and strconv.ParseUint when the specified size is larger than the size of the type that number is converted to.

    - If you need to parse numeric values with specific bit sizes, avoid strconv.Atoi, and, instead, + If you need to parse numeric values with specific bit sizes, avoid strconv.Atoi, and instead use the functions specific to each type (strconv.ParseFloat, strconv.ParseInt, strconv.ParseUint) that also allow to specify the wanted bit size.

    - When using those functions, be careful to not convert the result to another type with a lower bit size than + When using those functions, be careful to not convert the result to another type with a smaller bit size than the bit size you specified when parsing the number.

    If this is not possible, then add upper (and lower) bound checks specific to each type and - bit size (you can find the min and max value for each type in the `math` package). + bit size (you can find the minimum and maximum value for each type in the `math` package).

    @@ -35,13 +35,13 @@

    - The bounds are not checked, so this means that if the provided number is greater than max int32, + The bounds are not checked, so this means that if the provided number is greater than the maximum value of type int32, the resulting value from the conversion will be different from the actual provided value.

    To avoid unexpected values, you should either use the other functions provided by the strconv - package to parse the specific types and bit sizes; in this case, strconv.ParseInt as you - can see in parseAllocateGood2 function; or check bounds as in parseAllocateGood1 + package to parse the specific types and bit sizes as shown in the + parseAllocateGood2 function; or check bounds as in the parseAllocateGood1 function.

    @@ -53,7 +53,7 @@

    - If the provided number is greater than max int32, the resulting value from the conversion will be + If the provided number is greater than the maximum value of type int32, the resulting value from the conversion will be different from the actual provided value.

    @@ -70,4 +70,4 @@ mitre.org: CWE-190: Integer Overflow or Wraparound. - \ No newline at end of file + diff --git a/ql/src/experimental/CWE-681/IncorrectNumericConversion.ql b/ql/src/experimental/CWE-681/IncorrectNumericConversion.ql index 1746f70701a..e9fa0ad8d95 100644 --- a/ql/src/experimental/CWE-681/IncorrectNumericConversion.ql +++ b/ql/src/experimental/CWE-681/IncorrectNumericConversion.ql @@ -1,10 +1,11 @@ /** - * @name Incorrect Conversion between Numeric Types + * @name Incorrect conversion between numeric types * @description Converting the result of strconv.Atoi (and other parsers from strconv package) - * to numeric types of lower bit size can produce unexpected values. + * to numeric types of smaller bit size can produce unexpected values. * @kind path-problem * @id go/incorrect-numeric-conversion * @tags security + * external/cwe/cwe-190 * external/cwe/cwe-681 */ From 67a7294d10c5430115802cdf2826089bebb0e9e4 Mon Sep 17 00:00:00 2001 From: Slavomir Date: Tue, 12 May 2020 12:51:13 +0300 Subject: [PATCH 18/22] Simplify and remove deprecated; add severity --- .../CWE-681/IncorrectNumericConversion.ql | 31 +++++-------------- 1 file changed, 7 insertions(+), 24 deletions(-) diff --git a/ql/src/experimental/CWE-681/IncorrectNumericConversion.ql b/ql/src/experimental/CWE-681/IncorrectNumericConversion.ql index e9fa0ad8d95..82f2244f081 100644 --- a/ql/src/experimental/CWE-681/IncorrectNumericConversion.ql +++ b/ql/src/experimental/CWE-681/IncorrectNumericConversion.ql @@ -3,6 +3,7 @@ * @description Converting the result of strconv.Atoi (and other parsers from strconv package) * to numeric types of smaller bit size can produce unexpected values. * @kind path-problem + * @problem.severity warning * @id go/incorrect-numeric-conversion * @tags security * external/cwe/cwe-190 @@ -28,6 +29,7 @@ class ParseUint extends Function { ParseUint() { this.hasQualifiedName("strconv", "ParseUint") } } +/** Provides a class for modeling number parser calls. */ module ParserCall { /** * A data-flow call node that parses a number. @@ -51,12 +53,10 @@ class ParserCall extends DataFlow::CallNode { string getParserName() { result = self.getParserName() } } -int archBasedBitSize() { result = 0 } - class AtoiCall extends DataFlow::CallNode, ParserCall::Range { AtoiCall() { exists(Atoi atoi | this = atoi.getACall()) } - override int getTargetBitSize() { result = archBasedBitSize() } + override int getTargetBitSize() { result = 0 } override string getParserName() { result = "strconv.Atoi" } } @@ -90,27 +90,10 @@ class NumericConversionExpr extends ConversionExpr { int bitSize; NumericConversionExpr() { - exists(ConversionExpr conv | - fullTypeName = conv.getTypeExpr().getType().getUnderlyingType*().getName() and - ( - // 8 bit - fullTypeName = ["int8", "uint8"] and - bitSize = 8 - or - // 16 bit - fullTypeName = ["int16", "uint16"] and - bitSize = 16 - or - // 32 bit - fullTypeName = ["int32", "uint32", "float32"] and - bitSize = 32 - or - // 64 bit - fullTypeName = ["int64", "uint64", "float64"] and - bitSize = 64 - ) - | - this = conv + exists(NumericType conv | + conv = getTypeExpr().getType().getUnderlyingType() and + fullTypeName = conv.getName() and + bitSize = conv.getSize() ) } From ea7c38c99c5d86eb3c4bddf1456575ca9c77afa3 Mon Sep 17 00:00:00 2001 From: Slavomir Date: Tue, 12 May 2020 13:00:27 +0300 Subject: [PATCH 19/22] Remove references section from qhelp file --- .../experimental/CWE-681/IncorrectNumericConversion.qhelp | 8 -------- 1 file changed, 8 deletions(-) diff --git a/ql/src/experimental/CWE-681/IncorrectNumericConversion.qhelp b/ql/src/experimental/CWE-681/IncorrectNumericConversion.qhelp index 4174e70cd62..f978858ed89 100644 --- a/ql/src/experimental/CWE-681/IncorrectNumericConversion.qhelp +++ b/ql/src/experimental/CWE-681/IncorrectNumericConversion.qhelp @@ -62,12 +62,4 @@

    - -
  • - mitre.org: CWE-681: Incorrect Conversion between Numeric Types. -
    • -
  • - mitre.org: CWE-190: Integer Overflow or Wraparound. -
    • -     From 623d5b3a97e7723b545719bf433e2e8774b00812 Mon Sep 17 00:00:00 2001 From: Slavomir Date: Tue, 12 May 2020 13:00:50 +0300 Subject: [PATCH 20/22] Add comments --- ql/src/experimental/CWE-681/IncorrectNumericConversion.ql | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/ql/src/experimental/CWE-681/IncorrectNumericConversion.ql b/ql/src/experimental/CWE-681/IncorrectNumericConversion.ql index 82f2244f081..0b157a879e9 100644 --- a/ql/src/experimental/CWE-681/IncorrectNumericConversion.ql +++ b/ql/src/experimental/CWE-681/IncorrectNumericConversion.ql @@ -13,23 +13,27 @@ import go import DataFlow::PathGraph +/** A function that parses integers. */ class Atoi extends Function { Atoi() { this.hasQualifiedName("strconv", "Atoi") } } +/** A function that parses floating-point numbers. */ class ParseFloat extends Function { ParseFloat() { this.hasQualifiedName("strconv", "ParseFloat") } } +/** A function that parses integers with a specifiable bitSize. */ class ParseInt extends Function { ParseInt() { this.hasQualifiedName("strconv", "ParseInt") } } +/** A function that parses unsigned integers with a specifiable bitSize. */ class ParseUint extends Function { ParseUint() { this.hasQualifiedName("strconv", "ParseUint") } } -/** Provides a class for modeling number parser calls. */ +/** Provides a class for modeling calls to number-parsing functions. */ module ParserCall { /** * A data-flow call node that parses a number. From e5e74f34d7504951f8c4e2ef9ab12e02fdee152f Mon Sep 17 00:00:00 2001 From: Slavomir Date: Tue, 12 May 2020 13:06:11 +0300 Subject: [PATCH 21/22] Add note on why the zero is commented out in Lt32BitFlowConfig --- ql/src/experimental/CWE-681/IncorrectNumericConversion.ql | 1 + 1 file changed, 1 insertion(+) diff --git a/ql/src/experimental/CWE-681/IncorrectNumericConversion.ql b/ql/src/experimental/CWE-681/IncorrectNumericConversion.ql index 0b157a879e9..60116a65111 100644 --- a/ql/src/experimental/CWE-681/IncorrectNumericConversion.ql +++ b/ql/src/experimental/CWE-681/IncorrectNumericConversion.ql @@ -144,6 +144,7 @@ class Lt32BitFlowConfig extends TaintTracking::Configuration, DataFlow::Configur Lt32BitFlowConfig() { this = "Lt32BitFlowConfig" } override predicate isSource(DataFlow::Node source) { + // NOTE: target bit size 0 is already addressed in Lt64BitFlowConfig. exists(ParserCall call | call.getTargetBitSize() = [/*0,*/ 32] | source = call) } From 556f527193be81040b272c440ce967dfd95de7b2 Mon Sep 17 00:00:00 2001 From: Slavomir Date: Tue, 12 May 2020 13:12:47 +0300 Subject: [PATCH 22/22] Exclude results in test files --- .../CWE-681/IncorrectNumericConversion.ql | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/ql/src/experimental/CWE-681/IncorrectNumericConversion.ql b/ql/src/experimental/CWE-681/IncorrectNumericConversion.ql index 60116a65111..def77b696e9 100644 --- a/ql/src/experimental/CWE-681/IncorrectNumericConversion.ql +++ b/ql/src/experimental/CWE-681/IncorrectNumericConversion.ql @@ -249,9 +249,17 @@ string formatBitSize(ParserCall call) { from DataFlow::PathNode source, DataFlow::PathNode sink where - exists(Lt64BitFlowConfig cfg | cfg.hasFlowPath(source, sink)) or - exists(Lt32BitFlowConfig cfg | cfg.hasFlowPath(source, sink)) or - exists(Lt16BitFlowConfig cfg | cfg.hasFlowPath(source, sink)) + ( + exists(Lt64BitFlowConfig cfg | cfg.hasFlowPath(source, sink)) + or + exists(Lt32BitFlowConfig cfg | cfg.hasFlowPath(source, sink)) + or + exists(Lt16BitFlowConfig cfg | cfg.hasFlowPath(source, sink)) + ) and + // Exclude results in test files: + exists(File fl | fl = sink.getNode().asExpr().(NumericConversionExpr).getFile() | + not fl instanceof TestFile + ) select source.getNode(), source, sink, "Incorrect conversion of a " + formatBitSize(source.getNode().(ParserCall)) + "-bit number from " + source.getNode().(ParserCall).getParserName() + " result to a lower bit size type " +