11 ms·
Integer overflow in glibc strncat
- martin_ 11y agoWhat'd be a use case of strncat(s1, s2, SIZE_MAX)? It seems like this would be hard to find in the wild
- mjevans 11y agoImagine 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.
- Someone 11y agoAre 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.
- stephencanon 11y agoCommonly, 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.
- dietrichepp 11y agoAh yes, we are absolutely certain that this new code is incorrect, but it doesn't go against corporate policy.
- stephencanon 11y agoThe new code is precisely equivalent to the old code (except for the bug report linked here).
- brohee 11y agoNo 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.
- dietrichepp 11y agoIn 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.
- spoiler 11y agoI 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?
- dietrichepp 11y agoBasically, 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.
- __david__ 11y ago> 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.
- nly 11y agoUnderflow, aka: strncat(s1,s2,-1)
- mjevans 11y agoIt 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/glibc/wiki/GlibcGit https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/x86_64/multiarch/strncat-sse2-unaligned.S https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/x86... https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/x86_64/multiarch/strcat-sse2-unaligned.S 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.
- Kristine1975 11y ago/* Inline corresponding strlen file, temporary until new strcpy implementation gets merged. */ git blame reveals this is from March 2013. "Temporary" indeed...
- gnarbarian 11y agoNothing is more permanent than a temporary solution.
- munificent 11y agoI'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.
- btrask 11y agoThose 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.
- ambrop7 11y agoHuh, I never use str(n)cat, because it's inefficient by design.
- halayli 11y agowhat's inefficient about it?
- sepeth 11y agoIt 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.
- ambrop7 11y agoThe 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.
- halayli 11y ago
- nomadlogic 11y agoSeems to give more credence to: https://www.sudo.ws/todd/papers/strlcpy.html https://www.sudo.ws/todd/papers/strlcpy.html
- dietrichepp 11y agoNot 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.
- jmgao 11y ago> 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.
- klodolph 11y agoNo one? I thought Microsoft implemented it, or at least an early version thereof.
- cremno 11y agoA 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/ms647466%28v=vs.85%29.aspx https://msdn.microsoft.com/en-us/library/windows/desktop/ms6...
- pascal_cuoq 11y agoNo, 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.)
- xroche 11y agoYup, I reported the same issue a year ago (http://blog.httrack.com/blog/2014/08/31/i-found-a-bug-in-strncat/ http://blog.httrack.com/blog/2014/08/31/i-found-a-bug-in-str...)
- __michaelg 11y agoI 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.)
- stephencanon 11y ago> 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.