Skip to content

Commit 2b7e599

Browse files
geoffw0geoffw0
authored
Merge pull request github#5703 from MathiasVP/improve-access-of-memory-location-after-end-of-buffer-using-strncat
C++: Improve cpp/access-memory-location-after-end-buffer-strncat
2 parents cb524b6 + 95742ae commit 2b7e599

File tree

4 files changed

+92
-98
lines changed

4 files changed

+92
-98
lines changed

‎cpp/ql/src/experimental/Security/CWE/CWE-788/AccessOfMemoryLocationAfterEndOfBufferUsingStrncat.ql

Lines changed: 22 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -11,54 +11,32 @@
1111
*/
1212

1313
import cpp
14+
import semmle.code.cpp.models.implementations.Strcat
1415
import semmle.code.cpp.valuenumbering.GlobalValueNumbering
1516

1617
/**
17-
* A call to `strncat` of the form `strncat(buff, str, someExpr - strlen(buf))`, for some expression `someExpr` equal to `sizeof(buff)`.
18+
* Holds if `call` is a call to `strncat` such that `sizeArg` and `destArg` are the size and
19+
* destination arguments, respectively.
1820
*/
19-
class WrongCallStrncat extends FunctionCall {
20-
Expr leftsomeExpr;
21-
22-
WrongCallStrncat() {
23-
this.getTarget().hasGlobalOrStdName("strncat") and
24-
// the expression of the first argument in `strncat` and `strnlen` is identical
25-
globalValueNumber(this.getArgument(0)) =
26-
globalValueNumber(this.getArgument(2).(SubExpr).getRightOperand().(StrlenCall).getStringExpr()) and
27-
// using a string constant often speaks of manually calculating the length of the required buffer.
28-
(
29-
not this.getArgument(1) instanceof StringLiteral and
30-
not this.getArgument(1) instanceof CharLiteral
31-
) and
32-
// for use in predicates
33-
leftsomeExpr = this.getArgument(2).(SubExpr).getLeftOperand()
34-
}
35-
36-
/**
37-
* Holds if the left side of the expression `someExpr` equal to `sizeof(buf)`.
38-
*/
39-
predicate isExpressionEqualSizeof() {
40-
// the left side of the expression `someExpr` is `sizeof(buf)`.
41-
globalValueNumber(this.getArgument(0)) =
42-
globalValueNumber(leftsomeExpr.(SizeofExprOperator).getExprOperand())
43-
or
44-
// value of the left side of the expression `someExpr` equal `sizeof(buf)` value, and `buf` is array.
45-
leftsomeExpr.getValue().toInt() = this.getArgument(0).getType().getSize()
46-
}
47-
48-
/**
49-
* Holds if the left side of the expression `someExpr` equal to variable containing the length of the memory allocated for the buffer.
50-
*/
51-
predicate isVariableEqualValueSizegBuffer() {
52-
// the left side of expression `someExpr` is the variable that was used in the function of allocating memory for the buffer`.
53-
exists(AllocationExpr alc |
54-
leftsomeExpr.(VariableAccess).getTarget() =
55-
alc.(FunctionCall).getArgument(0).(VariableAccess).getTarget()
56-
)
57-
}
21+
predicate interestringCallWithArgs(Call call, Expr sizeArg, Expr destArg) {
22+
exists(StrcatFunction strcat |
23+
strcat = call.getTarget() and
24+
sizeArg = call.getArgument(strcat.getParamSize()) and
25+
destArg = call.getArgument(strcat.getParamDest())
26+
)
5827
}
5928

60-
from WrongCallStrncat sc
29+
from FunctionCall call, Expr sizeArg, Expr destArg, SubExpr sub, int n
6130
where
62-
sc.isExpressionEqualSizeof() or
63-
sc.isVariableEqualValueSizegBuffer()
64-
select sc, "if the used buffer is full, writing out of the buffer is possible"
31+
interestringCallWithArgs(call, sizeArg, destArg) and
32+
// The destination buffer is an array of size n
33+
destArg.getUnspecifiedType().(ArrayType).getSize() = n and
34+
// The size argument is equivalent to a subtraction
35+
globalValueNumber(sizeArg).getAnExpr() = sub and
36+
// ... where the left side of the subtraction is the constant n
37+
globalValueNumber(sub.getLeftOperand()).getAnExpr().getValue().toInt() = n and
38+
// ... and the right side of the subtraction is a call to `strlen` where the argument is the
39+
// destination buffer.
40+
globalValueNumber(sub.getRightOperand()).getAnExpr().(StrlenCall).getStringExpr() =
41+
globalValueNumber(destArg).getAnExpr()
42+
select call, "Possible out-of-bounds write due to incorrect size argument."

‎cpp/ql/test/experimental/query-tests/Security/CWE/CWE-788/semmle/tests/AccessOfMemoryLocationAfterEndOfBufferUsingStrlen.expected

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
1-
| test.c:42:3:42:24 | ... = ... | potential unsafe or redundant assignment. |
2-
| test.c:43:3:43:40 | ... = ... | potential unsafe or redundant assignment. |
3-
| test.c:44:3:44:40 | ... = ... | potential unsafe or redundant assignment. |
4-
| test.c:45:3:45:44 | ... = ... | potential unsafe or redundant assignment. |
5-
| test.c:46:3:46:44 | ... = ... | potential unsafe or redundant assignment. |
6-
| test.c:47:3:47:48 | ... = ... | potential unsafe or redundant assignment. |
7-
| test.c:48:3:48:48 | ... = ... | potential unsafe or redundant assignment. |
8-
| test.c:49:3:49:50 | ... = ... | potential unsafe or redundant assignment. |
9-
| test.c:50:3:50:50 | ... = ... | potential unsafe or redundant assignment. |
1+
| test.c:54:3:54:24 | ... = ... | potential unsafe or redundant assignment. |
2+
| test.c:55:3:55:40 | ... = ... | potential unsafe or redundant assignment. |
3+
| test.c:56:3:56:44 | ... = ... | potential unsafe or redundant assignment. |
4+
| test.c:57:3:57:44 | ... = ... | potential unsafe or redundant assignment. |
5+
| test.c:58:3:58:48 | ... = ... | potential unsafe or redundant assignment. |
6+
| test.c:59:3:59:48 | ... = ... | potential unsafe or redundant assignment. |
7+
| test.c:60:3:60:52 | ... = ... | potential unsafe or redundant assignment. |
8+
| test.c:61:3:61:50 | ... = ... | potential unsafe or redundant assignment. |
9+
| test.c:62:3:62:54 | ... = ... | potential unsafe or redundant assignment. |

‎cpp/ql/test/experimental/query-tests/Security/CWE/CWE-788/semmle/tests/AccessOfMemoryLocationAfterEndOfBufferUsingStrncat.expected

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1-
| test.c:4:3:4:9 | call to strncat | if the used buffer is full, writing out of the buffer is possible |
2-
| test.c:11:3:11:9 | call to strncat | if the used buffer is full, writing out of the buffer is possible |
3-
| test.c:19:3:19:9 | call to strncat | if the used buffer is full, writing out of the buffer is possible |
1+
| test.c:8:3:8:9 | call to strncat | Possible out-of-bounds write due to incorrect size argument. |
2+
| test.c:9:3:9:9 | call to strncat | Possible out-of-bounds write due to incorrect size argument. |
3+
| test.c:17:3:17:9 | call to strncat | Possible out-of-bounds write due to incorrect size argument. |
4+
| test.c:18:3:18:9 | call to strncat | Possible out-of-bounds write due to incorrect size argument. |
5+
| test.c:46:3:46:9 | call to strncat | Possible out-of-bounds write due to incorrect size argument. |

‎cpp/ql/test/experimental/query-tests/Security/CWE/CWE-788/semmle/tests/test.c

Lines changed: 56 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -1,70 +1,84 @@
1-
void workFunction_0(char *s) {
1+
char * strncat(char*, const char*, unsigned);
2+
unsigned strlen(const char*);
3+
void* malloc(unsigned);
4+
5+
void strncat_test1(char *s) {
26
char buf[80];
3-
strncat(buf, s, sizeof(buf)-strlen(buf)-1); // GOOD
4-
strncat(buf, s, sizeof(buf)-strlen(buf)); // BAD
5-
strncat(buf, "fix", sizeof(buf)-strlen(buf)); // BAD [NOT DETECTED]
7+
strncat(buf, s, sizeof(buf) - strlen(buf) - 1); // GOOD
8+
strncat(buf, s, sizeof(buf) - strlen(buf)); // BAD
9+
strncat(buf, "fix", sizeof(buf)-strlen(buf)); // BAD
610
}
7-
void workFunction_1(char *s) {
11+
812
#define MAX_SIZE 80
13+
14+
void strncat_test2(char *s) {
915
char buf[MAX_SIZE];
10-
strncat(buf, s, MAX_SIZE-strlen(buf)-1); // GOOD
11-
strncat(buf, s, MAX_SIZE-strlen(buf)); // BAD
12-
strncat(buf, "fix", MAX_SIZE-strlen(buf)); // BAD [NOT DETECTED]
16+
strncat(buf, s, MAX_SIZE - strlen(buf) - 1); // GOOD
17+
strncat(buf, s, MAX_SIZE - strlen(buf)); // BAD
18+
strncat(buf, "fix", MAX_SIZE - strlen(buf)); // BAD
1319
}
14-
void workFunction_2_0(char *s) {
15-
char * buf;
16-
int len=80;
17-
buf = (char *) malloc(len);
18-
strncat(buf, s, len-strlen(buf)-1); // GOOD
19-
strncat(buf, s, len-strlen(buf)); // BAD
20-
strncat(buf, "fix", len-strlen(buf)); // BAD [NOT DETECTED]
20+
21+
void strncat_test3(char *s) {
22+
int len = 80;
23+
char* buf = (char *) malloc(len);
24+
strncat(buf, s, len - strlen(buf) - 1); // GOOD
25+
strncat(buf, s, len - strlen(buf)); // BAD [NOT DETECTED]
26+
strncat(buf, "fix", len - strlen(buf)); // BAD [NOT DETECTED]
2127
}
22-
void workFunction_2_1(char *s) {
23-
char * buf;
24-
int len=80;
25-
buf = (char *) malloc(len+1);
26-
strncat(buf, s, len-strlen(buf)-1); // GOOD
27-
strncat(buf, s, len-strlen(buf)); // GOOD
28+
29+
void strncat_test4(char *s) {
30+
int len = 80;
31+
char* buf = (char *) malloc(len + 1);
32+
strncat(buf, s, len - strlen(buf) - 1); // GOOD
33+
strncat(buf, s, len - strlen(buf)); // GOOD
2834
}
2935

3036
struct buffers
3137
{
32-
unsigned char buff1[50];
33-
unsigned char *buff2;
38+
unsigned char array[50];
39+
unsigned char *pointer;
3440
} globalBuff1,*globalBuff2,globalBuff1_c,*globalBuff2_c;
3541

42+
void strncat_test5(char* s, struct buffers* buffers) {
43+
unsigned len_array = strlen(buffers->array);
44+
unsigned max_size = sizeof(buffers->array);
45+
unsigned free_size = max_size - len_array;
46+
strncat(buffers->array, s, free_size); // BAD
47+
}
3648

37-
void badFunc0(){
49+
void strlen_test1(){
3850
unsigned char buff1[12];
3951
struct buffers buffAll;
4052
struct buffers * buffAll1;
4153

4254
buff1[strlen(buff1)]=0; // BAD
43-
buffAll.buff1[strlen(buffAll.buff1)]=0; // BAD
44-
buffAll.buff2[strlen(buffAll.buff2)]=0; // BAD
45-
buffAll1->buff1[strlen(buffAll1->buff1)]=0; // BAD
46-
buffAll1->buff2[strlen(buffAll1->buff2)]=0; // BAD
47-
globalBuff1.buff1[strlen(globalBuff1.buff1)]=0; // BAD
48-
globalBuff1.buff2[strlen(globalBuff1.buff2)]=0; // BAD
49-
globalBuff2->buff1[strlen(globalBuff2->buff1)]=0; // BAD
50-
globalBuff2->buff2[strlen(globalBuff2->buff2)]=0; // BAD
55+
buffAll.array[strlen(buffAll.array)]=0; // BAD
56+
buffAll.pointer[strlen(buffAll.pointer)]=0; // BAD
57+
buffAll1->array[strlen(buffAll1->array)]=0; // BAD
58+
buffAll1->pointer[strlen(buffAll1->pointer)]=0; // BAD
59+
globalBuff1.array[strlen(globalBuff1.array)]=0; // BAD
60+
globalBuff1.pointer[strlen(globalBuff1.pointer)]=0; // BAD
61+
globalBuff2->array[strlen(globalBuff2->array)]=0; // BAD
62+
globalBuff2->pointer[strlen(globalBuff2->pointer)]=0; // BAD
5163
}
52-
void noBadFunc0(){
64+
65+
void strlen_test2(){
5366
unsigned char buff1[12],buff1_c[12];
5467
struct buffers buffAll,buffAll_c;
5568
struct buffers * buffAll1,*buffAll1_c;
5669

5770
buff1[strlen(buff1_c)]=0; // GOOD
58-
buffAll.buff1[strlen(buffAll_c.buff1)]=0; // GOOD
59-
buffAll.buff2[strlen(buffAll.buff1)]=0; // GOOD
60-
buffAll1->buff1[strlen(buffAll1_c->buff1)]=0; // GOOD
61-
buffAll1->buff2[strlen(buffAll1->buff1)]=0; // GOOD
62-
globalBuff1.buff1[strlen(globalBuff1_c.buff1)]=0; // GOOD
63-
globalBuff1.buff2[strlen(globalBuff1.buff1)]=0; // GOOD
64-
globalBuff2->buff1[strlen(globalBuff2_c->buff1)]=0; // GOOD
65-
globalBuff2->buff2[strlen(globalBuff2->buff1)]=0; // GOOD
71+
buffAll.array[strlen(buffAll_c.array)]=0; // GOOD
72+
buffAll.pointer[strlen(buffAll.array)]=0; // GOOD
73+
buffAll1->array[strlen(buffAll1_c->array)]=0; // GOOD
74+
buffAll1->pointer[strlen(buffAll1->array)]=0; // GOOD
75+
globalBuff1.array[strlen(globalBuff1_c.array)]=0; // GOOD
76+
globalBuff1.pointer[strlen(globalBuff1.array)]=0; // GOOD
77+
globalBuff2->array[strlen(globalBuff2_c->array)]=0; // GOOD
78+
globalBuff2->pointer[strlen(globalBuff2->array)]=0; // GOOD
6679
}
67-
void goodFunc0(){
80+
81+
void strlen_test3(){
6882
unsigned char buffer[12];
6983
int i;
7084
for(i = 0; i < 6; i++)

0 commit comments

Comments
 (0)