Skip to content
This repository was archived by the owner on Jan 5, 2023. It is now read-only.

Commit 329888e

Browse files
owen-mcowen-mc
committed
Add query for incorrect integer conversion
1 parent 34fa072 commit 329888e

File tree

4 files changed

+280
-0
lines changed

4 files changed

+280
-0
lines changed

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

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
package main
2+
3+
import (
4+
"strconv"
5+
)
6+
7+
func parseAllocateBad1(wanted string) int32 {
8+
parsed, err := strconv.Atoi(wanted)
9+
if err != nil {
10+
panic(err)
11+
}
12+
return int32(parsed)
13+
}
14+
func parseAllocateBad2(wanted string) int32 {
15+
parsed, err := strconv.ParseInt(wanted, 10, 64)
16+
if err != nil {
17+
panic(err)
18+
}
19+
return int32(parsed)
20+
}

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

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>
7+
If a string is parsed into an int using <code>strconv.Atoi</code>, and subsequently that int
8+
is converted into another integer type of a smaller size, the result can produce unexpected values.
9+
</p>
10+
<p>
11+
This also applies to the results of <code>strconv.ParseInt</code> and <code>strconv.ParseUint</code> when
12+
the specified size is larger than the size of the type that number is converted to.
13+
</p>
14+
</overview>
15+
16+
<recommendation>
17+
<p>
18+
If you need to parse integer values with specific bit sizes, avoid <code>strconv.Atoi</code>, and instead
19+
use <code>strconv.ParseInt</code> or <code>strconv.ParseUint</code>, which also allow specifying the
20+
bit size.
21+
</p>
22+
<p>
23+
When using those functions, be careful to not convert the result to another type with a smaller bit size than
24+
the bit size you specified when parsing the number.
25+
</p>
26+
<p>
27+
If this is not possible, then add upper (and lower) bound checks specific to each type and
28+
bit size (you can find the minimum and maximum value for each type in the `math` package).
29+
</p>
30+
</recommendation>
31+
32+
<example>
33+
<p>
34+
In the first example, assume that an input string is passed to <code>parseAllocateBad1</code> function,
35+
parsed by <code>strconv.Atoi</code>, and then converted into an <code>int32</code> type:
36+
</p>
37+
<sample src="IncorrectIntegerConversion.go"/>
38+
<p>
39+
The bounds are not checked, so this means that if the provided number is greater than the maximum value of type <code>int32</code>,
40+
the resulting value from the conversion will be different from the actual provided value.
41+
</p>
42+
<p>
43+
To avoid unexpected values, you should either use the other functions provided by the <code>strconv</code>
44+
package to parse the specific types and bit sizes as shown in the
45+
<code>parseAllocateGood2</code> function; or check bounds as in the <code>parseAllocateGood1</code>
46+
function.
47+
</p>
48+
<sample src="IncorrectIntegerConversionGood.go"/>
49+
</example>
50+
51+
<example>
52+
<p>
53+
In the second example, assume that an input string is passed to <code>parseAllocateBad2</code> function,
54+
parsed by <code>strconv.ParseInt</code> with a bit size set to 64, and then converted into an <code>int32</code> type:
55+
</p>
56+
<sample src="IncorrectIntegerConversion.go"/>
57+
<p>
58+
If the provided number is greater than the maximum value of type <code>int32</code>, the resulting value from the conversion will be
59+
different from the actual provided value.
60+
</p>
61+
<p>
62+
To avoid unexpected values, you should specify the correct bit size as in <code>parseAllocateGood3</code>;
63+
or check bounds before making the conversion as in <code>parseAllocateGood4</code>.
64+
</p>
65+
<sample src="IncorrectIntegerConversionGood.go"/>
66+
</example>
67+
</qhelp>

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

Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,142 @@
1+
/**
2+
* @name Incorrect conversion between integer types
3+
* @description Converting the result of strconv.Atoi, strconv.ParseInt and strconv.ParseUint
4+
* to integer types of smaller bit size can produce unexpected values.
5+
* @kind path-problem
6+
* @problem.severity warning
7+
* @id go/incorrect-integer-conversion
8+
* @tags security
9+
* external/cwe/cwe-190
10+
* external/cwe/cwe-681
11+
* @precision very-high
12+
*/
13+
14+
import go
15+
import DataFlow::PathGraph
16+
17+
/**
18+
* Gets the maximum value of an integer (signed if `isSigned`
19+
* is true, unsigned otherwise) with `bitSize` bits.
20+
*/
21+
float getMaxIntValue(int bitSize, boolean isSigned) {
22+
bitSize in [8, 16, 32, 64] and
23+
(
24+
isSigned = true and result = 2.pow(bitSize - 1) - 1
25+
or
26+
isSigned = false and result = 2.pow(bitSize) - 1
27+
)
28+
}
29+
30+
/**
31+
* Holds if converting from an integer types with size `sourceBitSize` to
32+
* one with size `sinkBitSize` can produce unexpected values, where 0 means
33+
* architecture-dependent.
34+
*/
35+
private predicate isIncorrectIntegerConversion(int sourceBitSize, int sinkBitSize) {
36+
sourceBitSize in [0, 16, 32, 64] and
37+
sinkBitSize in [0, 8, 16, 32] and
38+
not (sourceBitSize = 0 and sinkBitSize = 0) and
39+
exists(int source, int sink |
40+
(if sourceBitSize = 0 then source = 64 else source = sourceBitSize) and
41+
if sinkBitSize = 0 then sink = 32 else sink = sinkBitSize
42+
|
43+
source > sink
44+
)
45+
}
46+
47+
/**
48+
* A taint-tracking configuration for reasoning about when an integer
49+
* obtained from parsing a string flows to a type conversion to a smaller
50+
* integer types, which could cause unexpected values.
51+
*/
52+
class ConversionWithoutBoundsCheckConfig extends TaintTracking::Configuration {
53+
boolean sourceIsSigned;
54+
int sourceBitSize;
55+
int sinkBitSize;
56+
57+
ConversionWithoutBoundsCheckConfig() {
58+
sourceIsSigned in [true, false] and
59+
isIncorrectIntegerConversion(sourceBitSize, sinkBitSize) and
60+
this =
61+
sourceBitSize.toString() + sourceIsSigned.toString() + sinkBitSize.toString() +
62+
"ConversionWithoutBoundsCheckConfig"
63+
}
64+
65+
override predicate isSource(DataFlow::Node source) {
66+
exists(ParserCall pc, int bitSize | source = pc.getResult(0) |
67+
sourceIsSigned = pc.getTargetIsSigned() and
68+
(if pc.getTargetBitSize() = 0 then bitSize = 0 else bitSize = pc.getTargetBitSize()) and
69+
// `bitSize` could be any value between 0 and 64, but we can round
70+
// it up to the nearest size of an integer type without changing
71+
// behaviour.
72+
sourceBitSize = min(int b | b in [0, 8, 16, 32, 64] and b >= bitSize)
73+
)
74+
}
75+
76+
/**
77+
* Holds if sink is a typecast to an integer type with size `bitSize` (where
78+
* 0 represents architecture-dependent) and the expression being typecast is
79+
* not also in a right-shift expression.
80+
*/
81+
predicate isSink(DataFlow::TypeCastNode sink, int bitSize) {
82+
exists(IntegerType integerType | sink.getType().getUnderlyingType() = integerType |
83+
bitSize = integerType.getSize()
84+
or
85+
(
86+
integerType instanceof IntType or
87+
integerType instanceof UintType
88+
) and
89+
bitSize = 0
90+
) and
91+
not exists(ShrExpr shrExpr |
92+
shrExpr.getLeftOperand().getGlobalValueNumber() =
93+
sink.getOperand().asExpr().getGlobalValueNumber()
94+
)
95+
}
96+
97+
override predicate isSink(DataFlow::Node sink) { isSink(sink, sinkBitSize) }
98+
99+
override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
100+
exists(int bitSize | if sinkBitSize != 0 then bitSize = sinkBitSize else bitSize = 32 |
101+
guard.(UpperBoundCheckGuard).getBound() <= getMaxIntValue(bitSize, sourceIsSigned)
102+
)
103+
}
104+
105+
override predicate isSanitizerOut(DataFlow::Node node) {
106+
exists(int bitSize | isIncorrectIntegerConversion(sourceBitSize, bitSize) |
107+
isSink(node, bitSize)
108+
)
109+
}
110+
}
111+
112+
/** An upper bound check that compares a variable to a constant value. */
113+
class UpperBoundCheckGuard extends DataFlow::BarrierGuard, DataFlow::RelationalComparisonNode {
114+
UpperBoundCheckGuard() { count(expr.getAnOperand().getIntValue()) = 1 }
115+
116+
/**
117+
* Gets the constant value which this upper bound check ensures the
118+
* other value is less than or equal to.
119+
*/
120+
float getBound() {
121+
exists(int strictnessOffset |
122+
if expr.isStrict() then strictnessOffset = 1 else strictnessOffset = 0
123+
|
124+
result = expr.getAnOperand().getIntValue() - strictnessOffset
125+
)
126+
}
127+
128+
override predicate checks(Expr e, boolean branch) {
129+
this.leq(branch, DataFlow::exprNode(e), _, _) and
130+
not exists(e.getIntValue())
131+
}
132+
}
133+
134+
from
135+
DataFlow::PathNode source, DataFlow::PathNode sink, ConversionWithoutBoundsCheckConfig cfg,
136+
ParserCall pc
137+
where cfg.hasFlowPath(source, sink) and pc.getResult(0) = source.getNode()
138+
select source.getNode(), source, sink,
139+
"Incorrect conversion of " + pc.getBitSizeString() + " from " + pc.getParserName() +
140+
" to a lower bit size type " +
141+
sink.getNode().(DataFlow::TypeCastNode).getType().getUnderlyingType().getName() +
142+
" without an upper bound check."

‎ql/src/Security/CWE-681/IncorrectIntegerConversionGood.go

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
package main
2+
3+
import (
4+
"math"
5+
"strconv"
6+
)
7+
8+
func main() {
9+
10+
}
11+
12+
const DefaultAllocate int32 = 256
13+
14+
func parseAllocateGood1(desired string) int32 {
15+
parsed, err := strconv.Atoi(desired)
16+
if err != nil {
17+
return DefaultAllocate
18+
}
19+
// GOOD: check for lower and upper bounds
20+
if parsed > 0 && parsed <= math.MaxInt32 {
21+
return int32(parsed)
22+
}
23+
return DefaultAllocate
24+
}
25+
func parseAllocateGood2(desired string) int32 {
26+
// GOOD: parse specifying the bit size
27+
parsed, err := strconv.ParseInt(desired, 10, 32)
28+
if err != nil {
29+
return DefaultAllocate
30+
}
31+
return int32(parsed)
32+
}
33+
34+
func parseAllocateGood3(wanted string) int32 {
35+
parsed, err := strconv.ParseInt(wanted, 10, 32)
36+
if err != nil {
37+
panic(err)
38+
}
39+
return int32(parsed)
40+
}
41+
func parseAllocateGood4(wanted string) int32 {
42+
parsed, err := strconv.ParseInt(wanted, 10, 64)
43+
if err != nil {
44+
panic(err)
45+
}
46+
// GOOD: check for lower and uppper bounds
47+
if parsed > 0 && parsed <= math.MaxInt32 {
48+
return int32(parsed)
49+
}
50+
return DefaultAllocate
51+
}

0 commit comments

Comments
 (0)