I don't really get, what you are trying to accomplish here, but the vulnerability comes from letting the userhacker supply a format string. He can then supply %n which causes the printf-function-family to write to memory (the number of bytes written so far).
int answer = 42;
printf("Hello %nworld\n", &answer);
// "Hello " is 6 bytes long
printf(
"The answer to the ultimate question of life, "
"the universe and everything is: %i\n"
, answer
);
Will print 6 as answer, instead of 42.
Answer from Bodo Thiesen on Stack OverflowVideos
Your code is fine.
The issue is that if you pass a string that is user controlled as a printf format string, security bugs can arise.
For instance, printf(userName);
Where userName is supplied by the user, a user can pass "%s", and get your function to start accessing data at a random address on the stack, which could result in a crash. printf will try to pop additional parameters off the stack, resulting in a stack corruption. Denial of service attack like this is probably the best case, information can be disclosed by getting printf to dump out values on the stack and there are even ways to get printf style functions to modify the return address on the stack.
Since your strings are not user controlled, it is safe to ignore this message. The typical fix is to replace the printf example I gave with printf("%s", userName);, which would not appear to help in your case because the const strings appear to contain format strings.
Wikipedia has more on format string vulnerabilities here: http://en.wikipedia.org/wiki/Format_string_vulnerabilities
Idea is that value of testStrings[testID] can be changed somehow to include extra format specifiers.
Because snprintf() has no possibility to check whether number of parameters match the number of format specifiers it will just take next address from stack to use as value for next format specifier and weird things can happen then.
It is known as format string attack.
What happens in these 2 cases ?
Case 1
char buf[3];
vsprint(buf, "%s", args);
Case 2
char buf[3];
vsnprint(buf, sizeof buf, "%s", args);
In case 1, if the string you're formatting has a length of 3 or greater, you have a buffer overrun, vsprintf might write to memory past the storage of the buf array, which is undefined behavior, possibly causing havoc/security concerns/crashes/etc.
In case 2. vsnprintf knows how big the buffer that will contain the result is, and it will make sure not to go past that(instead truncating the result to fit within buf ).
It's because vsnprintf has an additional size_t count parameter that vsprintf (and other non-n *sprintf methods) does not have. The implementation uses this to ensure that the data it writes to your buffer will not run off the end.
Data that runs off the end of a buffer can result in data corruption, or when maliciously exploited can be used as a buffer overrun attack.
Pre-C99 affords no simply solution to format strings with a high degree of safety of preventing buffer overruns.
It is those pesky "%s", "%[]", "%f" format specifiers that require so much careful consideration with their potential long output. Thus the need for such a function. @Jonathan Leffler
To do so with those early compilers obliges code to analyze format and the arguments to find the required size. At that point, code is nearly there to making you own complete my_vsnprintf(). I'd seek existing solutions for that. @user694733.
Even with C99, there are environmental limits for *printf().
The number of characters that can be produced by any single conversion shall be at least 4095. C11dr §7.21.6.1 15
So any code that tries to char buf[10000]; snprintf(buf, sizeof buf, "%s", long_string); risks problems even with a sufficient buf[] yet with strlen(long_string) > 4095.
This implies that a quick and dirty code could count the % and the format length and make the reasonable assumption that the size needed does not exceed:
size_t sz = 4095*percent_count + strlen(format) + 1;
Of course further analysis of the specifiers could lead to a more conservative sz. Continuing down this path we end at writing our own my_vsnprintf().
Even with your own my_vsnprintf() the safety is only so good. There is no run-time check that the format (which may be dynamic) matches the following arguments. To do so requires a new approach.
Cheeky self advertisement for a C99 solution to insure matching specifiers and arguments: Formatted print without the need to specify type matching specifiers using _Generic.
Transferring comments to answer.
The main reason
vsnprintf()was added to C99 was that it is hard to protectvsprintf()or similar. One workaround is to open/dev/null, usevfprintf()to format the data to it, note how big a result was needed, and then decide whether it is safe to proceed. Icky, especially if you open the device on each call.
That means your code might become:
#include <assert.h>
#include <stdarg.h>
#include <stdio.h>
#include <stdlib.h>
extern char *formatString(const char *format, ...);
char *formatString(const char *format, ...)
{
static FILE *fp_null = NULL;
if (fp_null == NULL)
{
fp_null = fopen("/dev/null", "w");
if (fp_null == NULL)
return NULL;
}
va_list ap;
va_start(ap, format);
int size = vfprintf(fp_null, format, ap);
va_end(ap);
if (size < 0)
return NULL;
char *result = (char *) malloc(size + 1);
if (result == NULL)
return NULL;
va_start(ap, format);
int check = vsprintf(result, format, ap);
va_end(ap);
assert(check == size);
return result;
}
int main(void)
{
char *r1 = formatString("%d Dancing Pigs = %4.2f%% of annual GDP (grandiose dancing pigs!)\n",
34241562, 21.2963);
char *r2 = formatString("%s [%-13.10s] %s is %d%% %s\n", "Peripheral",
"sub-atomic hyperdrive", "status", 99, "of normality");
if (r1 != NULL)
printf("r1 = %s", r1);
if (r2 != NULL)
printf("r2 = %s", r2);
free(r1);
free(r2);
return 0;
}
As written with fp_null a static variable inside the function, the file stream cannot be closed. If that's a bother, make it a variable inside the file and provide a function to if (fp_null != NULL) { fclose(fp_null); fp_null = NULL; }.
I'm unapologetically assuming a Unix-like environment with /dev/null; you can translate that to NUL: if you're working on Windows.
Note that the original code in the question did not use va_start() and va_end() twice (unlike this code); that would lead to disaster. In my opinion, it is a good idea to put the va_end() as soon after the va_start() as possible — as shown in this code. Clearly, if your function is itself stepping through the va_list, then there will be a bigger gap than shown here, but when you're simply relaying the variable arguments to another function as here, there should be just the one line in between.
The code compiles cleanly on a Mac running macOS 10.14 Mojave using GCC 8.2.0 (compiled on macOS 10.13 High Sierra) with the command line:
$ gcc -O3 -g -std=c90 -Wall -Wextra -Werror -Wmissing-prototypes \
> -Wstrict-prototypes vsnp37.c -o vsnp37
$
When run, it produces:
r1 = 34241562 Dancing Pigs = 21.30% of annual GDP (grandiose dancing pigs!)
r2 = Peripheral [sub-atomic ] status is 99% of normality