Skip to content

Commit 5300dd2

Browse files
committed
C++: Merge the experimental query 'cpp/access-memory-location-after-end-buffer-strncat' into 'cpp/unsafe-strncat'.
1 parent c4f604b commit 5300dd2

File tree

3 files changed

+66
-9
lines changed

3 files changed

+66
-9
lines changed

cpp/ql/src/Likely Bugs/Memory Management/SuspiciousCallToStrncat.cpp

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,3 +2,7 @@ strncat(dest, src, strlen(dest)); //wrong: should use remaining size of dest
22

33
strncat(dest, src, sizeof(dest)); //wrong: should use remaining size of dest.
44
//Also fails if dest is a pointer and not an array.
5+
6+
strncat(dest, source, sizeof(dest) - strlen(dest)); // wrong: writes a zero byte past the `dest` buffer.
7+
8+
strncat(dest, source, sizeof(dest) - strlen(dest) - 1); // correct: reserves space for the zero byte.

cpp/ql/src/Likely Bugs/Memory Management/SuspiciousCallToStrncat.qhelp

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,11 @@
44
<qhelp>
55
<overview>
66
<p>The standard library function <code>strncat</code> appends a source string to a target string.
7-
The third argument defines the maximum number of characters to append and should be less than or equal to the remaining space in the destination buffer. Calls of the form <code>strncat(dest, src, strlen(dest))</code> or <code>strncat(dest, src, sizeof(dest))</code> set the third argument to the entire size of the destination buffer. Executing a call of this type may cause a buffer overflow unless the buffer is known to be empty. Buffer overflows can lead to anything from a segmentation fault to a security vulnerability.</p>
7+
The third argument defines the maximum number of characters to append and should be less than or equal to the remaining space in the destination buffer.
8+
Calls of the form <code>strncat(dest, src, strlen(dest))</code> or <code>strncat(dest, src, sizeof(dest))</code> set the third argument to the entire size of the destination buffer.
9+
Executing a call of this type may cause a buffer overflow unless the buffer is known to be empty.
10+
Similarly, calls of the form <code>strncat(dest, src, sizeof (dest) - strlen (dest))</code> allows one byte to be written ouside the `dest` buffer.
11+
Buffer overflows can lead to anything from a segmentation fault to a security vulnerability.</p>
812

913
</overview>
1014
<recommendation>
@@ -25,6 +29,10 @@ The third argument defines the maximum number of characters to append and should
2529
<li>
2630
M. Donaldson, <em>Inside the Buffer Overflow Attack: Mechanism, Method &amp; Prevention</em>. SANS Institute InfoSec Reading Room, 2002.
2731
</li>
32+
<li>
33+
CERT C Coding Standard:
34+
<a href="https://wiki.sei.cmu.edu/confluence/display/c/STR31-C.+Guarantee+that+storage+for+strings+has+sufficient+space+for+character+data+and+the+null+terminator">STR31-C. Guarantee that storage for strings has sufficient space for character data and the null terminator</a>.
35+
</li>
2836

2937

3038
</references>
Lines changed: 53 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,26 +1,71 @@
11
/**
22
* @name Potentially unsafe call to strncat
3-
* @description Calling 'strncat' with the size of the destination buffer
4-
* as the third argument may result in a buffer overflow.
3+
* @description Calling `strncat` with the size of the destination buffer as the third argument may result in a buffer overflow.
4+
* Similarly, calling `strncat` with `sizeof (dest) - strlen (dest)` as the third argument may result in a buffer overflow.
55
* @kind problem
66
* @problem.severity warning
77
* @precision medium
88
* @id cpp/unsafe-strncat
99
* @tags reliability
1010
* correctness
1111
* security
12+
* external/cwe/cwe-788
1213
* external/cwe/cwe-676
1314
* external/cwe/cwe-119
1415
* external/cwe/cwe-251
1516
*/
1617

1718
import cpp
1819
import Buffer
20+
import semmle.code.cpp.models.implementations.Strcat
21+
import semmle.code.cpp.valuenumbering.GlobalValueNumbering
1922

20-
from FunctionCall fc, VariableAccess va1, VariableAccess va2
21-
where
22-
fc.getTarget().(Function).hasName("strncat") and
23-
va1 = fc.getArgument(0) and
24-
va2 = fc.getArgument(2).(BufferSizeExpr).getArg() and
25-
va1.getTarget() = va2.getTarget()
23+
/**
24+
* Holds if `call` is a call to `strncat` such that `sizeArg` and `destArg` are the size and
25+
* destination arguments, respectively.
26+
*/
27+
predicate interestringCallWithArgs(Call call, Expr sizeArg, Expr destArg) {
28+
exists(StrcatFunction strcat |
29+
strcat = call.getTarget() and
30+
sizeArg = call.getArgument(strcat.getParamSize()) and
31+
destArg = call.getArgument(strcat.getParamDest())
32+
)
33+
}
34+
35+
/**
36+
* Holds if `fc` is a call to `strncat` with size argument `sizeArg` and destination
37+
* argument `destArg`, and `destArg` is the size of the buffer pointed to by `destArg`.
38+
*/
39+
predicate case1(FunctionCall fc, Expr sizeArg, VariableAccess destArg) {
40+
interestringCallWithArgs(fc, sizeArg, destArg) and
41+
exists(StrcatFunction strncat, VariableAccess va |
42+
fc.getTarget() = strncat and
43+
destArg = fc.getArgument(strncat.getParamDest()) and
44+
va = sizeArg.(BufferSizeExpr).getArg() and
45+
destArg.getTarget() = va.getTarget()
46+
)
47+
}
48+
49+
/**
50+
* Holds if `fc` is a call to `strncat` with size argument `sizeArg` and destination
51+
* argument `destArg`, and `sizeArg` computes the value `sizeof (dest) - strlen (dest)`.
52+
*/
53+
predicate case2(FunctionCall fc, Expr sizeArg, VariableAccess destArg) {
54+
interestringCallWithArgs(fc, sizeArg, destArg) and
55+
exists(SubExpr sub, int n |
56+
// The destination buffer is an array of size n
57+
destArg.getUnspecifiedType().(ArrayType).getSize() = n and
58+
// The size argument is equivalent to a subtraction
59+
globalValueNumber(sizeArg).getAnExpr() = sub and
60+
// ... where the left side of the subtraction is the constant n
61+
globalValueNumber(sub.getLeftOperand()).getAnExpr().getValue().toInt() = n and
62+
// ... and the right side of the subtraction is a call to `strlen` where the argument is the
63+
// destination buffer.
64+
globalValueNumber(sub.getRightOperand()).getAnExpr().(StrlenCall).getStringExpr() =
65+
globalValueNumber(destArg).getAnExpr()
66+
)
67+
}
68+
69+
from FunctionCall fc, Expr sizeArg, Expr destArg
70+
where case1(fc, sizeArg, destArg) or case2(fc, sizeArg, destArg)
2671
select fc, "Potentially unsafe call to strncat."

0 commit comments

Comments
 (0)