Is snprintf or vsnprintf better, and how can I ensure I'm using them securely?

Question

I decided to run a static analysis tool on some old code and I found a bunch of places where I'm using sprintf. The tool recommends replacing the calls with either vsnprintf or snprintf because sprintf doesn't do any sort of bounds checking for buffer overflows.

I can easily just do a find and replace on the calls so that it uses snprintf or vsnprintf instead, but I want to make sure there's nothing else that needs to be done in order to make the functionality safe

In some cases the strings being used originate in user input, and in some cases they do not.

Does anybody have any advice on how to do it right?

Answer 1

I can easily just do a find and replace on the calls so that it uses snprintf or vsnprintf instead

No, it's not that easy. Just look at the definition of snprintf or vsnprintf and you will see that they take an extra argument named size that specifies the length of the output buffer. That's what the n in the function name means. To make your code secure, you have to look at every place where you are doing sprintf, figure out the maximum number of bytes that it is safe to write into the output buffer, and pass that number as the size argument to snprintf or vsnprintf.

Unsafe code:

char buffer[10];
sprintf(buffer, "%d %d", x, y);  // UNSAFE if x and y can be large

Equivalent safe code:

char buffer[10];
snprintf(buffer, sizeof(buffer), "%d %d", x, y);

Maybe if all your code fits the above example, then you could do search and replace. But for the cases that are more complicated you will probably have to think about it.