Package: libxdg-basedir1
Version: 1.1.1-2
Severity: critical
File: libxdg-basedir
Tags: patch upstream
Dear Maintainer,
Any application using xdgDataHome, xdgConfigHome and possibly others
will trigger invalid reads and writes in valgrind. For example the
following code:
const char *xdg_data_home = xdgDataHome(NULL);
printf("%s\n", xdg_data_home);
It triggers:
==14808== Invalid write of size 1
==14808== at 0x4C2D97A: memcpy@GLIBC_2.2.5 (mc_replace_strmem.c:881)
==14808== by 0x4E352A0: xdgGetRelativeHome (basedir.c:577)
==14808== by 0x4006A5: main (in /tmp/a.out)
==14808== Address 0x53e305b is 0 bytes after a block of size 27 alloc'd
==14808== at 0x4C294A0: malloc (vg_replace_malloc.c:291)
==14808== by 0x4E35278: xdgGetRelativeHome (basedir.c:575)
==14808== by 0x4006A5: main (in /tmp/a.out)
==14808==
==14808== Invalid read of size 1
==14808== at 0x4C2C954: __GI_strlen (mc_replace_strmem.c:405)
==14808== by 0x50A42DB: puts (ioputs.c:36)
==14808== by 0x4006B5: main (in /tmp/a.out)
==14808== Address 0x53e305b is 0 bytes after a block of size 27 alloc'd
==14808== at 0x4C294A0: malloc (vg_replace_malloc.c:291)
==14808== by 0x4E35278: xdgGetRelativeHome (basedir.c:575)
==14808== by 0x4006A5: main (in /tmp/a.out)
==14808==
The solution is simple: add a +1 to the malloc in line 575 of basedir.c.
This as it uses the +1 on line 577 when limiting the amount of data that
is to be copied.
Attached are:
* a full test case
* full result of valgrind before patching the library
* the patch for the library
* full result of valgrind after patching the library
I'm in doubt whether this is really this critical or not; I could
imagine ways that, under certain circumstances, it is possible to crash
users of this library. Especially when this allocation happens at the
end of a page, and the next page isn't valid. Then writing the last
byte/character could trigger a segmentation fault. Furthermore, the last
byte could corrupt the memory management pointers at the begin of
allocations causing malloc and/or free to crash, as well as malloc/free
overwriting the last byte and the string then becoming longer due to the
missing '\0' termination causing applications to crash when e.g. strdup
allocates the memory based on strlen before allocation, then allocates
and corrupts the string, and finally memcpy-ing but not '\0' terminating
the duplicated string. These are all hypothetical though.
Regards,
Remko Bijker
-- System Information:
Debian Release: jessie/sid
APT prefers unstable
APT policy: (500, 'unstable'), (1, 'experimental')
Architecture: amd64 (x86_64)
Kernel: Linux 3.11-2-amd64 (SMP w/4 CPU cores)
Locale: LANG=en_GB.UTF-8, LC_CTYPE=en_GB.UTF-8 (charmap=UTF-8)
Shell: /bin/sh linked to /bin/dash
Versions of packages libxdg-basedir1 depends on:
ii libc6 2.17-96
libxdg-basedir1 recommends no packages.
libxdg-basedir1 suggests no packages.
-- no debconf information
==14838== Memcheck, a memory error detector
==14838== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==14838== Using Valgrind-3.9.0 and LibVEX; rerun with -h for copyright info
==14838== Command: ./a.out
==14838==
/home/rubidium/.local/share
==14838==
==14838== HEAP SUMMARY:
==14838== in use at exit: 0 bytes in 0 blocks
==14838== total heap usage: 1 allocs, 1 frees, 28 bytes allocated
==14838==
==14838== All heap blocks were freed -- no leaks are possible
==14838==
==14838== For counts of detected and suppressed errors, rerun with: -v
==14838== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 2 from 2)
==14808== Memcheck, a memory error detector
==14808== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==14808== Using Valgrind-3.9.0 and LibVEX; rerun with -h for copyright info
==14808== Command: ./a.out
==14808==
==14808== Invalid write of size 1
==14808== at 0x4C2D97A: memcpy@GLIBC_2.2.5 (mc_replace_strmem.c:881)
==14808== by 0x4E352A0: xdgGetRelativeHome (basedir.c:577)
==14808== by 0x4006A5: main (in /tmp/a.out)
==14808== Address 0x53e305b is 0 bytes after a block of size 27 alloc'd
==14808== at 0x4C294A0: malloc (vg_replace_malloc.c:291)
==14808== by 0x4E35278: xdgGetRelativeHome (basedir.c:575)
==14808== by 0x4006A5: main (in /tmp/a.out)
==14808==
==14808== Invalid read of size 1
==14808== at 0x4C2C954: __GI_strlen (mc_replace_strmem.c:405)
==14808== by 0x50A42DB: puts (ioputs.c:36)
==14808== by 0x4006B5: main (in /tmp/a.out)
==14808== Address 0x53e305b is 0 bytes after a block of size 27 alloc'd
==14808== at 0x4C294A0: malloc (vg_replace_malloc.c:291)
==14808== by 0x4E35278: xdgGetRelativeHome (basedir.c:575)
==14808== by 0x4006A5: main (in /tmp/a.out)
==14808==
/home/rubidium/.local/share
==14808==
==14808== HEAP SUMMARY:
==14808== in use at exit: 0 bytes in 0 blocks
==14808== total heap usage: 1 allocs, 1 frees, 27 bytes allocated
==14808==
==14808== All heap blocks were freed -- no leaks are possible
==14808==
==14808== For counts of detected and suppressed errors, rerun with: -v
==14808== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 2 from 2)
diff -Nupr libxdg-basedir-1.1.1.orig/src/basedir.c libxdg-basedir-1.1.1/src/basedir.c
--- libxdg-basedir-1.1.1.orig/src/basedir.c 2013-11-25 16:07:09.864380760 +0100
+++ libxdg-basedir-1.1.1/src/basedir.c 2013-11-25 16:09:07.452378162 +0100
@@ -572,7 +572,7 @@ static char * xdgGetRelativeHome(const c
unsigned int homelen;
if (!(home = xdgGetEnv("HOME")))
return NULL;
- if (!(relhome = (char*)malloc((homelen = strlen(home))+fallbacklength))) return NULL;
+ if (!(relhome = (char*)malloc((homelen = strlen(home))+fallbacklength+1))) return NULL;
memcpy(relhome, home, homelen);
memcpy(relhome+homelen, relativefallback, fallbacklength+1);
}
#include <stdlib.h>
#include <stdio.h>
#include <basedir.h>
int main(int argc, char *argv[])
{
const char *xdg_data_home = xdgDataHome(NULL);
printf("%s\n", xdg_data_home);
free((void*)xdg_data_home);
return 0;
}
