Re: [BUG]Buffer Overflow in string library

[Matthew Daley <mattjd@gmail.com>]

(re-adding xen-devel; if you meant to drop it, you need to say so explicitly.)

On Mon, Sep 16, 2013 at 8:00 AM, Steve Calandra
<steven.calandra@gmail.com> wrote:
> Hey, thanks for the reply.  In addition to helping the project, this also
> forwards my thesis research, so thanks.

Cool.

>
>>Which string.c? There are multiple, but I'm guessing xen/common/string.c.
>
> Yes, xen/common/string.c (Sorry, I didn't realize there was more than one.)
>
>
>>I can't see this (broken?) line in any of Xen's source...?
>
> It's at line 60 in xen/common/string.c.  Not sure why you can't find it?  I
> pulled the latest to make sure it wasn't removed in the time that I
> submitted this and you looked at, but it's still there.

http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/common/string.c;h=9a5a4ba5e8a89e3493bd019699e652401a0cf7d2;hb=HEAD#l60

That's the current source of xen/common/string.c at line 60 in master.
I still can't see it... (note that I'm only referring to the
>     size_t destLen = strLen(dest);
line, which declares an unused variable, as well as uses "strLen"
instead of "strlen" (note the case.)

>>Well, 'size' only needs to be bigger than the 'dest' buffer size to
>>cause a write overflow, but that's moot anyway; strlcpy is a
>>well-known function provided by many C standard libraries, and it
>>provides no claims as to the safety of calling it with a 'size' bigger
>>than the 'dest' buffer size.
>
> Right, as far as an overflow goes, it only needs to be bigger than dest.  I
> specified both
> because the ternary statement seems to handle the case when size > src, but
> not dest.

Yes. The safety strlcpy provides over strcpy (vs. strncpy) is that it
always NULL-terminates 'dest' (assuming non-zero 'size'). It still
doesn't provide any safety over overflowing 'dest' if 'size' is wrong.

> I hadn't noticed the #ifndef around the function, so I thought this was
> implemented as an alternative to the standard C function.  I guess that
> makes this less of a problem.

Right. Note that OpenBSD's version of the function has the same
non-issue: http://www.openbsd.org/cgi-bin/cvsweb/src/lib/libc/string/strlcpy.c?rev=1.11

If there was, however, a *user* of strlcpy that specified a 'size'
larger than that of 'dest', *that* would be an issue. As an aside, any
security problems should be reported privately to
security@xenproject.org, and not xen-devel. They'll look at anything
reported with a community-formed (and IMO sensible) process.

- Matthew