Re: [BUG]Buffer Overflow in string library

Re: [BUG]Buffer Overflow in string library

[Matthew Daley]

(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

[BUG]Buffer Overflow in string library

[Steve Calandra]

[-- Attachment #1.1: Type: text/plain, Size: 651 bytes --]

There is a potential, though unlikely buffer overflow vulnerability in the
function strlcpy() in string.c

size_t strlcpy(char *dest, const char *src, size_t size)
{
    size_t ret = strlen(src);
    size_t destLen = strLen(dest);
    if (size) {
        size_t len = (ret >= size) ? size-1 : ret;
        memcpy(dest, src, len);
        dest[len] = '\0';
    }
    return ret;
}

In the event that size is greater than the length of src and dest, dest
will be overflowed.  This can be fixed with the following:

if (len >= strlen(dest))
     len = strlen(dest) -1;

I tried fixing it myself, but I was having problems pushing the change to
the repo.

[-- Attachment #1.2: Type: text/html, Size: 850 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [BUG]Buffer Overflow in string library

[Matthew Daley]

On Sat, Sep 14, 2013 at 9:33 AM, Steve Calandra
<steven.calandra@gmail.com> wrote:
> There is a potential, though unlikely buffer overflow vulnerability in the
> function strlcpy() in string.c

Which string.c? There are multiple, but I'm guessing xen/common/string.c.

>
> size_t strlcpy(char *dest, const char *src, size_t size)
> {
>     size_t ret = strlen(src);
>     size_t destLen = strLen(dest);

I can't see this (broken?) line in any of Xen's source...?

>
>     if (size) {
>         size_t len = (ret >= size) ? size-1 : ret;
>         memcpy(dest, src, len);
>         dest[len] = '\0';
>     }
>     return ret;
> }
>
> In the event that size is greater than the length of src and dest, dest will
> be overflowed.  This can be fixed with the following:
>
> if (len >= strlen(dest))
>      len = strlen(dest) -1;

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.

See, for example,
http://www.openbsd.org/cgi-bin/man.cgi?query=strlcpy&sektion=3 . The
version in xen/common/string.c is for when it's not provided by the
system C library (ie. with glibc), that's why it's wrapped in '#ifndef
__HAVE_ARCH_STRLCPY'.

Also, using strlen(dest) wouldn't work as there is no requirement for
'dest' to already be a valid string, only a valid pointer to a
writeable buffer of at least size 'size'. Perhaps you've confused
strlcpy with strlcat?

>
> I tried fixing it myself, but I was having problems pushing the change to
> the repo.

Only committers can push (and things go through osstest first anyway),
assuming you're talking about a repo on xenbits.xen.org.

- Matthew