Contents
V645. Function call may lead to buffer overflow. Bounds should not contain size of a buffer, but a number of characters it can hold.
The analyzer detected a potential error related to string concatenation. An error can cause buffer overflow. A program may run consistently for a long time if only short strings come to input. This makes such errors nasty.
Functions like 'strncat', 'wcsncat' and others [1] are subject to this type of vulnerability.
'strncat' function description:
char *strncat(
char *strDest,
const char *strSource,
size_t count
);Where:
- 'destination' - desnination string;
- 'source' - source string;
- 'count' - maximum number of characters you can add.
The 'strncat' function is perhaps one of the most dangerous string functions. Its working principle differs from the way programmers imagine it.
The third argument does not specify the size of the buffer—it indicates the number of characters that you can place in it. MSDN describes this function as follows: "strncat does not check for sufficient space in strDest; it is therefore a potential cause of buffer overruns. Keep in mind that count limits the number of characters appended; it is not a limit on the size of strDest."
Developers often forget this and use 'strncat' in wrong ways. 3 types of common mistakes:
1) Developers think that the 'count' argument is the size of the 'strdest' buffer. This misunderstanding results in incorrect code, as follows:
char newProtoFilter[2048] = "....";
strncat(newProtoFilter, szTemp, 2048);
strncat(newProtoFilter, "|", 2048);The author passes 2048 as the third argument. The developer mistakes to believe that it protects the code from overflow. That's not the case. In fact, this indicates that we can add up to 2048 characters to the string!
2) Developers forget that the 'strncat' function will add terminal 0 after copying characters. Example of dangerous code:
char filename[NNN];
...
strncat(filename,
dcc->file_info.filename,
sizeof(filename) - strlen(filename));It may seem that the developer has secured from 'filename' buffer overflow. That's not true. The code author subtracted the length of the string from the array size. If the entire string is already filled, the expression 'sizeof (filename) - strlen(filename)' will return 1. As a result, one more character will be added to the string, and the terminal null will be written outside the buffer boundary.
This simple example explains the mistake:
char buf[5] = "ABCD";
strncat(buf, "E", 5 - strlen(buf));There is no room for new characters in the buffer anymore. It contains 4 characters and the terminal null. The expression "5 - strlen(buf)" equals 1. strncpy() will copy "E" to the last element of the 'buf' array. Terminal 0 will be written outside the buffer!
3) Developers forget the integer overflow factor. Look at this error example:
struct A
{
....
char consoleText[512];
};
void foo(A a)
{
char inputBuffer[1024];
....
strncat(a.consoleText, inputBuffer,
sizeof(a.consoleText) - strlen(a.consoleText) - 5);
}Here, an infix expression is used as the third argument. Once reviewed heedlessly, the value of the expression "sizeof(a.consoleText) - strlen(a.consoleText) – 5" lies in the range [0, 507], and the code is correct. But that's not so.
- The result of the 'strlen (A.Consoletext)' function can be in the range [0, 511].
- If 'strlen(a.consoleText)' returns a value from 0 to 507, the resulting expression value will also be in the range [0, 507]. 'a.consoleText' buffer overflow will not happen.
- If 'strlen(a.consoleText)' returns a value from 508 to 511, an unsigned overflow will occur in the resulting expression. If the type 'size_t' is 64-bit size, we'll get the range [0xFFFFFFFFFFFFFFFC, 0xFFFFFFFFFFFFFFFF] accordingly. It's like you can write a huge number of characters into the buffer. Obviously, that's not the case. Eventually, we get the 'a.consoleText' buffer overflow.
Fixed versions of the above examples:
// Sample N1
char newProtoFilter[2048] = "....";
strncat(newProtoFilter, szTemp,
2048 - 1 - strlen(newProtoFilter));
strncat(newProtoFilter, "|",
2048 - 1 - strlen(newProtoFilter));
// Sample N2
char filename[NNN];
...
strncat(filename,
dcc->file_info.filename,
sizeof(filename) - strlen(filename) - 1);
// Sample N3
void foo(A a)
{
char inputBuffer[1024];
....
size_t textSize = strlen(a.consoleText);
if (sizeof(a.consoleText) - textSize > 5u)
{
strncat(a.consoleText, inputBuffer,
sizeof(a.consoleText) - textSize - 5);
}
else
{
// ....
}
}This code is not readable or truly safe. A much better solution would be to avoid the use of 'strncat' functions in favor of more secure ones. For example, one can use the 'std::string' class or functions such as 'strncat_s', and others [2].
Resources
- MSDN. strncat, _strncat_l, wcsncat, wcsncat_l, _mbsncat _mbsncat_l
- MSDN. strncat_s, _strncat_s_l, wcsncat_s, _wcsncat_s_l, _mbsncat_s, _mbsncat_s_l
This diagnostic is classified as:
You can look at examples of errors detected by the V645 diagnostic. |
Was this page helpful?
