readdir_r smashes stack on long dir entry

[RuslanK]

Binary package hint: libc6

see attached archive

[Kees Cook (kees)]

Thanks for taking the time to report this bug and helping to make Ubuntu better. I cannot reproduce this on either amd64 or i386 with Ubuntu 9.10. Regardless, I suspect this is a bug with the code, based on the notes about readdir_r in the readdir_r manpage and the need to manually allocate the dir entry.

[Kees Cook (kees)]

Actually, on further investigation (thanks to Marc Deslauriers), it seems something weird is going on. With _FILE_OFFSET_BITS set to 64, glibc thinks the dirent is 280 bytes, not 276. This needs more study.

[Kees Cook (kees)]

Forwarded from https://launchpad.net/bugs/392501

It seems that the actual size of "struct dirent" with LFS enabled is 280 bytes,
but when defined for 32bit applications, the defined struct ends up at 276, and
something (the kernel?) is still writing the remaining 4 bytes.

Built on 64bit:
cc -Wall -Werror -fstack-protector -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64
-o test-native test.c
cc -Wall -Werror -fstack-protector -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64
-m32 -o test-m32 test.c
mkdir -p bug-dir
touch
bug-dir/111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111
./test-native bug-dir
sizeof(struct dirent): 280
./test-m32 bug-dir
sizeof(struct dirent): 276
*** stack smashing detected ***: ./test-m32 terminated

Built on 32bit:
cc -Wall -Werror -fstack-protector -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64
-o test-native test.c
cc -Wall -Werror -fstack-protector -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64
-m32 -o test-m32 test.c
mkdir -p bug-dir
touch
bug-dir/111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111
./test-native bug-dir
sizeof(struct dirent): 276
*** stack smashing detected ***: ./test-native terminated

/// test.c
#include <stdio.h>
#include <stdlib.h>
#include <dirent.h>
#include <inttypes.h>

void func(const char*path) {
    struct dirent entry;
    struct dirent *result = NULL;
    int ret;

    DIR *dir = opendir(path);
    if(!dir) abort();
    printf("sizeof(struct dirent): %" PRIuFAST32 "\n", sizeof(entry));
    while (!(ret = readdir_r(dir, &entry, &result)) && result) {}
}

int main(int argc, const char** argv) {
    if(argc < 2) abort();
    func(argv[1]);
    return 0;
}

[Kees Cook (kees)]

Created attachment 4636
Makefile

Line-wrapping did nasty things to the 255-character filename in the original
bug description. Here is a Makefile and test.c that demonstrates the issue.
What's really odd is that the 4 byte difference appears to be strictly padding?
 All the offsets and sizes are the same between 64bit and 32bit:

./test-native bug-dir
sizeof(struct dirent): 280
 sizeof(dirent.d_ino@0): 8
 sizeof(dirent.d_off@8): 8
 sizeof(dirent.d_reclen@16): 2
 sizeof(dirent.d_type@18): 1
 sizeof(dirent.d_name@19): 256
./test-m32 bug-dir
sizeof(struct dirent): 276
 sizeof(dirent.d_ino@0): 8
 sizeof(dirent.d_off@8): 8
 sizeof(dirent.d_reclen@16): 2
 sizeof(dirent.d_type@18): 1
 sizeof(dirent.d_name@19): 256
*** stack smashing detected ***: ./test-m32 terminated

[Kees Cook (kees)]

Created attachment 4637
test.c

[Kees Cook (kees)]

Created attachment 4638
test.c

This reports the reclen coming from the dirp->data. sysdeps/unix/readdir_r.c:

      bytes = __GETDENTS (dirp->fd, dirp->data, maxread);
...
      dp = (DIRENT_TYPE *) &dirp->data[dirp->offset];
...
      reclen = dp->d_reclen;
...
    *result = memcpy (entry, dp, reclen);

It seems that the memcpy is what overflows. I wonder if adding an
"assert(sizeof(*entry) >= reclen)" should be added in here for fun, too.

[Kees Cook (kees)]

Looks like the kernel unconditionally aligns/pads to 8 bytes in the 64bit
interface. fs/readdir.c, filldir64() says:
   int reclen = ALIGN(NAME_OFFSET(dirent) + namlen + 1, sizeof(u64));

which means it looks like alignment needs to be forced in glibc too. I don't
think __attribute__ ((aligned (sizeof (__off64_t)))) is acceptable for
bits/dirent.h as that's a gcc extension. Thoughts?

[Kees Cook (kees)]

See upstream bug, I've tracked this down now. The question remains:

- is gcc at fault for not padding struct dirent on 32bit compiles to 64bits?
- is the kernel at fault for padding reclen to 64bits regardless of struct size?
- is glibc at fault for not protecting callers from an overly large reclen from the kernel?

[Drepper-fsp (drepper-fsp)]

You cannot change the data structure definition, that's an ABI change.

I've added code handling the memcpy.

[Kees Cook (kees)]

Thanks!

http://repo.or.cz/w/glibc.git/commitdiff/1a81139728494810f65aaa0d0c538ff8c2783dd5

[Kees Cook (kees)]

Fixed upstream now.

http://repo.or.cz/w/glibc.git/commitdiff/1a81139728494810f65aaa0d0c538ff8c2783dd5

[Launchpad Janitor (janitor)]

This bug was fixed in the package eglibc - 2.11.1-0ubuntu6

---------------
eglibc (2.11.1-0ubuntu6) lucid; urgency=low

  [ Kees Cook ]
  * [BZ #11333], Handle unnecessary padding in getdents64. LP: #392501.

  [ Matthias Klose ]
  * Apply from the 2.11-x86 branch:
    - Fix bugs in strcmp-sse4.S and strcmp-ssse3.S (H.J. Lu). LP: #563291.
    - Fix bugs in memcpy-ssse3. LP: #560135.
  * Assign global scope to RFC 1918 addresses in getaddrinfo(). Thanks
    Tore Anderson. LP: #555210.
  * Re-enable the local-ipv6-lookup patch. Addresses #417757.
 -- Matthias Klose <email address hidden> Sun, 18 Apr 2010 00:05:05 +0200

[Alex Valavanis (valavanisalex)]

Intrepid Ibex reached end-of-life on 30 April 2010 so I am closing the
report. The bug is still marked as confirmed in later versions of Ubuntu.

[Launchpad Janitor (janitor)]

This bug was fixed in the package eglibc - 2.10.1-0ubuntu17

---------------
eglibc (2.10.1-0ubuntu17) karmic-security; urgency=low

  * SECURITY UPDATE: integer overflow in strfmon() might lead to arbitrary
    code execution.
    - debian/patches/any/git-strfmon-overflow.diff: backport from upstream.
    - CVE-2008-1391
  * SECURITY UPDATE: newlines not escaped in /etc/mtab.
    - debian/patches/any/git-mntent-newline-escape.diff: upstream fixes.
    - CVE-2010-0296
  * SECURITY UPDATE: arbitrary code execution from ELF headers (LP: #542197).
    - debian/patches/any/git-fix-dtag-cast.diff: upstream fixes.
    - CVE-2010-0830
  * debian/patches/any/git-readdir-padding.diff: fix readdir padding when
    processing getdents64() in a 32-bit execution environment (LP: #392501).
 -- Kees Cook <email address hidden> Wed, 19 May 2010 16:57:47 -0700

[Launchpad Janitor (janitor)]

This bug was fixed in the package glibc - 2.9-4ubuntu6.2

---------------
glibc (2.9-4ubuntu6.2) jaunty-security; urgency=low

  * SECURITY UPDATE: integer overflow in strfmon() might lead to arbitrary
    code execution.
    - debian/patches/any/git-strfmon-overflow.diff: backport from upstream.
    - CVE-2008-1391
  * SECURITY UPDATE: newlines not escaped in /etc/mtab.
    - debian/patches/any/git-mntent-newline-escape.diff: upstream fixes.
    - CVE-2010-0296
  * SECURITY UPDATE: arbitrary code execution from ELF headers (LP: #542197).
    - debian/patches/any/git-fix-dtag-cast.diff: upstream fixes.
    - CVE-2010-0830
  * debian/patches/any/git-readdir-padding.diff: fix readdir padding when
    processing getdents64() in a 32-bit execution environment (LP: #392501).
 -- Kees Cook <email address hidden> Wed, 19 May 2010 16:58:40 -0700

[Launchpad Janitor (janitor)]

This bug was fixed in the package glibc - 2.7-10ubuntu6

---------------
glibc (2.7-10ubuntu6) hardy-security; urgency=low

  * SECURITY UPDATE: integer overflow in strfmon() might lead to arbitrary
    code execution.
    - debian/patches/any/git-strfmon-overflow.diff: backport from upstream.
    - CVE-2008-1391
  * SECURITY UPDATE: newlines not escaped in /etc/mtab.
    - debian/patches/any/git-mntent-newline-escape.diff: upstream fixes.
    - CVE-2010-0296
  * SECURITY UPDATE: arbitrary code execution from ELF headers (LP: #542197).
    - debian/patches/any/git-fix-dtag-cast.diff: upstream fixes.
    - CVE-2010-0830
  * debian/patches/any/git-readdir-padding.diff: fix readdir padding when
    processing getdents64() in a 32-bit execution environment (LP: #392501).
 -- Kees Cook <email address hidden> Wed, 19 May 2010 16:59:18 -0700

[devsk (funtoos)]

Does this apply to earlier glibc versions as well?

[Kees Cook (kees)]

Yes, this bug seems to have always existed. I checked back through ancient
Linux kernel history, and it's always padded the dirent up to get the 64bit
alignment.