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

Commit 30f1762

Browse files
owen-mcowen-mc
committed
Address review comments 3
1 parent 89eae10 commit 30f1762

File tree

5 files changed

+57
-5
lines changed

5 files changed

+57
-5
lines changed

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,4 +64,11 @@ or check bounds before making the conversion as in <code>parseAllocateGood4</cod
6464
</p>
6565
<sample src="IncorrectIntegerConversionGood.go"/>
6666
</example>
67+
<references>
68+
<li>Wikipedia <a href="https://en.wikipedia.org/wiki/Integer_overflow">Integer overflow</a>.</li>
69+
<li>Go language specification <a href="https://golang.org/ref/spec#Integer_overflow">Integer overflow</a>.</li>
70+
<li>Documentation for <a href="https://golang.org/pkg/strconv/#Atoi"><code>strconv.Atoi</code></a>.</li>
71+
<li>Documentation for <a href="https://golang.org/pkg/strconv/#ParseInt"><code>strconv.ParseInt</code></a>.</li>
72+
<li>Documentation for <a href="https://golang.org/pkg/strconv/#ParseUint"><code>strconv.ParseUint</code></a>.</li>
73+
</references>
6774
</qhelp>

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

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,7 @@ class ConversionWithoutBoundsCheckConfig extends TaintTracking::Configuration {
6161
this = "ConversionWithoutBoundsCheckConfig" + sourceBitSize + sourceIsSigned + sinkBitSize
6262
}
6363

64+
/** Gets the bit size of the source. */
6465
int getSourceBitSize() { result = sourceBitSize }
6566

6667
override predicate isSource(DataFlow::Node source) {
@@ -73,8 +74,14 @@ class ConversionWithoutBoundsCheckConfig extends TaintTracking::Configuration {
7374
else sourceIsSigned = false
7475
) and
7576
(
76-
bitSize = ip.getTargetBitSize() or
77-
bitSize = ip.getTargetBitSizeInput().getNode(c).getIntValue()
77+
bitSize = ip.getTargetBitSize()
78+
or
79+
if
80+
exists(StrConv::IntSize intSize |
81+
ip.getTargetBitSizeInput().getNode(c).(DataFlow::ReadNode).reads(intSize)
82+
)
83+
then bitSize = 0
84+
else bitSize = ip.getTargetBitSizeInput().getNode(c).getIntValue()
7885
) and
7986
// `bitSize` could be any value between 0 and 64, but we can round
8087
// it up to the nearest size of an integer type without changing
@@ -129,7 +136,7 @@ class UpperBoundCheckGuard extends DataFlow::BarrierGuard, DataFlow::RelationalC
129136
exists(int strictnessOffset |
130137
if expr.isStrict() then strictnessOffset = 1 else strictnessOffset = 0
131138
|
132-
result = expr.getAnOperand().getIntValue() - strictnessOffset
139+
result = expr.getAnOperand().getExactValue().toFloat() - strictnessOffset
133140
)
134141
}
135142

‎ql/src/semmle/go/frameworks/Stdlib.qll

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -524,8 +524,8 @@ module IntegerParser {
524524

525525
/**
526526
* Gets the `FunctionInput` containing the maximum bit size of the
527-
* return value, if this makes sense, where 0 represents the bit
528-
* size of `int` and `uint`.
527+
* return value, if this makes sense. Note that if the value of the
528+
* input is 0 then it means the bit size of `int` and `uint`.
529529
*/
530530
FunctionInput getTargetBitSizeInput() { none() }
531531
}

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

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,12 @@ edges
5353
| IncorrectIntegerConversion.go:268:3:268:49 | ... := ...[0] : uint64 | IncorrectIntegerConversion.go:287:7:287:19 | type conversion |
5454
| IncorrectIntegerConversion.go:303:3:303:48 | ... := ...[0] : int64 | IncorrectIntegerConversion.go:307:7:307:18 | type conversion |
5555
| IncorrectIntegerConversion.go:313:2:313:47 | ... := ...[0] : int64 | IncorrectIntegerConversion.go:317:7:317:19 | type conversion |
56+
| IncorrectIntegerConversion.go:322:2:322:60 | ... := ...[0] : int64 | IncorrectIntegerConversion.go:326:6:326:17 | type conversion |
57+
| IncorrectIntegerConversion.go:322:2:322:60 | ... := ...[0] : int64 | IncorrectIntegerConversion.go:327:6:327:18 | type conversion |
58+
| IncorrectIntegerConversion.go:322:2:322:60 | ... := ...[0] : int64 | IncorrectIntegerConversion.go:328:6:328:18 | type conversion |
59+
| IncorrectIntegerConversion.go:322:2:322:60 | ... := ...[0] : int64 | IncorrectIntegerConversion.go:329:6:329:19 | type conversion |
60+
| IncorrectIntegerConversion.go:322:2:322:60 | ... := ...[0] : int64 | IncorrectIntegerConversion.go:330:6:330:18 | type conversion |
61+
| IncorrectIntegerConversion.go:322:2:322:60 | ... := ...[0] : int64 | IncorrectIntegerConversion.go:331:6:331:19 | type conversion |
5662
nodes
5763
| IncorrectIntegerConversion.go:26:2:26:28 | ... := ...[0] : int | semmle.label | ... := ...[0] : int |
5864
| IncorrectIntegerConversion.go:35:41:35:50 | type conversion | semmle.label | type conversion |
@@ -139,6 +145,15 @@ nodes
139145
| IncorrectIntegerConversion.go:307:7:307:18 | type conversion | semmle.label | type conversion |
140146
| IncorrectIntegerConversion.go:313:2:313:47 | ... := ...[0] : int64 | semmle.label | ... := ...[0] : int64 |
141147
| IncorrectIntegerConversion.go:317:7:317:19 | type conversion | semmle.label | type conversion |
148+
| IncorrectIntegerConversion.go:322:2:322:60 | ... := ...[0] : int64 | semmle.label | ... := ...[0] : int64 |
149+
| IncorrectIntegerConversion.go:322:2:322:60 | ... := ...[0] : int64 | semmle.label | ... := ...[0] : int64 |
150+
| IncorrectIntegerConversion.go:322:2:322:60 | ... := ...[0] : int64 | semmle.label | ... := ...[0] : int64 |
151+
| IncorrectIntegerConversion.go:326:6:326:17 | type conversion | semmle.label | type conversion |
152+
| IncorrectIntegerConversion.go:327:6:327:18 | type conversion | semmle.label | type conversion |
153+
| IncorrectIntegerConversion.go:328:6:328:18 | type conversion | semmle.label | type conversion |
154+
| IncorrectIntegerConversion.go:329:6:329:19 | type conversion | semmle.label | type conversion |
155+
| IncorrectIntegerConversion.go:330:6:330:18 | type conversion | semmle.label | type conversion |
156+
| IncorrectIntegerConversion.go:331:6:331:19 | type conversion | semmle.label | type conversion |
142157
#select
143158
| IncorrectIntegerConversion.go:26:2:26:28 | ... := ...[0] | IncorrectIntegerConversion.go:26:2:26:28 | ... := ...[0] : int | IncorrectIntegerConversion.go:35:41:35:50 | type conversion | Incorrect conversion of an integer with architecture-dependent bit size from strconv.Atoi to a lower bit size type int32 without an upper bound check. |
144159
| IncorrectIntegerConversion.go:65:3:65:49 | ... := ...[0] | IncorrectIntegerConversion.go:65:3:65:49 | ... := ...[0] : int64 | IncorrectIntegerConversion.go:69:7:69:18 | type conversion | Incorrect conversion of a 16-bit integer from strconv.ParseInt to a lower bit size type int8 without an upper bound check. |
@@ -194,3 +209,9 @@ nodes
194209
| IncorrectIntegerConversion.go:268:3:268:49 | ... := ...[0] | IncorrectIntegerConversion.go:268:3:268:49 | ... := ...[0] : uint64 | IncorrectIntegerConversion.go:287:7:287:19 | type conversion | Incorrect conversion of a 32-bit integer from strconv.ParseUint to a lower bit size type uint8 without an upper bound check. |
195210
| IncorrectIntegerConversion.go:303:3:303:48 | ... := ...[0] | IncorrectIntegerConversion.go:303:3:303:48 | ... := ...[0] : int64 | IncorrectIntegerConversion.go:307:7:307:18 | type conversion | Incorrect conversion of a 16-bit integer from strconv.ParseInt to a lower bit size type uint8 without an upper bound check. |
196211
| IncorrectIntegerConversion.go:313:2:313:47 | ... := ...[0] | IncorrectIntegerConversion.go:313:2:313:47 | ... := ...[0] : int64 | IncorrectIntegerConversion.go:317:7:317:19 | type conversion | Incorrect conversion of a 32-bit integer from strconv.ParseInt to a lower bit size type int16 without an upper bound check. |
212+
| IncorrectIntegerConversion.go:322:2:322:60 | ... := ...[0] | IncorrectIntegerConversion.go:322:2:322:60 | ... := ...[0] : int64 | IncorrectIntegerConversion.go:326:6:326:17 | type conversion | Incorrect conversion of an integer with architecture-dependent bit size from strconv.ParseInt to a lower bit size type int8 without an upper bound check. |
213+
| IncorrectIntegerConversion.go:322:2:322:60 | ... := ...[0] | IncorrectIntegerConversion.go:322:2:322:60 | ... := ...[0] : int64 | IncorrectIntegerConversion.go:327:6:327:18 | type conversion | Incorrect conversion of an integer with architecture-dependent bit size from strconv.ParseInt to a lower bit size type uint8 without an upper bound check. |
214+
| IncorrectIntegerConversion.go:322:2:322:60 | ... := ...[0] | IncorrectIntegerConversion.go:322:2:322:60 | ... := ...[0] : int64 | IncorrectIntegerConversion.go:328:6:328:18 | type conversion | Incorrect conversion of an integer with architecture-dependent bit size from strconv.ParseInt to a lower bit size type int16 without an upper bound check. |
215+
| IncorrectIntegerConversion.go:322:2:322:60 | ... := ...[0] | IncorrectIntegerConversion.go:322:2:322:60 | ... := ...[0] : int64 | IncorrectIntegerConversion.go:329:6:329:19 | type conversion | Incorrect conversion of an integer with architecture-dependent bit size from strconv.ParseInt to a lower bit size type uint16 without an upper bound check. |
216+
| IncorrectIntegerConversion.go:322:2:322:60 | ... := ...[0] | IncorrectIntegerConversion.go:322:2:322:60 | ... := ...[0] : int64 | IncorrectIntegerConversion.go:330:6:330:18 | type conversion | Incorrect conversion of an integer with architecture-dependent bit size from strconv.ParseInt to a lower bit size type int32 without an upper bound check. |
217+
| IncorrectIntegerConversion.go:322:2:322:60 | ... := ...[0] | IncorrectIntegerConversion.go:322:2:322:60 | ... := ...[0] : int64 | IncorrectIntegerConversion.go:331:6:331:19 | type conversion | Incorrect conversion of an integer with architecture-dependent bit size from strconv.ParseInt to a lower bit size type uint32 without an upper bound check. |

‎ql/test/query-tests/Security/CWE-681/IncorrectIntegerConversion.go

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -317,3 +317,20 @@ func testPathWithMoreThanOneSink(input string) {
317317
v := int16(parsed) // NOT OK
318318
_ = int8(v) // OK
319319
}
320+
321+
func testUsingStrConvIntSize(input string) {
322+
parsed, err := strconv.ParseInt(input, 10, strconv.IntSize)
323+
if err != nil {
324+
panic(err)
325+
}
326+
_ = int8(parsed) // NOT OK
327+
_ = uint8(parsed) // NOT OK
328+
_ = int16(parsed) // NOT OK
329+
_ = uint16(parsed) // NOT OK
330+
_ = int32(parsed) // NOT OK
331+
_ = uint32(parsed) // NOT OK
332+
_ = int64(parsed) // OK
333+
_ = uint64(parsed) // OK
334+
_ = int(parsed) // OK
335+
_ = uint(parsed) // OK
336+
}

0 commit comments

Comments
 (0)