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

You assessment is correct, except that the springf will put 28 characters in the buffer counting the end-of-string NUL at the end (That's why you needed the misplaced "+1" in the first place)

Note that in my experiences, if something fails outside of a debugger, but works with stepping through in the debugger, in 100% of the time, you've overrun a local buffer. Debuggers push a lot more onto the stack, so it's less likely the something important was overwritten.

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

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.

Answer 4

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 5

You are correct that pointer arithmetic in this example would produce an incorrect (shorter) length passed to new. The most probable reason why you are not able to make this crash is because there is some uncertainty as to how much buffer space is actually provided by the memory allocation.

The library is allowed to provide a larger buffer than was requested. Furthermore, it is also possible that whatever follows your buffer is prefixed by an allocation header that is subject to machine word alignment rules. This means there could be up to three padding bytes (depending on platform) before the very next allocation header.

Even if you overwrote the next allocation header (which is used to manage the allocated memory blocks) it would not manifest itself as a problem until the owner of that next block attempted to return it to the heap.

Answer 6

Many historic malloc implementations put bookkeeping data immediately before and/or after the allocated block. It's possible that you're overwriting such data, in which case you would not see any error/crash until you try to free the memory (or perhaps free whatever the next block happens to be). Likewise, it's possible that the bookkeeping information for a subsequent allocation will later overwrite your string.

I suspect modern malloc implementations make some effort to protect against heap corruption by padding allocations with integrity-check data, so if you're lucky, nothing bad will happen or you might get a warning message during a later allocation/free operation.

Answer 7

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 8

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 9

I tried it with heap allocations, variables are not continuous in memory in this case. That is why it is hard to make buffer overflow in this case.

Buy try it with stack overflow

#include "stdio.h"
#include "string.h"

int main()
{
     unsigned int  y      = (0xFFFFFFFF);
     char buffer[strlen("This string is 27 char long" + 1)];
      unsigned int  x      = (0xFFFFFFFF);
      sprintf(buffer, "This string is 27 char long");

      printf("X (%#x) is %#x, Y (%#x) is %#x, buffer '%s' (%#x) \n", &x, x,&y, y, buffer, buffer);
      return 0;
  }

You will see that Y is corrupted.

Answer 10

The reason the string is printing fine in the debugger is that as part of the sprintf, the trailing NULL character is being written to memory (in this case beyond the buffer you allocated) and when it comes to reading the string the NULL character is present to terminate the string as expected.

The problem is that the byte containing the NULL character hasn't been allocated as part of the original new and so could be used for a different allocation later. In this case, when you come to read the string afterwards you will likely get your original string with garbage appended.

Answer 11

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.