understanding the dangers of sprintf(...)

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

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

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.

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.

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.

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.