On Thu, Nov 09, 2023 at 13:23:14 +0100, Alejandro Colomar wrote:
... snip ...
> > > > For the sake of reference, I looked into a few big C and C++ projects to
> > > > see how often a strncpy(3)-based snippet was used to produce a truncated
> > > > copy. I found 18 instances in glibc 2.38, 2 in util-linux 2.39.2 (in spite
> > > > of its custom xstrncpy() function), 61 in GNU binutils 2.41, 43 in
> > > > GDB 13.2, 1 in LLVM 17.0.4, 7 in CPython 3.12.0, 99 in OpenJDK 22+22,
> > > > 10 in .NET Runtime 7.0.13, 3 in V8 12.1.82, and 86 in Firefox 120.0. (Note
> > > > that I haven't filtered out vendored dependencies, so there's a little bit
> > > > of double-counting.) It seems like most codebases that don't ban strncpy(3)
> > > > use a derived snippet somewhere or another. Also, I found 3 instances in
> > > > glibc 2.38 and 5 instances in Firefox 120.0 of detecting truncation by
> > > > checking the last character.
> > >
> > > I know. I've been rewriting the code handling strings in shadow-utils
> > > for the last year, and ther was a lot of it. I fixed several small bugs
> > > in the process, so I recommend avoiding it.
> >
> > I can't tell you about your own experience, but in mine, the root cause of
> > most string-handling bugs has been excessive cleverness in using the
> > standard string functions, rather than the behavior of the functions
> > themselves. So one worry of mine is that if strncpy(3) ends up being
> > deprecated or whatever, then authors of portable libraries will start
> > writing lots of custom memcpy(3)-based replacements to their strncpy(3)-
> > based snippets, and more lines of code will introduce more opportunities
> > for cleverness.
>
> Don't worry. strncpy(3) won't be deprecated, thanks to tar(1). ;)
>
Just please don't tar and feather [1] the people who use it ;)
... snip ...
> > > > the code to understand the concept behind how these two snippets work, that
> > > > the only difference between the strncpy(3)'s special "character sequence"
> > > > and an ordinary C string is an additional null terminator at the end of the
> > > > destination buffer.
> > >
> > > This is part of string_copying(7):
> > >
> > > DESCRIPTION
> > > Terms (and abbreviations)
> > > string (str)
> > > is a sequence of zero or more non‐null characters followed by a
> > > null byte.
> > >
> > > character sequence
> > > is a sequence of zero or more non‐null characters. A program
> > > should never use a character sequence where a string is required.
> > > However, with appropriate care, a string can be used in the place
> > > of a character sequence.
> > >
> > > I think that is very explicit in the difference. strncpy(3) refers to
> > > that page for understanding the differences, so I think it is
> > > documented.
> > >
> > > strncpy(3):
> > > CAVEATS
> > > The name of these functions is confusing. These functions produce a
> > > null‐padded character sequence, not a string (see string_copying(7)).
> >
> > My point is isn't that the difference is undocumented, but that the typical
> > man page reader isn't reading the man pages for their own sake, but because
> > they're looking at some code, and they want to Know What It's Doing as soon
> > as possible.
>
> We could maybe add a list of ways people have tried to be clever with
> strncpy(3) in the past and failed, and then explain why those uses are
> broken. This could be in a BUGS section.
>
This would be a very fun read.
... snip ...
> > > Also, I've seen a lot of off-by-one bugs in calls to strncpy(3), so no,
> > > it's not correct code. It's rather dangerous code that just happens to
> > > not be vulnerable most of the time.
> >
> > So will all the custom strlen(3)+memcpy(3)-based replacements suddenly be
> > immune to off-by-one bugs?
>
> Slightly. Here's the typical use of strlen(3)+strcpy(3):
>
> if (strlen(src) >= dsize)
> goto error;
> strcpy(dst, src);
>
> There's no +1 or -1 in that code, so it's hard to make an off-by-one
> mistake. Okay, you may have seen that it has a '>=', which one could
> accidentally replace by a '>', causing an off-by-one. I'd wrap that
> thing in a strxcpy() wrapper so you avoid repetition.
>
Might I go so far as to recommend strnlen(3) instead of strlen(3)? That
way, instead of blindly looking for a null terminator, you stop after a
predetermined max length. Especially nice for untrusted input where you
can't make assumptions on the "fitness for a purpose" of what's being
fed in.
if (src == NULL || strnlen(src, dsize) == dsize)
goto error;
strcpy(dst, src);
This, of course, assumes you have POSIX at your disposal.
I'm writing this before going to bed. I did briefly sanity check it with
a simple test prog, but it would be quite ironic if I missed something
wouldn't it...
- Oskari
[1]: https://en.wikipedia.org/wiki/Tarring_and_feathering
Attachment:
signature.asc
Description: PGP signature
