Skip to content

Commit d3d56fb

Browse files
MathiasVPMathiasVP
authored
Merge pull request #5011 from ihsinme/ihsinme-patch-221
CPP: add query for CWE-788 Access of memory location after the end of a buffer using strlen.
2 parents 9b39163 + b7df18b commit d3d56fb

6 files changed

+129
-0
lines changed

‎cpp/ql/src/experimental/Security/CWE/CWE-788/AccessOfMemoryLocationAfterEndOfBufferUsingStrlen.c

+9
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
// BAD: if buffer does not have a terminal zero, then access outside the allocated memory is possible.
2+
3+
buffer[strlen(buffer)] = 0;
4+
5+
6+
// GOOD: we will eliminate dangerous behavior if we use a different method of calculating the length.
7+
size_t len;
8+
...
9+
buffer[len] = 0

‎cpp/ql/src/experimental/Security/CWE/CWE-788/AccessOfMemoryLocationAfterEndOfBufferUsingStrlen.qhelp

+31
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>Potentially dangerous use of the strlen function to calculate the length of a string.
7+
The expression <code>buffer[strlen(buffer)] = 0</code> is potentially dangerous, if the variable buffer does not have a terminal zero, then access beyond the bounds of the allocated memory is possible, which will lead to undefined behavior.
8+
If terminal zero is present, then the specified expression is meaningless.</p>
9+
10+
<p>False positives include heavily nested strlen. This situation is unlikely.</p>
11+
12+
</overview>
13+
<recommendation>
14+
15+
<p>We recommend using another method for calculating the string length</p>
16+
17+
</recommendation>
18+
<example>
19+
<p>The following example demonstrates an erroneous and corrected use of the strlen function.</p>
20+
<sample src="AccessOfMemoryLocationAfterEndOfBufferUsingStrlen.c" />
21+
22+
</example>
23+
<references>
24+
25+
<li>
26+
CERT C Coding Standard:
27+
<a href="https://wiki.sei.cmu.edu/confluence/display/c/STR32-C.+Do+not+pass+a+non-null-terminated+character+sequence+to+a+library+function+that+expects+a+string">STR32-C. Do not pass a non-null-terminated character sequence to a library function that expects a string</a>.
28+
</li>
29+
30+
</references>
31+
</qhelp>

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

+34
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
/**
2+
* @name Access Of Memory Location After End Of Buffer
3+
* @description The expression `buffer [strlen (buffer)] = 0` is potentially dangerous, if the variable `buffer` does not have a terminal zero, then access beyond the bounds of the allocated memory is possible, which will lead to undefined behavior.
4+
* If terminal zero is present, then the specified expression is meaningless.
5+
* @kind problem
6+
* @id cpp/access-memory-location-after-end-buffer
7+
* @problem.severity warning
8+
* @precision medium
9+
* @tags correctness
10+
* security
11+
* external/cwe/cwe-788
12+
*/
13+
14+
import cpp
15+
import semmle.code.cpp.valuenumbering.GlobalValueNumbering
16+
import semmle.code.cpp.dataflow.DataFlow
17+
18+
from StrlenCall fc, AssignExpr expr, ArrayExpr exprarr
19+
where
20+
exprarr = expr.getLValue() and
21+
expr.getRValue().getValue().toInt() = 0 and
22+
globalValueNumber(exprarr.getArrayOffset()) = globalValueNumber(fc) and
23+
not exists(Expr exptmp |
24+
(
25+
DataFlow::localExprFlow(fc, exptmp) or
26+
exptmp.getAChild*() = fc.getArgument(0).(VariableAccess).getTarget().getAnAccess()
27+
) and
28+
dominates(exptmp, expr) and
29+
postDominates(exptmp, fc) and
30+
not exptmp.getEnclosingStmt() = fc.getEnclosingStmt() and
31+
not exptmp.getEnclosingStmt() = expr.getEnclosingStmt()
32+
) and
33+
globalValueNumber(fc.getArgument(0)) = globalValueNumber(exprarr.getArrayBase())
34+
select expr, "potential unsafe or redundant assignment."

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

+9
Original file line numberDiff line numberDiff line change
@@ -0,0 +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. |

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

+1
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
experimental/Security/CWE/CWE-788/AccessOfMemoryLocationAfterEndOfBufferUsingStrlen.ql

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

+45
Original file line numberDiff line numberDiff line change
@@ -26,3 +26,48 @@ void workFunction_2_1(char *s) {
2626
strncat(buf, s, len-strlen(buf)-1); // GOOD
2727
strncat(buf, s, len-strlen(buf)); // GOOD
2828
}
29+
30+
struct buffers
31+
{
32+
unsigned char buff1[50];
33+
unsigned char *buff2;
34+
} globalBuff1,*globalBuff2,globalBuff1_c,*globalBuff2_c;
35+
36+
37+
void badFunc0(){
38+
unsigned char buff1[12];
39+
struct buffers buffAll;
40+
struct buffers * buffAll1;
41+
42+
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
51+
}
52+
void noBadFunc0(){
53+
unsigned char buff1[12],buff1_c[12];
54+
struct buffers buffAll,buffAll_c;
55+
struct buffers * buffAll1,*buffAll1_c;
56+
57+
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
66+
}
67+
void goodFunc0(){
68+
unsigned char buffer[12];
69+
int i;
70+
for(i = 0; i < 6; i++)
71+
buffer[i] = 'A';
72+
buffer[i]=0;
73+
}

0 commit comments

Comments
 (0)