mirror of
https://github.com/github/codeql.git
synced 2025-12-20 10:46:30 +01:00
Merge pull request #5938 from MathiasVP/promote-access-of-memory-location-after-end-of-buffer-using-strncat
C++: Promote `cpp/access-memory-location-after-end-buffer-strncat` out of experimental
This commit is contained in:
Geoffrey White
@@ -2,3 +2,7 @@ strncat(dest, src, strlen(dest)); //wrong: should use remaining size of dest
|
||||
|
||||
strncat(dest, src, sizeof(dest)); //wrong: should use remaining size of dest.
|
||||
//Also fails if dest is a pointer and not an array.
|
||||
|
||||
strncat(dest, source, sizeof(dest) - strlen(dest)); // wrong: writes a zero byte past the `dest` buffer.
|
||||
|
||||
strncat(dest, source, sizeof(dest) - strlen(dest) - 1); // correct: reserves space for the zero byte.
|
||||
|
||||
@@ -4,7 +4,17 @@
|
||||
<qhelp>
|
||||
<overview>
|
||||
<p>The standard library function <code>strncat</code> appends a source string to a target string.
|
||||
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>
|
||||
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.</p>
|
||||
|
||||
<p>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.</p>
|
||||
|
||||
<p>Similarly, calls of the form <code>strncat(dest, src, sizeof (dest) - strlen (dest))</code> allow one
|
||||
byte to be written ouside the <code>dest</code> buffer.</p>
|
||||
|
||||
<p>Buffer overflows can lead to anything from a segmentation fault to a security vulnerability.</p>
|
||||
|
||||
</overview>
|
||||
<recommendation>
|
||||
@@ -25,6 +35,10 @@ The third argument defines the maximum number of characters to append and should
|
||||
<li>
|
||||
M. Donaldson, <em>Inside the Buffer Overflow Attack: Mechanism, Method & Prevention</em>. SANS Institute InfoSec Reading Room, 2002.
|
||||
</li>
|
||||
<li>
|
||||
CERT C Coding Standard:
|
||||
<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>.
|
||||
</li>
|
||||
|
||||
|
||||
</references>
|
||||
|
||||
@@ -1,7 +1,6 @@
|
||||
/**
|
||||
* @name Potentially unsafe call to strncat
|
||||
* @description Calling 'strncat' with the size of the destination buffer
|
||||
* as the third argument may result in a buffer overflow.
|
||||
* @description Calling 'strncat' with an incorrect size argument may result in a buffer overflow.
|
||||
* @kind problem
|
||||
* @problem.severity warning
|
||||
* @precision medium
|
||||
@@ -9,6 +8,7 @@
|
||||
* @tags reliability
|
||||
* correctness
|
||||
* security
|
||||
* external/cwe/cwe-788
|
||||
* external/cwe/cwe-676
|
||||
* external/cwe/cwe-119
|
||||
* external/cwe/cwe-251
|
||||
@@ -16,11 +16,53 @@
|
||||
|
||||
import cpp
|
||||
import Buffer
|
||||
import semmle.code.cpp.models.implementations.Strcat
|
||||
import semmle.code.cpp.valuenumbering.GlobalValueNumbering
|
||||
|
||||
from FunctionCall fc, VariableAccess va1, VariableAccess va2
|
||||
where
|
||||
fc.getTarget().(Function).hasName("strncat") and
|
||||
va1 = fc.getArgument(0) and
|
||||
va2 = fc.getArgument(2).(BufferSizeExpr).getArg() and
|
||||
va1.getTarget() = va2.getTarget()
|
||||
/**
|
||||
* Holds if `call` is a call to `strncat` such that `sizeArg` and `destArg` are the size and
|
||||
* destination arguments, respectively.
|
||||
*/
|
||||
predicate interestringCallWithArgs(Call call, Expr sizeArg, Expr destArg) {
|
||||
exists(StrcatFunction strcat |
|
||||
strcat = call.getTarget() and
|
||||
sizeArg = call.getArgument(strcat.getParamSize()) and
|
||||
destArg = call.getArgument(strcat.getParamDest())
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* Holds if `fc` is a call to `strncat` with size argument `sizeArg` and destination
|
||||
* argument `destArg`, and `destArg` is the size of the buffer pointed to by `destArg`.
|
||||
*/
|
||||
predicate case1(FunctionCall fc, Expr sizeArg, VariableAccess destArg) {
|
||||
interestringCallWithArgs(fc, sizeArg, destArg) and
|
||||
exists(VariableAccess va |
|
||||
va = sizeArg.(BufferSizeExpr).getArg() and
|
||||
destArg.getTarget() = va.getTarget()
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* Holds if `fc` is a call to `strncat` with size argument `sizeArg` and destination
|
||||
* argument `destArg`, and `sizeArg` computes the value `sizeof (dest) - strlen (dest)`.
|
||||
*/
|
||||
predicate case2(FunctionCall fc, Expr sizeArg, VariableAccess destArg) {
|
||||
interestringCallWithArgs(fc, sizeArg, destArg) and
|
||||
exists(SubExpr sub, int n |
|
||||
// The destination buffer is an array of size n
|
||||
destArg.getUnspecifiedType().(ArrayType).getSize() = n and
|
||||
// The size argument is equivalent to a subtraction
|
||||
globalValueNumber(sizeArg).getAnExpr() = sub and
|
||||
// ... where the left side of the subtraction is the constant n
|
||||
globalValueNumber(sub.getLeftOperand()).getAnExpr().getValue().toInt() = n and
|
||||
// ... and the right side of the subtraction is a call to `strlen` where the argument is the
|
||||
// destination buffer.
|
||||
globalValueNumber(sub.getRightOperand()).getAnExpr().(StrlenCall).getStringExpr() =
|
||||
globalValueNumber(destArg).getAnExpr()
|
||||
)
|
||||
}
|
||||
|
||||
from FunctionCall fc, Expr sizeArg, Expr destArg
|
||||
where case1(fc, sizeArg, destArg) or case2(fc, sizeArg, destArg)
|
||||
select fc, "Potentially unsafe call to strncat."
|
||||
|
||||
@@ -1,4 +0,0 @@
|
||||
|
||||
strncat(dest, source, sizeof(dest) - strlen(dest)); // BAD: writes a zero byte past the `dest` buffer.
|
||||
|
||||
strncat(dest, source, sizeof(dest) - strlen(dest) -1); // GOOD: Reserves space for the zero byte.
|
||||
@@ -1,32 +0,0 @@
|
||||
<!DOCTYPE qhelp PUBLIC
|
||||
"-//Semmle//qhelp//EN"
|
||||
"qhelp.dtd">
|
||||
<qhelp>
|
||||
<overview>
|
||||
<p>The standard library function <code>strncat(dest, source, count)</code> appends the <code>source</code> string to the <code>dest</code> string. <code>count</code> specifies the maximum number of characters to append and must be less than the remaining space in the target buffer. Calls of the form <code> strncat (dest, source, sizeof (dest) - strlen (dest)) </code> set the third argument to one more than possible. So when the <code>dest</code> is full, the expression <code> sizeof (dest) - strlen (dest) </code> will be equal to one, and not zero as the programmer might think. Making a call of this type may result in a zero byte being written just outside the <code>dest</code> buffer.</p>
|
||||
|
||||
|
||||
</overview>
|
||||
<recommendation>
|
||||
|
||||
<p>We recommend subtracting one from the third argument. For example, replace <code>strncat(dest, source, sizeof(dest)-strlen(dest))</code> with <code>strncat(dest, source, sizeof(dest)-strlen(dest)-1)</code>.</p>
|
||||
|
||||
</recommendation>
|
||||
<example>
|
||||
<p>The following example demonstrates an erroneous and corrected use of the <code>strncat</code> function.</p>
|
||||
<sample src="AccessOfMemoryLocationAfterEndOfBufferUsingStrncat.c" />
|
||||
|
||||
</example>
|
||||
<references>
|
||||
|
||||
<li>
|
||||
CERT C Coding Standard:
|
||||
<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>.
|
||||
</li>
|
||||
<li>
|
||||
CERT C Coding Standard:
|
||||
<a href="https://wiki.sei.cmu.edu/confluence/display/c/ARR30-C.+Do+not+form+or+use+out-of-bounds+pointers+or+array+subscripts">ARR30-C. Do not form or use out-of-bounds pointers or array subscripts</a>.
|
||||
</li>
|
||||
|
||||
</references>
|
||||
</qhelp>
|
||||
@@ -1,42 +0,0 @@
|
||||
/**
|
||||
* @name Access Of Memory Location After The End Of A Buffer Using Strncat
|
||||
* @description Calls of the form `strncat(dest, source, sizeof (dest) - strlen (dest))` set the third argument to one more than possible. So when `dest` is full, the expression `sizeof(dest) - strlen (dest)` will be equal to one, and not zero as the programmer might think. Making a call of this type may result in a zero byte being written just outside the `dest` buffer.
|
||||
* @kind problem
|
||||
* @id cpp/access-memory-location-after-end-buffer-strncat
|
||||
* @problem.severity warning
|
||||
* @precision medium
|
||||
* @tags correctness
|
||||
* security
|
||||
* external/cwe/cwe-788
|
||||
*/
|
||||
|
||||
import cpp
|
||||
import semmle.code.cpp.models.implementations.Strcat
|
||||
import semmle.code.cpp.valuenumbering.GlobalValueNumbering
|
||||
|
||||
/**
|
||||
* Holds if `call` is a call to `strncat` such that `sizeArg` and `destArg` are the size and
|
||||
* destination arguments, respectively.
|
||||
*/
|
||||
predicate interestringCallWithArgs(Call call, Expr sizeArg, Expr destArg) {
|
||||
exists(StrcatFunction strcat |
|
||||
strcat = call.getTarget() and
|
||||
sizeArg = call.getArgument(strcat.getParamSize()) and
|
||||
destArg = call.getArgument(strcat.getParamDest())
|
||||
)
|
||||
}
|
||||
|
||||
from FunctionCall call, Expr sizeArg, Expr destArg, SubExpr sub, int n
|
||||
where
|
||||
interestringCallWithArgs(call, sizeArg, destArg) and
|
||||
// The destination buffer is an array of size n
|
||||
destArg.getUnspecifiedType().(ArrayType).getSize() = n and
|
||||
// The size argument is equivalent to a subtraction
|
||||
globalValueNumber(sizeArg).getAnExpr() = sub and
|
||||
// ... where the left side of the subtraction is the constant n
|
||||
globalValueNumber(sub.getLeftOperand()).getAnExpr().getValue().toInt() = n and
|
||||
// ... and the right side of the subtraction is a call to `strlen` where the argument is the
|
||||
// destination buffer.
|
||||
globalValueNumber(sub.getRightOperand()).getAnExpr().(StrlenCall).getStringExpr() =
|
||||
globalValueNumber(destArg).getAnExpr()
|
||||
select call, "Possible out-of-bounds write due to incorrect size argument."
|
||||
@@ -1,9 +1,9 @@
|
||||
| test.c:54:3:54:24 | ... = ... | potential unsafe or redundant assignment. |
|
||||
| test.c:55:3:55:40 | ... = ... | potential unsafe or redundant assignment. |
|
||||
| test.c:56:3:56:44 | ... = ... | potential unsafe or redundant assignment. |
|
||||
| test.c:57:3:57:44 | ... = ... | potential unsafe or redundant assignment. |
|
||||
| test.c:58:3:58:48 | ... = ... | potential unsafe or redundant assignment. |
|
||||
| test.c:59:3:59:48 | ... = ... | potential unsafe or redundant assignment. |
|
||||
| test.c:60:3:60:52 | ... = ... | potential unsafe or redundant assignment. |
|
||||
| test.c:61:3:61:50 | ... = ... | potential unsafe or redundant assignment. |
|
||||
| test.c:62:3:62:54 | ... = ... | potential unsafe or redundant assignment. |
|
||||
| test.c:16:3:16:24 | ... = ... | potential unsafe or redundant assignment. |
|
||||
| test.c:17:3:17:40 | ... = ... | potential unsafe or redundant assignment. |
|
||||
| test.c:18:3:18:44 | ... = ... | potential unsafe or redundant assignment. |
|
||||
| test.c:19:3:19:44 | ... = ... | potential unsafe or redundant assignment. |
|
||||
| test.c:20:3:20:48 | ... = ... | potential unsafe or redundant assignment. |
|
||||
| test.c:21:3:21:48 | ... = ... | potential unsafe or redundant assignment. |
|
||||
| test.c:22:3:22:52 | ... = ... | potential unsafe or redundant assignment. |
|
||||
| test.c:23:3:23:50 | ... = ... | potential unsafe or redundant assignment. |
|
||||
| test.c:24:3:24:54 | ... = ... | potential unsafe or redundant assignment. |
|
||||
|
||||
@@ -1,5 +0,0 @@
|
||||
| test.c:8:3:8:9 | call to strncat | Possible out-of-bounds write due to incorrect size argument. |
|
||||
| test.c:9:3:9:9 | call to strncat | Possible out-of-bounds write due to incorrect size argument. |
|
||||
| test.c:17:3:17:9 | call to strncat | Possible out-of-bounds write due to incorrect size argument. |
|
||||
| test.c:18:3:18:9 | call to strncat | Possible out-of-bounds write due to incorrect size argument. |
|
||||
| test.c:46:3:46:9 | call to strncat | Possible out-of-bounds write due to incorrect size argument. |
|
||||
@@ -1 +0,0 @@
|
||||
experimental/Security/CWE/CWE-788/AccessOfMemoryLocationAfterEndOfBufferUsingStrncat.ql
|
||||
@@ -2,50 +2,12 @@ char * strncat(char*, const char*, unsigned);
|
||||
unsigned strlen(const char*);
|
||||
void* malloc(unsigned);
|
||||
|
||||
void strncat_test1(char *s) {
|
||||
char buf[80];
|
||||
strncat(buf, s, sizeof(buf) - strlen(buf) - 1); // GOOD
|
||||
strncat(buf, s, sizeof(buf) - strlen(buf)); // BAD
|
||||
strncat(buf, "fix", sizeof(buf)-strlen(buf)); // BAD
|
||||
}
|
||||
|
||||
#define MAX_SIZE 80
|
||||
|
||||
void strncat_test2(char *s) {
|
||||
char buf[MAX_SIZE];
|
||||
strncat(buf, s, MAX_SIZE - strlen(buf) - 1); // GOOD
|
||||
strncat(buf, s, MAX_SIZE - strlen(buf)); // BAD
|
||||
strncat(buf, "fix", MAX_SIZE - strlen(buf)); // BAD
|
||||
}
|
||||
|
||||
void strncat_test3(char *s) {
|
||||
int len = 80;
|
||||
char* buf = (char *) malloc(len);
|
||||
strncat(buf, s, len - strlen(buf) - 1); // GOOD
|
||||
strncat(buf, s, len - strlen(buf)); // BAD [NOT DETECTED]
|
||||
strncat(buf, "fix", len - strlen(buf)); // BAD [NOT DETECTED]
|
||||
}
|
||||
|
||||
void strncat_test4(char *s) {
|
||||
int len = 80;
|
||||
char* buf = (char *) malloc(len + 1);
|
||||
strncat(buf, s, len - strlen(buf) - 1); // GOOD
|
||||
strncat(buf, s, len - strlen(buf)); // GOOD
|
||||
}
|
||||
|
||||
struct buffers
|
||||
{
|
||||
unsigned char array[50];
|
||||
unsigned char *pointer;
|
||||
} globalBuff1,*globalBuff2,globalBuff1_c,*globalBuff2_c;
|
||||
|
||||
void strncat_test5(char* s, struct buffers* buffers) {
|
||||
unsigned len_array = strlen(buffers->array);
|
||||
unsigned max_size = sizeof(buffers->array);
|
||||
unsigned free_size = max_size - len_array;
|
||||
strncat(buffers->array, s, free_size); // BAD
|
||||
}
|
||||
|
||||
void strlen_test1(){
|
||||
unsigned char buff1[12];
|
||||
struct buffers buffAll;
|
||||
|
||||
@@ -1 +1,5 @@
|
||||
| test.c:24:2:24:8 | call to strncat | Potentially unsafe call to strncat. |
|
||||
| test.c:45:3:45:9 | call to strncat | Potentially unsafe call to strncat. |
|
||||
| test.c:67:3:67:9 | call to strncat | Potentially unsafe call to strncat. |
|
||||
| test.c:75:3:75:9 | call to strncat | Potentially unsafe call to strncat. |
|
||||
| test.c:76:3:76:9 | call to strncat | Potentially unsafe call to strncat. |
|
||||
|
||||
@@ -39,3 +39,46 @@ void bad1(char *s) {
|
||||
strncat(buf, ".", 1); // BAD [NOT DETECTED] -- Need to check if any space is left
|
||||
}
|
||||
|
||||
void strncat_test1(char *s) {
|
||||
char buf[80];
|
||||
strncat(buf, s, sizeof(buf) - strlen(buf) - 1); // GOOD
|
||||
strncat(buf, s, sizeof(buf) - strlen(buf)); // BAD
|
||||
}
|
||||
|
||||
void* malloc(size_t);
|
||||
|
||||
void strncat_test2(char *s) {
|
||||
int len = 80;
|
||||
char* buf = (char *)malloc(len);
|
||||
strncat(buf, s, len - strlen(buf) - 1); // GOOD
|
||||
strncat(buf, s, len - strlen(buf)); // BAD [NOT DETECTED]
|
||||
}
|
||||
|
||||
struct buffers
|
||||
{
|
||||
char array[50];
|
||||
char* pointer;
|
||||
};
|
||||
|
||||
void strncat_test3(char* s, struct buffers* buffers) {
|
||||
unsigned len_array = strlen(buffers->array);
|
||||
unsigned max_size = sizeof(buffers->array);
|
||||
unsigned free_size = max_size - len_array;
|
||||
strncat(buffers->array, s, free_size); // BAD
|
||||
}
|
||||
|
||||
#define MAX_SIZE 80
|
||||
|
||||
void strncat_test4(char *s) {
|
||||
char buf[MAX_SIZE];
|
||||
strncat(buf, s, MAX_SIZE - strlen(buf) - 1); // GOOD
|
||||
strncat(buf, s, MAX_SIZE - strlen(buf)); // BAD
|
||||
strncat(buf, "...", MAX_SIZE - strlen(buf)); // BAD
|
||||
}
|
||||
|
||||
void strncat_test5(char *s) {
|
||||
int len = 80;
|
||||
char* buf = (char *) malloc(len + 1);
|
||||
strncat(buf, s, len - strlen(buf) - 1); // GOOD
|
||||
strncat(buf, s, len - strlen(buf)); // GOOD
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user