[libvirt] [PATCH] Fix latent buffer overflow in qemudOpenMonitorUnix.
Fix a possible latent bug in qemudOpenMonitorUnix(). If the pathname to the monitor is very long (i.e. >= UNIX_MAX_PATH), then strncpy will *not* place a final \0 on the string (see "man strncpy"). NULL terminate the buffer to ensure we don't run off the end. Signed-off-by: Chris Lalancette <clalance@redhat.com> --- src/qemu_driver.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 9fcc07a..4f173b7 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -910,6 +910,7 @@ qemudOpenMonitorUnix(virConnectPtr conn, memset(&addr, 0, sizeof(addr)); addr.sun_family = AF_UNIX; strncpy(addr.sun_path, monitor, sizeof(addr.sun_path)); + NUL_TERMINATE(addr.sun_path); do { ret = connect(monfd, (struct sockaddr *) &addr, sizeof(addr)); -- 1.6.0.6
On Fri, 2009-07-31 at 15:20 +0200, Chris Lalancette wrote:
Fix a possible latent bug in qemudOpenMonitorUnix(). If the pathname to the monitor is very long (i.e. >= UNIX_MAX_PATH), then strncpy will *not* place a final \0 on the string (see "man strncpy"). NULL terminate the buffer to ensure we don't run off the end.
Signed-off-by: Chris Lalancette <clalance@redhat.com> --- src/qemu_driver.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 9fcc07a..4f173b7 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -910,6 +910,7 @@ qemudOpenMonitorUnix(virConnectPtr conn, memset(&addr, 0, sizeof(addr)); addr.sun_family = AF_UNIX; strncpy(addr.sun_path, monitor, sizeof(addr.sun_path)); + NUL_TERMINATE(addr.sun_path);
Good catch, ACK Thanks, Mark.
On Fri, Jul 31, 2009 at 03:20:09PM +0200, Chris Lalancette wrote:
Fix a possible latent bug in qemudOpenMonitorUnix(). If the pathname to the monitor is very long (i.e. >= UNIX_MAX_PATH), then strncpy will *not* place a final \0 on the string (see "man strncpy"). NULL terminate the buffer to ensure we don't run off the end.
Signed-off-by: Chris Lalancette <clalance@redhat.com> --- src/qemu_driver.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 9fcc07a..4f173b7 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -910,6 +910,7 @@ qemudOpenMonitorUnix(virConnectPtr conn, memset(&addr, 0, sizeof(addr)); addr.sun_family = AF_UNIX; strncpy(addr.sun_path, monitor, sizeof(addr.sun_path)); + NUL_TERMINATE(addr.sun_path);
do { ret = connect(monfd, (struct sockaddr *) &addr, sizeof(addr));
ACK, but this mistake is made in pretty much every use of unix domain sockets :-( Could you fix the others here too. Clearly strncpy() is just unfit for purpose, and we should plan to banish it in favour of a virStrncpy impl that guarnetees to add the trailing \0. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Daniel P. Berrange wrote:
On Fri, Jul 31, 2009 at 03:20:09PM +0200, Chris Lalancette wrote:
Fix a possible latent bug in qemudOpenMonitorUnix(). If the pathname to the monitor is very long (i.e. >= UNIX_MAX_PATH), then strncpy will *not* place a final \0 on the string (see "man strncpy"). NULL terminate the buffer to ensure we don't run off the end.
Signed-off-by: Chris Lalancette <clalance@redhat.com> --- src/qemu_driver.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 9fcc07a..4f173b7 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -910,6 +910,7 @@ qemudOpenMonitorUnix(virConnectPtr conn, memset(&addr, 0, sizeof(addr)); addr.sun_family = AF_UNIX; strncpy(addr.sun_path, monitor, sizeof(addr.sun_path)); + NUL_TERMINATE(addr.sun_path);
do { ret = connect(monfd, (struct sockaddr *) &addr, sizeof(addr));
ACK, but this mistake is made in pretty much every use of unix domain sockets :-( Could you fix the others here too.
Clearly strncpy() is just unfit for purpose, and we should plan to banish it in favour of a virStrncpy impl that guarnetees to add the trailing \0.
Yeah, true. Despite the ACKs from you and markmc, I decided not to commit this yet. It's quite a theoretical bug (you'd have to have /var/very/long/non-standard/paths/to/libvirt), so I don't think it's absolutely critical right now. I'll convert the rest of the users, and possible come up with a make syntax-check rule to prevent against future use. -- Chris Lalancette
Chris Lalancette wrote:
Clearly strncpy() is just unfit for purpose, and we should plan to banish it in favour of a virStrncpy impl that guarnetees to add the trailing \0.
Yeah, true. Despite the ACKs from you and markmc, I decided not to commit this yet. It's quite a theoretical bug (you'd have to have /var/very/long/non-standard/paths/to/libvirt), so I don't think it's absolutely critical right now. I'll convert the rest of the users, and possible come up with a make syntax-check rule to prevent against future use.
Sigh. Unfortunately, this is a difficult problem. I did some reading, and found that besides having the NULL termination problem, strncpy() is also very slow, so should be avoided. I then looked at doing an implementation of strlcpy(), but while it would prevent the buffer overflow, it can truncate the resulting string. This could lead to difficult detect errors later on. Note that both my original proposed patch for this problem plus many of current in-tree users of strncpy() suffer from this problem as well. Therefore, I think the way to go is probably to have a safe version of strcpy() which checks that the length of destination is sufficient to hold all of the source plus the \0. If not, it should return an error, and then the caller can decide what to do. Something like this: /** * virStrcpy * * A safe version of strcpy. The last parameter is the number of bytes * available in the destination string, *not* the number of bytes you want * to copy. If the destination is not large enough to hold all of the * src string, NULL is returned and no data is copied. If the destination * is large enough to hold all of the src, then the string is copied and * a pointer to the destination string is returned. */ char * virStrcpy(char *dest, const char *src, size_t destbytes) { int len; len = strlen(src) + 1; if (len > destbytes) return NULL; return memcpy(dest, src, len); } And usage would be: char foo[5]; virStrcpy(foo, "hello", sizeof(foo)); /* returns an error, foo is not large enough for hello\0 */ char bar[6]; virStrcpy(bar, "hello", sizeof(foo)); /* succeeds, foo is large enough for hello\0 */ A brief look through the current usage of strncpy() in libvirt shows that this would be a drop in replacement for most of the current users of strncpy(), and be safer as well. What do you think? -- Chris Lalancette
On Sun, Aug 02, 2009 at 12:15:47PM +0200, Chris Lalancette wrote:
Daniel P. Berrange wrote:
On Fri, Jul 31, 2009 at 03:20:09PM +0200, Chris Lalancette wrote:
Fix a possible latent bug in qemudOpenMonitorUnix(). If the pathname to the monitor is very long (i.e. >= UNIX_MAX_PATH), then strncpy will *not* place a final \0 on the string (see "man strncpy"). NULL terminate the buffer to ensure we don't run off the end.
Signed-off-by: Chris Lalancette <clalance@redhat.com> --- src/qemu_driver.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 9fcc07a..4f173b7 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -910,6 +910,7 @@ qemudOpenMonitorUnix(virConnectPtr conn, memset(&addr, 0, sizeof(addr)); addr.sun_family = AF_UNIX; strncpy(addr.sun_path, monitor, sizeof(addr.sun_path)); + NUL_TERMINATE(addr.sun_path);
do { ret = connect(monfd, (struct sockaddr *) &addr, sizeof(addr));
ACK, but this mistake is made in pretty much every use of unix domain sockets :-( Could you fix the others here too.
Clearly strncpy() is just unfit for purpose, and we should plan to banish it in favour of a virStrncpy impl that guarnetees to add the trailing \0.
Yeah, true. Despite the ACKs from you and markmc, I decided not to commit this yet. It's quite a theoretical bug (you'd have to have /var/very/long/non-standard/paths/to/libvirt), so I don't think it's absolutely critical right now. I'll convert the rest of the users, and possible come up with a make syntax-check rule to prevent against future use.
Agreed, thanks ! Let's see with Dan if we can cook up a really clean solution, thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (4)
-
Chris Lalancette -
Daniel P. Berrange -
Daniel Veillard -
Mark McLoughlin