understanding the dangers of sprintf(...)

Question

OWASP says:

"C library functions such as strcpy (), strcat (), sprintf () and vsprintf () operate on null terminated strings and perform no bounds checking."

sprintf writes formatted data to string int sprintf ( char * str, const char * format, ... );

Example:

sprintf(str, "%s", message); // assume declaration and 
                             // initialization of variables

If I understand OWASP's comment, then the dangers of using sprintf are that

1) if message's length > str's length, there's a buffer overflow

and

2) if message does not null-terminate with \0, then message could get copied into str beyond the memory address of message, causing a buffer overflow

Please confirm/deny. Thanks

Answer 1

You're correct on both problems, though they're really both the same problem (which is accessing data beyond the boundaries of an array).

A solution to your first problem is to instead use std::snprintf, which accepts a buffer size as an argument.

A solution to your second problem is to give a maximum length argument to snprintf. For example:

char buffer[128];

std::snprintf(buffer, sizeof(buffer), "This is a %.4s\n", "testGARBAGE DATA");

// std::strcmp(buffer, "This is a test\n") == 0

If you want to store the entire string (e.g. in the case sizeof(buffer) is too small), run snprintf twice:

int length = std::snprintf(nullptr, 0, "This is a %.4s\n", "testGARBAGE DATA");

++length;           // +1 for null terminator
char *buffer = new char[length];

std::snprintf(buffer, length, "This is a %.4s\n", "testGARBAGE DATA");

(You can probably fit this into a function using va or variadic templates.)

Answer 2

Both of your assertions are correct.

There's an additional problem not mentioned. There is no type checking on the parameters. If you mismatch the format string and the parameters, undefined and undesirable behavior could result. For example:

char buf[1024] = {0};
float f = 42.0f;
sprintf(buf, "%s", f);  // `f` isn't a string.  the sun may explode here

This can be particularly nasty to debug.

All of the above lead many C++ developers to the conclusion that you should never use sprintf and its brethren. Indeed, there are facilities you can use to avoid all of the above problems. One, streams, is built right in to the language:

#include <sstream>
#include <string>

// ...

float f = 42.0f;

stringstream ss;
ss << f;
string s = ss.str();

...and another popular choice for those who, like me, still prefer to use sprintf comes from the boost Format libraries:

#include <string>
#include <boost\format.hpp>

// ...

float f = 42.0f;
string s = (boost::format("%1%") %f).str();

Should you adopt the "never use sprintf" mantra? Decide for yourself. There's usually a best tool for the job and depending on what you're doing, sprintf just might be it.

Answer 3

Yes, it is mostly a matter of buffer overflows. However, those are quite serious business nowdays, since buffer overflows are the prime attack vector used by system crackers to circumvent software or system security. If you expose something like this to user input, there's a very good chance you are handing the keys to your program (or even your computer itself) to the crackers.

From OWASP's perspective, let's pretend we are writing a web server, and we use sprintf to parse the input that a browser passes us.

Now let's suppose someone malicious out there passes our web browser a string far larger than will fit in the buffer we chose. His extra data will instead overwrite nearby data. If he makes it large enough, some of his data will get copied over the webserver's instructions rather than its data. Now he can get our webserver to execute his code.

Answer 4

Your 2 numbered conclusions are correct, but incomplete.

There is an additional risk:

char* format = 0;
char buf[128];
sprintf(buf, format, "hello");

Here, format is not NULL-terminated. sprintf() doesn't check that either.

Answer 5

Your interpretation seems to be correct. However, your case #2 isn't really a buffer overflow. It's more of a memory access violation. That's just terminology though, it's still a major problem.

Answer 6

The sprintf function, when used with certain format specifiers, poses two types of security risk: (1) writing memory it shouldn't; (2) reading memory it shouldn't. If snprintf is used with a size parameter that matches the buffer, it won't write anything it shouldn't. Depending upon the parameters, it may still read stuff it shouldn't. Depending upon the operating environment and what else a program is doing, the danger from improper reads may or may not be less severe than that from improper writes.

Answer 7

It is very important to remember that sprintf() adds the ASCII 0 character as string terminator at the end of each string. Therefore, the destination buffer must have at least n+1 bytes (To print the word "HELLO", a 6-byte buffer is required, NOT 5)

In the example below, it may not be obvious, but in the 2-byte destination buffer, the second byte will be overwritten by ASCII 0 character. If only 1 byte was allocated for the buffer, this would cause buffer overrun.

char buf[3] = {'1', '2'};
int n = sprintf(buf, "A");

Also note that the return value of sprintf() does NOT include the null-terminating character. In the example above, 2 bytes were written, but the function returns '1'.

In the example below, the first byte of class member variable 'i' would be partially overwritten by sprintf() (on a 32-bit system).

struct S
{
    char buf[4];
    int i;
};


int main()
{
    struct S s = { };
    s.i = 12345;

    int num = sprintf(s.buf, "ABCD");
    // The value of s.i is NOT 12345 anymore !

    return 0;
}

Answer 8

I pretty much have stated a small example how you could get rid of the buffer size declaration for the sprintf (if you intended to, of course!) and no snprintf envolved ....

Note: This is an APPEND/CONCATENATION example, take a look at here