Skip to content

Commit 33ef634

Browse files
authored
Merge pull request #6679 from andersfugmann/relax_memberMayBeVarSize
Improve precision on OverflowStatic query.
2 parents c4714b5 + e49cd83 commit 33ef634

File tree

6 files changed

+81
-21
lines changed

6 files changed

+81
-21
lines changed
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
lgtm,codescanning
2+
* The `memberMayBeVarSize` predicate considers more fields to be variable size.
3+
As a result, the "Static buffer overflow" query (cpp/static-buffer-overflow)
4+
produces fewer false positives.

cpp/ql/lib/semmle/code/cpp/commons/Buffer.qll

Lines changed: 28 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -2,17 +2,18 @@ import cpp
22
import semmle.code.cpp.dataflow.DataFlow
33

44
/**
5-
* Holds if `v` is a member variable of `c` that looks like it might be variable sized in practice. For
6-
* example:
5+
* Holds if `v` is a member variable of `c` that looks like it might be variable sized
6+
* in practice. For example:
77
* ```
88
* struct myStruct { // c
99
* int amount;
1010
* char data[1]; // v
1111
* };
1212
* ```
13-
* This requires that `v` is an array of size 0 or 1, and `v` is the last member of `c`. In addition,
14-
* there must be at least one instance where a `c` pointer is allocated with additional space. For
15-
* example, holds for `c` if it occurs as
13+
* This requires that `v` is an array of size 0 or 1, and `v` is the last member of `c`.
14+
* In addition, if the size of the structure is taken, there must be at least one instance
15+
* where a `c` pointer is allocated with additional space.
16+
* For example, holds for `c` if it occurs as
1617
* ```
1718
* malloc(sizeof(c) + 100 * sizeof(char))
1819
* ```
@@ -27,27 +28,25 @@ predicate memberMayBeVarSize(Class c, MemberVariable v) {
2728
i = max(int j | c.getCanonicalMember(j) instanceof Field | j) and
2829
v = c.getCanonicalMember(i) and
2930
// v is an array of size at most 1
30-
v.getUnspecifiedType().(ArrayType).getArraySize() <= 1
31+
v.getUnspecifiedType().(ArrayType).getArraySize() <= 1 and
32+
not c instanceof Union
3133
) and
34+
// If the size is taken, then arithmetic is performed on the result at least once
3235
(
36+
// `sizeof(c)` is not taken
37+
not exists(SizeofOperator so |
38+
so.(SizeofTypeOperator).getTypeOperand().getUnspecifiedType() = c or
39+
so.(SizeofExprOperator).getExprOperand().getUnspecifiedType() = c
40+
)
41+
or
42+
// or `sizeof(c)` is taken
3343
exists(SizeofOperator so |
34-
// `sizeof(c)` is taken
3544
so.(SizeofTypeOperator).getTypeOperand().getUnspecifiedType() = c or
3645
so.(SizeofExprOperator).getExprOperand().getUnspecifiedType() = c
3746
|
38-
// arithmetic is performed on the result
47+
// and arithmetic is performed on the result
3948
so.getParent*() instanceof AddExpr
4049
)
41-
or
42-
exists(AddressOfExpr aoe |
43-
// `&(c.v)` is taken
44-
aoe.getAddressable() = v
45-
)
46-
or
47-
exists(BuiltInOperationBuiltInOffsetOf oo |
48-
// `offsetof(c, v)` using a builtin
49-
oo.getAChild().(VariableAccess).getTarget() = v
50-
)
5150
)
5251
}
5352

@@ -61,6 +60,10 @@ int getBufferSize(Expr bufferExpr, Element why) {
6160
result = bufferVar.getUnspecifiedType().(ArrayType).getSize() and
6261
why = bufferVar and
6362
not memberMayBeVarSize(_, bufferVar) and
63+
not exists(Union bufferType |
64+
bufferType.getAMemberVariable() = why and
65+
bufferVar.getUnspecifiedType().(ArrayType).getSize() <= 1
66+
) and
6467
not result = 0 // zero sized arrays are likely to have special usage, for example
6568
or
6669
// behaving a bit like a 'union' overlapping other fields.
@@ -82,6 +85,13 @@ int getBufferSize(Expr bufferExpr, Element why) {
8285
parentPtr.getTarget().getUnspecifiedType().(PointerType).getBaseType() = parentClass and
8386
result = getBufferSize(parentPtr, _) + bufferVar.getType().getSize() - parentClass.getSize()
8487
)
88+
or
89+
exists(Union bufferType |
90+
bufferType.getAMemberVariable() = why and
91+
why = bufferVar and
92+
bufferVar.getUnspecifiedType().(ArrayType).getSize() <= 1 and
93+
result = bufferType.getSize()
94+
)
8595
)
8696
or
8797
// buffer is a fixed size dynamic allocation

cpp/ql/test/query-tests/Critical/OverflowStatic/OverflowStatic.expected

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,10 @@
99
| test.c:15:9:15:13 | access to array | Potential buffer-overflow: 'xs' has size 5 but 'xs[6]' is accessed here. |
1010
| test.c:20:9:20:18 | access to array | Potential buffer-overflow: 'ys' has size 5 but 'ys[5]' is accessed here. |
1111
| test.c:21:9:21:18 | access to array | Potential buffer-overflow: 'ys' has size 5 but 'ys[6]' is accessed here. |
12+
| test.c:47:3:47:18 | access to array | Potential buffer-overflow: 'ptr' has size 8 but 'ptr[8]' is accessed here. |
13+
| test.c:54:3:54:26 | access to array | Potential buffer-overflow: 'ptr' has size 8 but 'ptr[8]' is accessed here. |
14+
| test.c:61:3:61:18 | access to array | Potential buffer-overflow: 'ptr' has size 8 but 'ptr[8]' is accessed here. |
15+
| test.c:72:3:72:11 | access to array | Potential buffer-overflow: 'buf' has size 1 but 'buf[1]' is accessed here. |
1216
| test.cpp:19:3:19:12 | access to array | Potential buffer-overflow: counter 'i' <= 3 but 'buffer1' has 3 elements. |
1317
| test.cpp:20:3:20:12 | access to array | Potential buffer-overflow: counter 'i' <= 3 but 'buffer2' has 3 elements. |
1418
| test.cpp:24:27:24:27 | 4 | Potential buffer-overflow: 'buffer1' has size 3 not 4. |

cpp/ql/test/query-tests/Critical/OverflowStatic/test.c

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,3 +27,47 @@ void f(void) {
2727
c = stru.zs[6]; // GOOD (zs is variable size)
2828
}
2929

30+
void* malloc(long unsigned int);
31+
void test_buffer_sentinal() {
32+
struct { char len; char buf[1]; } *b = malloc(10); // len(buf.buffer) effectively 8
33+
b->buf[0] = 0; // GOOD
34+
b->buf[7] = 0; // GOOD
35+
b->buf[8] = 0; // BAD [NOT DETECTED]
36+
}
37+
38+
union u {
39+
unsigned long value;
40+
char ptr[1];
41+
};
42+
43+
void union_test() {
44+
union u u;
45+
u.ptr[0] = 0; // GOOD
46+
u.ptr[sizeof(u)-1] = 0; // GOOD
47+
u.ptr[sizeof(u)] = 0; // BAD
48+
}
49+
50+
void test_struct_union() {
51+
struct { union u u; } v;
52+
v.u.ptr[0] = 0; // GOOD
53+
v.u.ptr[sizeof(union u)-1] = 0; // GOOD
54+
v.u.ptr[sizeof(union u)] = 0; // BAD
55+
}
56+
57+
void union_test2() {
58+
union { char ptr[1]; unsigned long value; } u;
59+
u.ptr[0] = 0; // GOOD
60+
u.ptr[sizeof(u)-1] = 0; // GOOD
61+
u.ptr[sizeof(u)] = 0; // BAD
62+
}
63+
64+
typedef struct {
65+
char len;
66+
char buf[1];
67+
} var_buf;
68+
69+
void test_alloc() {
70+
// Special case of taking sizeof without any addition or multiplications
71+
var_buf *b = malloc(sizeof(var_buf));
72+
b->buf[1] = 0; // BAD
73+
}

cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/OverflowBuffer.expected

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,4 +80,4 @@
8080
| var_size_struct.cpp:99:3:99:8 | call to memset | This 'memset' operation accesses 129 bytes but the $@ is only 128 bytes. | var_size_struct.cpp:92:8:92:10 | str | destination buffer |
8181
| var_size_struct.cpp:101:3:101:8 | call to memset | This 'memset' operation accesses 129 bytes but the $@ is only 128 bytes. | var_size_struct.cpp:92:8:92:10 | str | destination buffer |
8282
| var_size_struct.cpp:103:3:103:9 | call to strncpy | This 'strncpy' operation may access 129 bytes but the $@ is only 128 bytes. | var_size_struct.cpp:92:8:92:10 | str | destination buffer |
83-
| var_size_struct.cpp:171:3:171:8 | call to memset | This 'memset' operation accesses 100 bytes but the $@ is only 1 byte. | var_size_struct.cpp:125:17:125:19 | arr | destination buffer |
83+
| var_size_struct.cpp:169:3:169:8 | call to memset | This 'memset' operation accesses 100 bytes but the $@ is only 1 byte. | var_size_struct.cpp:125:17:125:19 | arr | destination buffer |

cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/var_size_struct.cpp

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -161,8 +161,6 @@ void useVarStruct34(varStruct5 *vs5) {
161161
varStruct5 *vs5b = (varStruct5 *)malloc(sizeof(*vs5));
162162
varStruct6 *vs6 = (varStruct6 *)malloc(offsetof(varStruct6, arr) + 9); // establish varStruct6 as variable size
163163
varStruct7 *vs7 = (varStruct7 *)malloc(sizeForVarStruct7(9)); // establish varStruct7 as variable size
164-
varStruct8 *vs8a = (varStruct8 *)malloc(sizeof(varStruct8) + 9); // establish varStruct8 as variable size
165-
varStruct8 *vs8b = (varStruct8 *)malloc(sizeof(varStruct8));
166164
varStruct9 *vs9 = (varStruct9 *)malloc(__builtin_offsetof(varStruct9, arr) + 9); // establish varStruct9 as variable size
167165
}
168166

0 commit comments

Comments
 (0)