tech-security archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: strscpy



Le 30/05/2020 à 19:27, Robert Elz a écrit :
     Date:        Sat, 30 May 2020 15:41:39 +0000
     From:        Taylor R Campbell <campbell+netbsd-tech-security%mumble.net@localhost>
     Message-ID:  <20200530154159.0B34860AFB%jupiter.mumble.net@localhost>

   | Of course, if strwhatevercpy is _guaranteed_ to return the length of
   | the output string,

That wasn't the intent of my suggestion, it would still be "length or error"
just the error would be a length value, rather than -1.

So, unless it is guaranteed that sizeof(dst) > strlen(src1)
(perhaps because dst isn't an array, but is malloc'd to be big enough)
this would be the correct idiom:

   | 	n = strlcpy(dst, src1, sizeof(dst));
   | 	if (n >= sizeof(dst))
   | 		goto fail;
   | 	strlcpy(dst + n, src2, sizeof(dst) - n);

using whatever name is finally decided upon.

   | - Returning size (or size+1) might be safer than strlcpy.  But it has
   |   the (to my mind) counterintuitive property that the result is
   |   sometimes the length _without_ the NUL terminator and the result is
   |   sometimes the size _with_ the NUL terminator (or with the NUL
   |   terminator and one more, for size+1).

You don't want to think of the size+1 (or SIZE_MAX or whatever) as being
the length of anything - it is rather an indicator that truncation happened.

Indeed. The first idiom quoted by Taylor is by definition not correct for
this function, and I wouldn't expect callers to be this wrong when checking
for errors.

   | - Using SIZE_MAX avoids the casts or ssize_t needed by -1, and means
   |   if you try to use the length in the truncation case, you will almost
   |   certainly cause a fault that will abort the application or panic the
   |   kernel -- in contrast to a one- or two-byte buffer overrun of size
   |   or size+1 which is much less likely to be noticed.

Except that SIZE_MAX *is* -1 (or (unsigned)-1) so when added to something
it effectively just decrements by 1.

Indeed. SIZE_MAX is more dangerous than size+1, because while we do have some
protections against overflows in the kernel, we have no protection against
underflows.

Overall, a good magic would be half the address space size. In general, that
means SSIZE_MAX.

That said, either way would work (size+1 or SIZE_MAX ... personally I
prefer not just size - even though that should never be returned for a
valid copy) ... but upon reflection, size+1 might not be the best choice,
as it could overflow (possibly).

size+1 overflowing would mean a copy of the full address space was made, and
this will page fault long before strwhatevercpy got the opportunity to
return. As well the pointers inside the function will have wrapped. So I
would eject this concern.

Maxime


Home | Main Index | Thread Index | Old Index