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

Commit 4907f65

Browse files
owen-mcowen-mc
committed
Address review comments 4
1 parent ed469a3 commit 4907f65

File tree

3 files changed

+60
-33
lines changed

3 files changed

+60
-33
lines changed

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

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,15 +32,24 @@ float getMaxIntValue(int bitSize, boolean isSigned) {
3232
* Holds if converting from an integer types with size `sourceBitSize` to
3333
* one with size `sinkBitSize` can produce unexpected values, where 0 means
3434
* architecture-dependent.
35+
*
36+
* Architecture-dependent bit sizes can be 32 or 64. To catch flows that
37+
* only manifest on 64-bit architectures we consider an
38+
* architecture-dependent source bit size to be 64. To catch flows that
39+
* only happen on 32-bit architectures we consider an
40+
* architecture-dependent sink bit size to be 32. We exclude the case where
41+
* both source and sink have architecture-dependent bit sizes.
3542
*/
3643
private predicate isIncorrectIntegerConversion(int sourceBitSize, int sinkBitSize) {
3744
sourceBitSize in [16, 32, 64] and
3845
sinkBitSize in [8, 16, 32] and
3946
sourceBitSize > sinkBitSize
4047
or
48+
// Treat `sourceBitSize = 0` like `sourceBitSize = 64`, and exclude `sinkBitSize = 0`
4149
sourceBitSize = 0 and
4250
sinkBitSize in [8, 16, 32]
4351
or
52+
// Treat `sinkBitSize = 0` like `sinkBitSize = 32`, and exclude `sourceBitSize = 0`
4453
sourceBitSize = 64 and
4554
sinkBitSize = 0
4655
}
@@ -76,6 +85,8 @@ class ConversionWithoutBoundsCheckConfig extends TaintTracking::Configuration {
7685
(
7786
bitSize = ip.getTargetBitSize()
7887
or
88+
// If we are reading a variable, check if it is
89+
// `strconv.IntSize`, and use 0 if it is.
7990
if
8091
exists(StrConv::IntSize intSize |
8192
ip.getTargetBitSizeInput().getNode(c).(DataFlow::ReadNode).reads(intSize)
@@ -105,13 +116,17 @@ class ConversionWithoutBoundsCheckConfig extends TaintTracking::Configuration {
105116
) and
106117
not exists(ShrExpr shrExpr |
107118
shrExpr.getLeftOperand().getGlobalValueNumber() =
119+
sink.getOperand().asExpr().getGlobalValueNumber() or
120+
shrExpr.getLeftOperand().(AndExpr).getAnOperand().getGlobalValueNumber() =
108121
sink.getOperand().asExpr().getGlobalValueNumber()
109122
)
110123
}
111124

112125
override predicate isSink(DataFlow::Node sink) { isSink(sink, sinkBitSize) }
113126

114127
override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
128+
// To catch flows that only happen on 32-bit architectures we
129+
// consider an architecture-dependent sink bit size to be 32.
115130
exists(int bitSize | if sinkBitSize != 0 then bitSize = sinkBitSize else bitSize = 32 |
116131
guard.(UpperBoundCheckGuard).getBound() <= getMaxIntValue(bitSize, sourceIsSigned)
117132
)

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

Lines changed: 29 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -50,15 +50,14 @@ edges
5050
| IncorrectIntegerConversion.go:235:3:235:48 | ... := ...[0] : int64 | IncorrectIntegerConversion.go:241:7:241:23 | type conversion |
5151
| IncorrectIntegerConversion.go:247:3:247:36 | ... := ...[0] : int | IncorrectIntegerConversion.go:261:8:261:19 | type conversion |
5252
| IncorrectIntegerConversion.go:268:3:268:49 | ... := ...[0] : uint64 | IncorrectIntegerConversion.go:282:8:282:21 | type conversion |
53-
| IncorrectIntegerConversion.go:268:3:268:49 | ... := ...[0] : uint64 | IncorrectIntegerConversion.go:287:7:287:19 | type conversion |
54-
| IncorrectIntegerConversion.go:303:3:303:48 | ... := ...[0] : int64 | IncorrectIntegerConversion.go:307:7:307:18 | type conversion |
55-
| 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 |
53+
| IncorrectIntegerConversion.go:319:3:319:48 | ... := ...[0] : int64 | IncorrectIntegerConversion.go:323:7:323:18 | type conversion |
54+
| IncorrectIntegerConversion.go:329:2:329:47 | ... := ...[0] : int64 | IncorrectIntegerConversion.go:333:7:333:19 | type conversion |
55+
| IncorrectIntegerConversion.go:338:2:338:60 | ... := ...[0] : int64 | IncorrectIntegerConversion.go:342:6:342:17 | type conversion |
56+
| IncorrectIntegerConversion.go:338:2:338:60 | ... := ...[0] : int64 | IncorrectIntegerConversion.go:343:6:343:18 | type conversion |
57+
| IncorrectIntegerConversion.go:338:2:338:60 | ... := ...[0] : int64 | IncorrectIntegerConversion.go:344:6:344:18 | type conversion |
58+
| IncorrectIntegerConversion.go:338:2:338:60 | ... := ...[0] : int64 | IncorrectIntegerConversion.go:345:6:345:19 | type conversion |
59+
| IncorrectIntegerConversion.go:338:2:338:60 | ... := ...[0] : int64 | IncorrectIntegerConversion.go:346:6:346:18 | type conversion |
60+
| IncorrectIntegerConversion.go:338:2:338:60 | ... := ...[0] : int64 | IncorrectIntegerConversion.go:347:6:347:19 | type conversion |
6261
nodes
6362
| IncorrectIntegerConversion.go:26:2:26:28 | ... := ...[0] : int | semmle.label | ... := ...[0] : int |
6463
| IncorrectIntegerConversion.go:35:41:35:50 | type conversion | semmle.label | type conversion |
@@ -138,22 +137,20 @@ nodes
138137
| IncorrectIntegerConversion.go:247:3:247:36 | ... := ...[0] : int | semmle.label | ... := ...[0] : int |
139138
| IncorrectIntegerConversion.go:261:8:261:19 | type conversion | semmle.label | type conversion |
140139
| IncorrectIntegerConversion.go:268:3:268:49 | ... := ...[0] : uint64 | semmle.label | ... := ...[0] : uint64 |
141-
| IncorrectIntegerConversion.go:268:3:268:49 | ... := ...[0] : uint64 | semmle.label | ... := ...[0] : uint64 |
142140
| IncorrectIntegerConversion.go:282:8:282:21 | type conversion | semmle.label | type conversion |
143-
| IncorrectIntegerConversion.go:287:7:287:19 | type conversion | semmle.label | type conversion |
144-
| IncorrectIntegerConversion.go:303:3:303:48 | ... := ...[0] : int64 | semmle.label | ... := ...[0] : int64 |
145-
| IncorrectIntegerConversion.go:307:7:307:18 | type conversion | semmle.label | type conversion |
146-
| IncorrectIntegerConversion.go:313:2:313:47 | ... := ...[0] : int64 | semmle.label | ... := ...[0] : int64 |
147-
| 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 |
141+
| IncorrectIntegerConversion.go:319:3:319:48 | ... := ...[0] : int64 | semmle.label | ... := ...[0] : int64 |
142+
| IncorrectIntegerConversion.go:323:7:323:18 | type conversion | semmle.label | type conversion |
143+
| IncorrectIntegerConversion.go:329:2:329:47 | ... := ...[0] : int64 | semmle.label | ... := ...[0] : int64 |
144+
| IncorrectIntegerConversion.go:333:7:333:19 | type conversion | semmle.label | type conversion |
145+
| IncorrectIntegerConversion.go:338:2:338:60 | ... := ...[0] : int64 | semmle.label | ... := ...[0] : int64 |
146+
| IncorrectIntegerConversion.go:338:2:338:60 | ... := ...[0] : int64 | semmle.label | ... := ...[0] : int64 |
147+
| IncorrectIntegerConversion.go:338:2:338:60 | ... := ...[0] : int64 | semmle.label | ... := ...[0] : int64 |
148+
| IncorrectIntegerConversion.go:342:6:342:17 | type conversion | semmle.label | type conversion |
149+
| IncorrectIntegerConversion.go:343:6:343:18 | type conversion | semmle.label | type conversion |
150+
| IncorrectIntegerConversion.go:344:6:344:18 | type conversion | semmle.label | type conversion |
151+
| IncorrectIntegerConversion.go:345:6:345:19 | type conversion | semmle.label | type conversion |
152+
| IncorrectIntegerConversion.go:346:6:346:18 | type conversion | semmle.label | type conversion |
153+
| IncorrectIntegerConversion.go:347:6:347:19 | type conversion | semmle.label | type conversion |
157154
#select
158155
| 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. |
159156
| 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. |
@@ -206,12 +203,11 @@ nodes
206203
| IncorrectIntegerConversion.go:235:3:235:48 | ... := ...[0] | IncorrectIntegerConversion.go:235:3:235:48 | ... := ...[0] : int64 | IncorrectIntegerConversion.go:241:7:241:23 | type conversion | Incorrect conversion of a 32-bit integer from strconv.ParseInt to a lower bit size type int16 without an upper bound check. |
207204
| IncorrectIntegerConversion.go:247:3:247:36 | ... := ...[0] | IncorrectIntegerConversion.go:247:3:247:36 | ... := ...[0] : int | IncorrectIntegerConversion.go:261:8:261:19 | type conversion | Incorrect conversion of an integer with architecture-dependent bit size from strconv.Atoi to a lower bit size type int8 without an upper bound check. |
208205
| IncorrectIntegerConversion.go:268:3:268:49 | ... := ...[0] | IncorrectIntegerConversion.go:268:3:268:49 | ... := ...[0] : uint64 | IncorrectIntegerConversion.go:282:8:282:21 | type conversion | Incorrect conversion of a 32-bit integer from strconv.ParseUint to a lower bit size type uint16 without an upper bound check. |
209-
| 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. |
210-
| 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. |
211-
| 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. |
206+
| IncorrectIntegerConversion.go:319:3:319:48 | ... := ...[0] | IncorrectIntegerConversion.go:319:3:319:48 | ... := ...[0] : int64 | IncorrectIntegerConversion.go:323:7:323: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. |
207+
| IncorrectIntegerConversion.go:329:2:329:47 | ... := ...[0] | IncorrectIntegerConversion.go:329:2:329:47 | ... := ...[0] : int64 | IncorrectIntegerConversion.go:333:7:333: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. |
208+
| IncorrectIntegerConversion.go:338:2:338:60 | ... := ...[0] | IncorrectIntegerConversion.go:338:2:338:60 | ... := ...[0] : int64 | IncorrectIntegerConversion.go:342:6:342: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. |
209+
| IncorrectIntegerConversion.go:338:2:338:60 | ... := ...[0] | IncorrectIntegerConversion.go:338:2:338:60 | ... := ...[0] : int64 | IncorrectIntegerConversion.go:343:6:343: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. |
210+
| IncorrectIntegerConversion.go:338:2:338:60 | ... := ...[0] | IncorrectIntegerConversion.go:338:2:338:60 | ... := ...[0] : int64 | IncorrectIntegerConversion.go:344:6:344: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. |
211+
| IncorrectIntegerConversion.go:338:2:338:60 | ... := ...[0] | IncorrectIntegerConversion.go:338:2:338:60 | ... := ...[0] : int64 | IncorrectIntegerConversion.go:345:6:345: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. |
212+
| IncorrectIntegerConversion.go:338:2:338:60 | ... := ...[0] | IncorrectIntegerConversion.go:338:2:338:60 | ... := ...[0] : int64 | IncorrectIntegerConversion.go:346:6:346: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. |
213+
| IncorrectIntegerConversion.go:338:2:338:60 | ... := ...[0] | IncorrectIntegerConversion.go:338:2:338:60 | ... := ...[0] : int64 | IncorrectIntegerConversion.go:347:6:347: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: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -299,6 +299,22 @@ func testRightShifted(input string) {
299299
_ = byte(parsed >> 16)
300300
_ = byte(parsed >> 24)
301301
}
302+
{
303+
parsed, err := strconv.ParseInt(input, 10, 16)
304+
if err != nil {
305+
panic(err)
306+
}
307+
_ = byte(parsed) // OK
308+
_ = byte(parsed & 0xff00 >> 8)
309+
}
310+
{
311+
parsed, err := strconv.ParseInt(input, 10, 32)
312+
if err != nil {
313+
panic(err)
314+
}
315+
_ = byte(parsed) // OK
316+
_ = byte(parsed >> 8 & 0xff)
317+
}
302318
{
303319
parsed, err := strconv.ParseInt(input, 10, 16)
304320
if err != nil {

0 commit comments

Comments
 (0)