Consequences of this buffer overflow?

Question

So here I believe I have a small buffer overflow problem I found when reviewing someone else's code. It immediately struck me as incorrect, and potentially dangerous, but admittedly I couldn't explain the ACTUAL consequences of this "mistake", if any.

I had written up a test app to demonstrate the error, but found (to my dismay) that it seems to run correctly regardless of the overflow. I want to believe that this is just by chance, but wanted some feedback to determine if my thinking were wrong, or if there truly is a problem here that just isn't showing its head in my test app.

The problem code (I think it is, anyway):

char* buffer = new char[strlen("This string is 27 char long" + 1)];
sprintf(buffer, "This string is 27 char long");

Now, the reason this stood out to me and I want to flag it as a possible buffer overflow is because of the first strlen. Due to pointer arithmetic, the 'incorrect' placement of the + 1 will cause the strlen to return 26 instead of 27 (taking the length of "his string is 27 char long"). sprintf, I believe, then prints 27 char into the buffer and has caused a buffer overflow.

Is that a correct assessment?

I wrote a test app to demonstrate this for the person who's code I was looking at, and found that even in the debugger the string will print correctly. I also attempting putting other variables on the stack and heap before and after this code to see if I could affect neighboring areas of memory, but was still receiving correct output. I realize that my newly allocated heap memory might not be adjacent, which would explain the lack of useful overflow, but I just really wanted to confirm with others' opinions if this is in fact an issue.

Since this is a pretty simple "question", it'd be nice if you could support your answer with some sort of reference as well. While I value and welcome your input, I'm not going to accept "yes it is" as the final answer. Thank you kindly in advance.

---

Update: Many good answers with a lot of additional insight. Unfortunately, I can't accept them all. Thank you for sharing your knowledge and for being my 'second opinion'. I appreciate the help.

Answer 1

Correct statement. Since you are passing address of the second character of the string to strlen(), you are getting the length one character less as a result. Aside from that, the main problem is with sprintf(), that's one of the reasons that it's not safe.

Even this compiles and executes (may also crash).

    char* x = new char;
    sprintf(x, "This is way longer than one character");
    printf("%s", x);

In order to avoid this dangerous issue, you should use safe versions of this function like snprintf() or asprintf() under GCC or sprintf_s() under MSVC.

As references, please have a look at The GNU C Library documentation in this regard and also security note of MSDN's sprintf() article.

Answer 2

Your assessment is correct. [edit] with the addition of the correction mentioned by James Curran.[/edit]

Likely, your test app didn't show the problem because the allocation is rounded up to the next multiple of 4, 8 or 16 (which are common allocation granularities).

This means you should be able to demonstrate with a 31 character long string.

Alternatively, use an "instrumenting" native memory profiler that can place guard bytes closely around such an allocation.

Answer 3

As stated by others, you are completely correct in assuming that this is no good, and the reason you don't see this is padding. Try valgrind on this, this should definitively find that error.

Answer 4

Your real problem is that you're writing

char* buffer = new char[strlen("This string is 27 char long" + 1)];

instead of

char* buffer = new char[strlen("This string is 27 char long") + 1];

Meaning that on the first one you're giving strlen() an address which isn't the beginning of your string.

Try this code:

const char szText[] = "This string is 27 char long";
char* buffer = new char[strlen(szText) + 1];
sprintf(buffer, szText);

Answer 5

The problem is that you are writing somewhere in the memory, but not on the stack. Therefore, it's hard to actually see what's wrong. If you want to see the damages, try allocating the string on the stack

char buffer[strlen("This string is 27 char long" + 1)];

and the write past it. Other variables will be written, you can also add some code to be executed if you really know how the binary works.

To exploit a buffer overflow like that, you need to write the data you want, then find a way to "jump" to this data to be executed.

Answer 6

Yes, you are correct. The buffer allocated will be 2 bytes too small to hold the string.

Since this is being allocated on the heap, it would be possible for this to result in a heap corruption. However, the liklihood of that depends on the what other allocations and releases of memory have occurred prior to this point and also on heap manager being used. See Heap Overflow for more.