readdir_r smashes stack on long dir entry

Bug #392501 reported by RuslanK on 2009-06-26
10
This bug affects 1 person
Affects Status Importance Assigned to Milestone
GLibC
Fix Released
Medium
eglibc (Ubuntu)
Medium
Matthias Klose
Dapper
Undecided
Unassigned
Hardy
Undecided
Unassigned
Intrepid
Undecided
Unassigned
Jaunty
Undecided
Unassigned
Karmic
Medium
Unassigned
Lucid
Medium
Matthias Klose
glibc (Ubuntu)
Undecided
Unassigned
Dapper
Medium
Unassigned
Hardy
Medium
Unassigned
Intrepid
Medium
Unassigned
Jaunty
Medium
Unassigned
Karmic
Undecided
Unassigned
Lucid
Undecided
Unassigned

Bug Description

Binary package hint: libc6

see attached archive

RuslanK (box-with-mail) wrote :
Kees Cook (kees) wrote :

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.

security vulnerability: yes → no
visibility: private → public
Changed in glibc (Ubuntu):
assignee: nobody → Kees Cook (kees)
status: New → Invalid
Kees Cook (kees) wrote :

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.

Changed in glibc (Ubuntu):
status: Invalid → Confirmed
assignee: Kees Cook (kees) → nobody

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) on 2010-02-27
Changed in eglibc (Ubuntu):
status: New → Confirmed

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

Created attachment 4637
test.c

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.

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?

Changed in glibc:
status: Unknown → Confirmed
Kees Cook (kees) on 2010-03-02
Changed in glibc (Ubuntu Lucid):
status: Confirmed → Invalid
Changed in glibc (Ubuntu Karmic):
status: New → Invalid
Changed in eglibc (Ubuntu Lucid):
status: Confirmed → Invalid
Changed in eglibc (Ubuntu Dapper):
status: New → Invalid
Changed in eglibc (Ubuntu Hardy):
status: New → Invalid
Changed in eglibc (Ubuntu Intrepid):
status: New → Invalid
Changed in eglibc (Ubuntu Jaunty):
status: New → Invalid
Changed in eglibc (Ubuntu Karmic):
status: New → Invalid
Changed in eglibc (Ubuntu Lucid):
status: Invalid → Triaged
Changed in glibc (Ubuntu Dapper):
status: New → Triaged
Changed in eglibc (Ubuntu Karmic):
status: Invalid → Triaged
Changed in glibc (Ubuntu Hardy):
status: New → Triaged
Kees Cook (kees) on 2010-03-02
Changed in glibc (Ubuntu Intrepid):
status: New → Triaged
Changed in glibc (Ubuntu Jaunty):
status: New → Triaged
Changed in glibc (Ubuntu Dapper):
importance: Undecided → Medium
Changed in glibc (Ubuntu Hardy):
importance: Undecided → Medium
Changed in glibc (Ubuntu Intrepid):
importance: Undecided → Medium
Changed in glibc (Ubuntu Jaunty):
importance: Undecided → Medium
Changed in eglibc (Ubuntu Lucid):
importance: Undecided → Medium
Changed in eglibc (Ubuntu Karmic):
importance: Undecided → Medium
Kees Cook (kees) wrote :

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?

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

I've added code handling the memcpy.

Changed in glibc:
status: Confirmed → Fix Released
Kees Cook (kees) on 2010-04-05
Changed in eglibc (Ubuntu Lucid):
assignee: nobody → Matthias Klose (doko)
milestone: none → ubuntu-10.04
Matthias Klose (doko) on 2010-04-15
Changed in eglibc (Ubuntu Lucid):
status: Triaged → Fix Committed
Launchpad Janitor (janitor) wrote :

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

Changed in eglibc (Ubuntu Lucid):
status: Fix Committed → Fix Released
Alex Valavanis (valavanisalex) wrote :

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.

Changed in glibc (Ubuntu Intrepid):
status: Triaged → Invalid
Launchpad Janitor (janitor) wrote :

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) wrote :

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) wrote :

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

Changed in eglibc (Ubuntu Karmic):
status: Triaged → Fix Released
Changed in glibc (Ubuntu Hardy):
status: Triaged → Fix Released
Changed in glibc (Ubuntu Jaunty):
status: Triaged → Fix Released
Kees Cook (kees) on 2010-05-25
Changed in glibc (Ubuntu Dapper):
status: Triaged → Fix Released

Does this apply to earlier glibc versions as well?

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.

Changed in glibc:
importance: Unknown → Medium
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers

Bug attachments

Remote bug watches

Bug watches keep track of this bug in other bug trackers.