Re: [PATCH 2.6]: IPv6: strcpy -> strlcpy

On Fri, Nov 28, 2003 at 04:54:13AM +0900, YOSHIFUJI Hideaki / ?$B5HF#1QL@?(B wrote:

In article [off-list ref] (at Thu, 27 Nov 2003 19:46:02 +0000), Russell King [off-list ref] says:

quoted

I'm slightly cautious here, although I haven't read the patch yet.
Did anyone consider whether any of these structures were copied to
user space, and whether, as a result of this change, we're now
copying uninitialised data to users?

I believe that it, to change from strcpy() to strlcpy(), just 
eliminates possibility of buffer-overrun.

While this is 100% correct, the bit which raised my attention was the
original message which didn't seem to show that the above had been
considered.

The thing that worries me is that an incorrect strlcpy() conversion
gives the impression that someone has thought about buffer underruns
as well as overruns, and, unless someone /has/ actually thought about
it, there could well still be a security problem lurking there.

I'm just overly wary of all strlcpy() conversions.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core

In article [off-list ref] (at Thu, 27 Nov 2003 20:00:41 +0000), Russell King [off-list ref] says:

The thing that worries me is that an incorrect strlcpy() conversion
gives the impression that someone has thought about buffer underruns
as well as overruns, and, unless someone /has/ actually thought about
it, there could well still be a security problem lurking there.

Hmm, what do you actually mean by "buffer underruns?"

(If I'm correct) do you suggest that we should zero-out rest of 
destination buffer?

if so, we may want to have a function, say strlcpy0(), like this:

size_t strlcpy0(char *dst, const char *src, size_t maxlen)
{
  size_t len = strlcpy(dst, src, maxlen);
  if (maxlen && len < maxlen - 1)
    memset(dst + len + 1, 0, maxlen - len - 1);
  return len;
}

--yoshfuji

On Thu, 2003-11-27 at 21:00, Russell King wrote:

quoted

I believe that it, to change from strcpy() to strlcpy(), just 
eliminates possibility of buffer-overrun.

While this is 100% correct, the bit which raised my attention was the
original message which didn't seem to show that the above had been
considered.

Well, I can't see the difference between using strcpy() and strlcpy().
Let be:

char destination[MAX];
char * source;
N = strlen(source);

We could use strlcpy(destination, source, MAX) or strcpy(destination,
source).

We have the following scenarios:

- N < MAX. In this case, both strcpy() and strlcpy() should yield the
same results. No buffer overflows. If the source strings does not
already contain uninitialised data, there's no way for strlcpy() to copy
them.

- N >= MAX. In this case, strlcpy() will copy less bytes than strcpy().
To be exact, strlcpy() will copy N-MAX+1 bytes less than strcpy().
Again, no buffer overflows. Also, it's still impossible to copy
uninitialised data since we just stop at \0 or when we fill up the
destination buffer.

So I don't see how using strlcpy() could copy uninitialised data from
kernel space to user space. If we used memcpy() we could end up copying
uninitialised data, but I can't see how using strlcpy() would do that.

In general terms, strlcpy() will copy *at most* the same number of bytes
as strcpy(), but there is no single case when strlcpy() will copy more
bytes than strcpy().

Can someone throw some light on this?