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.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
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.cWhich 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
(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