Integer overflow in glibc strncat | Hacker News Reader

Integer overflow in glibc strncat

sourceware.org

116 pointsanon138538 comments

It was easier to find the source page via a search engine than from the bug reporting details page...

https://sourceware.org/glibc/wiki/GlibcGit

https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/x86...

It's been too long since I've done any assembly programming, but this really doesn't appear to have good documentation for maintainability; or anything resembling sufficient documentation at all.

  /* Inline corresponding strlen file, temporary until new strcpy
  implementation gets merged.  */

git blame reveals this is from March 2013. "Temporary" indeed...

Nothing is more permanent than a temporary solution.

I'm guessing you mean this in jest, but I believe it is literally true.

Temporary code is usually poorly documented, poorly designed, deeply coupled, ugly code. Those are all the exact qualities that make it harder to change than high quality code.

Unless you work very hard, the natural tendency is for all good code (which is correspondingly easy to change) to eventually be replaced with worse code that is harder to replace.

Natural selection applies to code too and, unfortunately, the qualities that lead to longevity are often exactly opposite of the ones that lead to good code.

Those are all factors, but I think there's a deeper one: the same pressure to create a temporary solution also tends to keep it in place. On the other hand, when you are at your leisure to search for an ideal permanent solution, you probably have more time to keep changing it.

I have to agree, from direct experience on some long-lived projects. Code entropy is a surprisingly strong force, and needs a strong technical culture to balance and keep a code base healthy.

Seems to give more credence to: https://www.sudo.ws/todd/papers/strlcpy.html

Not really. The strlcat function is safer because it always NUL terminates the destination buffer, and therefore makes bugs in user code less likely. The flaw here is just an implementation defect.

Also note that SIZE_MAX is an unreasonable input, one would even say it violates preconditions.

No, passing SIZE_MAX does not violate any precondition of strncat(), because preconditions are fixed properties chosen in advance, not something you add retrospectively when you feel like it.

The C standard does not place any upper bound on the value of strncat()'s size argument, so an strncat implementation has to work as described for all large values of the argument. It's a bug of the implementation if it doesn't, not a violated precondition.

(Reasoning about C programs has been my day job for several years. Contrary to what many think, it is possible, it only needs that the rules be set in advance and respected.)

> Also note that SIZE_MAX is an unreasonable input, one would even say it violates preconditions.

Annex K adds rsize_t, RSIZE_MAX, and friends, which restrict inputs to reasonable values. Unfortunately, it doesn't retcon the old functions like strncpy, and worse, no one actually implements it.

No one? I thought Microsoft implemented it, or at least an early version thereof.

A very early version. Among other differences the functions don't care about RSIZE_MAX (even though newer VS versions define and use rsize_t). However the Windows SDK contains <strsafe.h> which does limit the length of strings (STRSAFE_MAX_CCH).

https://msdn.microsoft.com/en-us/library/windows/desktop/ms6...

cool!

What'd be a use case of strncat(s1, s2, SIZE_MAX)? It seems like this would be hard to find in the wild

Commonly, a policy declared that all use of `strcat` must be replaced with `strncat` and at one or more locations in a codebase an actual size bound could not be ascertained without more engineering effort than management was willing to spend.

Ah yes, we are absolutely certain that this new code is incorrect, but it doesn't go against corporate policy.

The new code is precisely equivalent to the old code (except for the bug report linked here).

No use case I can think of, but a credible scenario is an attacker somehow being able to manipulate the size parameter, due to another vulnerability, and then being able to further exploit the overflow.

Quite a few recent practical attacks chained what looked like mostly harmless vulnerabilities taken individually.

In this case I'm skeptical. SIZE_MAX may trigger integer overflow, but smaller values are still potential buffer overflows. It would have to be a rather contrived or pathological case in order for the integer overflow to be exploitable.

That, and programs shouldn't be using strncpy or strncat anyway, at least 99% of the time.

I rarely work with cstrings directly, and even then I don't tend to use them (`strcat`/`strncat`) myself; I tend to use `memcpy`/`memmove` instead. However, I'm curious why do you say they shouldn't be used? Is there any other reason, other than the performance overhead?

Basically, strncpy and strncat shouldn't be used because they don't do what you want. You want a NUL terminated string without buffer overflow, but these functions instead work with fixed size strings. They're simply designed to perform a different task. In particular, they put 0…N NUL bytes at the end, but you usually want exactly one NUL byte, no more, no less.

> In particular, they put 0…N NUL bytes at the end, but you usually want exactly one NUL byte, no more, no less.

strncpy() does that but strncat() does not.

strncat(s1,s2,size) is usually wrong for a different reason: the size argument specifies how many bytes to copy from s2 to the end of s1 (not including the trailing NULL). This means to not buffer overflow your s1 you need to call it like this

  strncat(s1, s2, sizeof(s1) - strlen(s1) - 1);

...Which of course everyone forgets. strlcat() is a nice replacement because it does that calculation for you, so you can just do:

  strlcat(s1, s2, sizeof(s1));

and you won't buffer overflow your s1.

Of course strl... functions are still braindamaged because they still return the length of the original string, regardless of what parameter you specified in the third argument. So if the initial string has lost its null terminator the strl... functions will still core dump even if you specified an otherwise safe size. Also, if you're copying the first few bytes out of an enormous buffer (especially a file mmap) it can take a long time for the function to chew through the rest of the buffer calculating that return value that you don't even care about.

That's why it's usually better to just use memcpy or memmove instead.

In particular, most people end up using memcpy/memmove, snprintf, or a library like strbuf. The BSD devs are somewhat unusual for favoring strlcpy.

Imagine you have a user configurable buffer size. Then imagine that the user wants to throw as much ram as it takes at the problem and sets the maximum size to that value. It happens more often than you'd think.

Are or were there systems where one can/could allocate a buffer of SIZE_MAX bytes? I have a hard time thinking of one, except for, possibly, segmented memory architectures where one wants to keep structures in one segment.

Underflow, aka: strncat(s1,s2,-1)

Yup, I reported the same issue a year ago (http://blog.httrack.com/blog/2014/08/31/i-found-a-bug-in-str...)

I would argue that copying more bytes (as demonstrated) is correct behaviour:

(1.) The function is supposed to append a string to another, not paste a byte range into another one, leaving extra bytes at the end untouched. Thus the functional result in the test code is correct (i.e. contains the concatenated string with for the usual definition of NUL-terminated strings.)

(2.) The strncat man page (and similarly the C99 standard) says: "[...] the size of dest must be at least strlen(dest)+n+1." It does not specify what happens to unused bytes at the end. It would be incorrect if the function would ever touch more than `strlen(dest)+n+1` bytes in dest, but it doesn't. Supplying a proper value for n in the code (e.g. `strlen(s2)`) shows that it wouldn't overrun the buffer.

Thus supplying SIZE_MAX for `n` would be a bad idea (because the function could overwrite random memory locations) and would probably be incorrect in most cases (e.g. on amd64 with SIZE_MAX=2^64-1 it would only be correct for the null pointer.)

Copying less would obviously be incorrect. I couldn't reproduce that though. (The more case even appears on Ubuntu with eglibc on both x86 and amd64.)

Also, this is probably still a bug and not the anticipated behaviour. (As in: whoever wrote that didn't think of those cases and the overflow happens by mistake.)

> The strncat man page (and similarly the C99 standard) says: "[...] the size of dest must be at least strlen(dest)+n+1."

No, the standard does not say that. It (both C99 and C11) says, verbatim: "the maximum number of characters that can end up in the array pointed to by s1 is strlen(s1)+n+1." If the manpage on your system says that, that's a fairly serious deficiency in the manpage.

The standard also says "characters that follow it [a null character] are not appended", so copying extra bytes is explicitly not correct.

Huh, I never use str(n)cat, because it's inefficient by design.

what's inefficient about it?

The typical strcat code you see in the wild:

char buf[100]; buf[0] = '\0'; strcat(buf, "Hello"); strcat(buf, " "); strcat(buf, "World"); strcat(buf, "!\n");

The inefficiency is that each time you do strcat, the existing characters of the string are iterated. So something like a common sequence of building a string by appending little pieces has quadratic inefficiency.

It's unfortunate that strcat & friends don't return a pointer to the new end of the string, or a count of the characters copied. You could use those to make subsequent cats more efficient.

This is a C problem in general. You can create your own data structures and use str.buf + str.len.

It puts zeros to unused bytes in the destination. Here's a note from its man page:

Some programmers consider strncpy() to be inefficient and error prone. If the programmer knows (i.e., includes code to test!) that the size of dest is greater than the length of src, then strcpy() can be used.

One valid (and intended) use of strncpy() is to copy a C string to a fixed-length buffer while ensuring both that the buffer is not overflowed and that unused bytes in the target buffer are zeroed out.

You're talking about strncpy not strncat.

Ah sorry, I was misreading it from the beginning :(

Reply on news.ycombinator.com