On 1/29/19 3:35 PM, Theo de Raadt wrote:
> Martijn van Duren <openbsd+t...@list.imperialat.at> wrote:
>
>> I think the others have given a decent explanation, but I would like to
>> elaborate a little furter.
>
> I strongly agree with your points, but a few comments.
>
>> OK, so strscpy won't read any more data from src than count bytes, which
>> could save you from reading from the end of the world, where strlcpy
>> does assume that src is a NUL-terminated string.
>
> In C, a string is a NUL-terminated byte sequence.
>
> If it isn't NUL-terminated, then it isn't a string.
>
> The language feature "char []" does this, and the string-function subset
> of the language RELY upon this. strlcpy follows that same pattern.
>
> We don't need to say "C string", it is clear enough to say "string".
>
>> src is clearly not a C-string, since it's not NUL-terminated, but
>> there's no problem, since strscpy doesn't throw us over the edge of the
>> world. Right?
>
> Oh my god, strlen also fails.
>
> Because the object isn't a string.
>
> That much is clear.
>
>> The problem is that you don't know what comes after src. It could be
>> sensitive information that's now being made available to an attacker, it
>> could introduce byte-sequences that completely blow up your terminal.
>> You just can't tell for sure. With strlcpy chances are greater that you
>> hit a page that's not mapped readable and you generate a SEGFAULT.
>> Like Joel said in his talk, it's better to crash and fix the bug.
>
> Indeed. Most OpenBSD security changes have focused on converting subtle
> failure conditions into quick & hard failures, rather than allowing code
> to continue running after creating the first instance of damage.
>
>> Note that I'm not familiar enough with the kernel to make any statement
>> on whether on not things work identically there.
>
> Same thing in the kernel. An aggressive over-access is likely to run
> into an unmapped page of memory, and kaboom you crash, which is good because
> now you know of a bug you didn't know previously. (1) the bug is less easily
> be exploited by an attacker since the common case crashes instead of leaving
> a damage artifact in memory, and (2) you know before the attacker, and (3)
> it is a priority for repair.
>
>> If you want to be 100% secure you'd need to keep track of both the src
>> length (note that's the length of the string and not of the allocated
>> memory) and the destination buffer size and built a set of string
>> manipulation functions around that. But that would be a lot more tedious
>> to use and one could just as easily overlook to change the length
>> attribute upon change, which brings you back to square one.
>
> The strlcpy paper described this as a design decision. Easy for people
> to incorporate the overflow-protection.
>
> Overflow-detection and handling is another problem. strlcpy also makes
> this easy, but a glance at our whole source tree demonstrates how
> un-urgent the extended software development community takes that
> problem, meaning we seldom receive diffs to add checks.
>
> As to strlcpy walking the whole string and determining the required
> length, that's the same behaviour as snprintf. So strlcpy is mergely
> copying the interface of an existing function in the standard.
>
> The enemy of the good is the perfect, and that enemy foams at the mouth
> about imperfection and then turns around and doesn't fix any software.
>
>> I'm not certain what the author tries to say with "the implementation
>> is robust to the string changing out from underneath it", so I won't
>> comment on that part.
>
> It must be grabbing the big string lock.
>
I did a bit of digging for shits and giggles and found the following:
The Linux kernel does contain a version of strlcpy, but it is a bit
different from our strlcpy:
/**
* strlcpy - Copy a C-string into a sized buffer
* @dest: Where to copy the string to
* @src: Where to copy the string from
* @size: size of destination buffer
*
* Compatible with ``*BSD``: the result is always a valid
* NUL-terminated string that fits in the buffer (unless,
* of course, the buffer size is zero). It does not pad
* out the result like strncpy() does.
*/
size_t strlcpy(char *dest, const char *src, size_t size)
{
size_t ret = strlen(src);
if (size) {
size_t len = (ret >= size) ? size - 1 : ret;
memcpy(dest, src, len);
dest[len] = '\0';
}
return ret;
}
So Linux first does a whole walk over the string to find the length
and then does a full copy via memcpy.
Then we have strscpy[0], which walks src one word (long) at a time and
checks for NUL as it goes along. I can't find any locking, nor can I
ascribe any other security measure to it compared to our strlcpy.
>From this I can only guess that their statement that their strlcpy is
less secure is based around TOCTOU. But I can't see how changing your
string in the middle of any operation is going to end well.
As for the walking src one word at a time, I put their 64 bit little
endian implementation in a test-program on my AMD64 to see if there's a
speed-bonus from doing it that way, but on my system the performance was
about a factor 2 slower compared to our libc strlcpy. So I have no clue
why they build this elaborate set of functions. YMMV.
So if my findings and interpretations above are correct all statements
about how strscpy is safer then strlcpy come from their own
implementation and people stating that our strlcpy is less secure than
strscpy are spreading FUD.
[0] https://github.com/torvalds/linux/blob/master/lib/string.c#L179
