c++ - 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 snprintf, which accepts a buffer size as an argument.

A solution to your second problem is to give a maximum length agument to s[n]printf. For example:

char buffer[128];

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

// 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:

// Behaviour is different in SUSv2; see
// "conforming to" section of man 3 sprintf

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

++length;           // +1 for null terminator
char buffer = malloc(length);

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

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

Answer 2

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 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

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 5

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 6

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.