Bug 621953

Summary: function readdir_r appears to return data beyond the buffer (overflow)
Product: Red Hat Enterprise Linux 5 Reporter: D. Marlin <dmarlin>
Component: man-pages-overridesAssignee: Peter Schiffer <pschiffe>
Status: CLOSED ERRATA QA Contact: Iveta Wiedermann <isenfeld>
Severity: medium Docs Contact:
Priority: low    
Version: 5.5CC: bhu, esandeen, isenfeld, law, mnewsome, ovasik, rwheeler
Target Milestone: rcKeywords: ManPageChange, Patch, Reopened
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Cause: undocumented size of the buffer "entry" for the readdir_r function in the readdir.3 manual page Consequence: possible overflow in user programs Fix: back-port of the readdir.3 manual page from RHEL-6 Result: complete documentation of readdir and readdir_r functions
Story Points: ---
Clone Of:
: 840840 (view as bug list) Environment:
Last Closed: 2013-01-08 02:36:39 EST Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
Bug Depends On:    
Bug Blocks: 840840    
Attachments:
Description Flags
test program to demonstrate problem
none
readdir.patch
none
readdir.patch
none
readdir.patch none

Description D. Marlin 2010-08-06 11:39:21 EDT
Created attachment 437184 [details]
test program to demonstrate problem

Description of problem:

When running a test program to check the buffer contents returned by readdir_r there appears to be data returned beyond the end of the dirent structure.  The test program allocates extra buffer space and fills it with a known value for comparison.


Version-Release number of selected component (if applicable):
  kernel-2.6.18-194.el5


How reproducible:
  always


Steps to Reproduce:
1. gcc -g -o0 -o readdir_r_buf_overrun readdir_r_buf_overrun.c
2. mkdir run; cd run
3. ../readdir_r_buf_overrun


I also ran this via gdb to examine the full contents of the dirent structure and the buffer after return from readdir_r, i.e.,

  gcc -g -o0 -o readdir_r_buf_overrun readdir_r_buf_overrun.c
  gdb readdir_r_buf_overrun
  break 45
  set print elements 300
  run
  print *buf
  x/272xb buf


Actual results:

dentry: long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-long-filename
buf overrun check: xooooooooooooooo  BUFFER OVERRUN DETECTED !!

(gdb) x/272xb buf
0x8052028:      0xbe    0x88    0xf8    0x03    0x03    0x00    0x00    0x00
    :
0x8052120:      0x6e    0x67    0x2d    0x6c    0x6f    0x6e    0x67    0x2d
0x8052128:      0x66    0x69    0x6c    0x65    0x6e    0x61    0x6d    0x65
0x8052130:      0x00    0x00    0x00    0x08    0x63    0x63    0x63    0x63
                        ^^^^    ^^^^    ^^^^

Expected results:

No overrun.

    :
0x8052128:      0x66    0x69    0x6c    0x65    0x6e    0x61    0x6d    0x65
0x8052130:      0x00    0x63    0x63    0x63    0x63    0x63    0x63    0x63
                        ^^^^    ^^^^    ^^^^

Additional info:

I found that the extra data also appears in 'some' reads of shorter file/directory names, i.e.,

(gdb) print *buf
$4 = {d_ino = 33216788, d_off = 5, d_reclen = 36, d_type = 8 '\b', 
  d_name = "readdir_r_buf_overrun.c\000\b", 'c' <repeats 231 times>}

(gdb) x/272xb buf
0x8052028:      0x14    0xd9    0xfa    0x01    0x05    0x00    0x00    0x00
0x8052030:      0x24    0x00    0x08    0x72    0x65    0x61    0x64    0x64
0x8052038:      0x69    0x72    0x5f    0x72    0x5f    0x62    0x75    0x66
0x8052040:      0x5f    0x6f    0x76    0x65    0x72    0x72    0x75    0x6e
0x8052048:      0x2e    0x63    0x00    0x08    0x63    0x63    0x63    0x63
    :                                   ^^^^

but this does not overflow the allocated space in d_name.

In linux-2.6.18.i686/fs/readdir.c I saw:

#define NAME_OFFSET(de) ((int) ((de)->d_name - (char __user *) (de)))
#define ROUND_UP(x) (((x)+sizeof(long)-1) & ~(sizeof(long)-1)) 

    :

static int filldir(void * __buf, const char * name, int namlen, loff_t offset,
                  u64 ino, unsigned int d_type)
{
       struct linux_dirent __user * dirent;
       struct getdents_callback * buf = (struct getdents_callback *) __buf;
       unsigned long d_ino;
       int reclen = ROUND_UP(NAME_OFFSET(dirent) + namlen + 2); 


Could the reclen calculation be causing extra bytes to be returned to userspace?  If so, might this overrun the allocated space for dirent in the event of very long file names?


note: This is also reproducible on other RHEL5 releases, and recent Fedora versions (e.g., F12).
Comment 1 Eric Sandeen 2010-08-09 22:49:30 EDT
The 0x08 you see is d_type:

"The d_type  field is implemented since Linux 2.6.4.  
It occupies a space that was previously a zero-filled padding byte in the
linux_dirent structure.  Thus, on kernels  before  2.6.3,  attempting  to  access  this  field  always  provides  the  value  0 (DT_UNKNOWN)."

You got 0x08 

/*
 * File types
 *
 * NOTE! These match bits 12..15 of stat.st_mode
 * (ie "(i_mode >> 12) & 15").
 */

...

#define DT_REG          8

I'm not certain about why you see more nulls before it, I'll have to look into that rounding up a little more.

How did you find this, did you find any actual functional problem?

Thanks,
-Eric
Comment 2 D. Marlin 2010-08-10 11:11:16 EDT
I'm not sure I made one point clear.  I see the dirent structure looks like:

  struct dirent
    {
  #ifndef __USE_FILE_OFFSET64
      __ino_t d_ino;
      __off_t d_off;
  #else
      __ino64_t d_ino;
      __off64_t d_off;
  #endif
      unsigned short int d_reclen;
      unsigned char d_type;
      char d_name[256];           /* We must not include limits.h! */
    };

and that d_type is the forth element in the structure, just before d_name, but the issue I'm seeing is that data may be returned _after_ d_name, and it (coincidentally?) matches the d_type value.  If I'm looking at this correctly there shouldn't be any data returned after the string in d_name.

In the second (short name) example I posted you can see this more clearly:

----------------------
(gdb) print *buf
$4 = {d_ino = 33216788, d_off = 5, d_reclen = 36, d_type = 8 '\b', 
  d_name = "readdir_r_buf_overrun.c\000\b", 'c' <repeats 231 times>}
                                       ^^^

(gdb) x/272xb buf
0x8052028:      0x14    0xd9    0xfa    0x01    0x05    0x00    0x00    0x00
               { d_ino                      }  { d_off                      }
0x8052030:      0x24    0x00    0x08    0x72    0x65    0x61    0x64    0x64
               { d_reclen   } {d_type} { d_name ...
0x8052038:      0x69    0x72    0x5f    0x72    0x5f    0x62    0x75    0x66
0x8052040:      0x5f    0x6f    0x76    0x65    0x72    0x72    0x75    0x6e
0x8052048:      0x2e    0x63    0x00    0x08    0x63    0x63    0x63    0x63
             ...            }   {Null} {????}
    :                                   ^^^^
----------------------


Should that last 0x08 be there (in the buffer _after_ the null terminated string)?  If so, what puts it there and why?  On the first (long name) example I posted, the extra data after d_name would overflow the buffer length, if the extra (16) bytes had not been allocated.  On shorter names it would probably go unnoticed, since it is within the buffer space and after the null terminator.

I hope this clarifies the issue I'm seeing.


This issue was reported by a customer using a custom kernel that we provided, which is based on a Fedora 10 kernel, but I found it was also reproducible on the stock RHEL5 kernel so I reported it here.  Since it was still reproducible on F12 it may affect RHEL6 as well.
Comment 3 Edward Shishkin 2010-08-24 12:27:09 EDT
Hello everyone. I confirm the overflow.
It occurs in glibc function readdir_r (3).

So in accordance with POSIX recommendations user allocates a buffer
of length

offsetof(struct dirent, d_name) + pathconf(dirpath, _PC_NAME_MAX) + 1    (*)

and passes it to readdir_r(). The last one reads directory entries to
the directory stream using getdents (2) system call, converts kernel
format of directory entry (struct kernel_dirent) to the POSIX format
(struct dirent) by function __GETDENTS() and passes a directory entry
in POSIX format to user. The overflow occurs in line 116,
file sysdeps/unix/readdir_r.c, function __READDIR_R (glibc-2.5-42):

  *result = memcpy (entry, dp, reclen);

here @reclen is size of directory entry in kernel format, which is
calculated and passed by kernel. However, directory entry in kernel
format is *larger* than directory entry in POSIX format: kernel rounds
size of his format up by sizeof(long), but POSIX doesn't have such
recommendations (AFAIK). In our testcase reclen = 268 (253 + 1 + 11 =
265, rounded up by 4), and user allocates buffer of smaller size (*)
255 + 11 + 1 = 267, so we get problems every time when size of
directory entry name (without null-termination) is 253, 254 or 255.

So I wouldn't blame a new kernel getdents interface: everything is
okay here, moreover glibc uses it correctly. Also I wouldn't blame
users, as they follow to POSIX recommendations (however, the problem
will disappear, if user allocates buffer of size, which is rounded
up by sizeof long). I think that glibc is the culprit: why do we need
to make an assumption that POSIX and kernel formats have identical
sizes?

glibc maintainer cc-ed, please, take a look.

Thanks,
Edward.
Comment 4 Beth Uptagrafft 2010-10-05 12:23:56 EDT
Has there been any more progress on this issue?
Comment 5 D. Marlin 2010-10-15 11:58:22 EDT
The customer is asking about this again, so I wanted to check if there was any progress to report.

Will someone please provide a status update?

Thank you.
Comment 6 Beth Uptagrafft 2010-11-08 14:00:14 EST
Please help - the customer is asking for an update. Has a glibc maintainer looked at this issue as requested in Comment #3? 

Thanks,
Beth
Comment 7 Andreas Schwab 2010-11-09 05:53:59 EST
The storage pointed to by entry shall be large enough for a dirent with an array of char d_name members containing at least {NAME_MAX}+1 elements.

The array of char d_name is not a fixed size. Implementations may need to declare struct dirent with an array size for d_name of 1, but the actual number of characters provided matches (or only slightly exceeds) the length of the filename.
Comment 8 D. Marlin 2010-11-09 12:03:27 EST
From your reply:

> Implementations may need to declare struct dirent with an array size for d_name of 1, but the actual number of characters provided matches (or only slightly exceeds) the length of the filename.

but as far as I can tell they were following the recommendations for the amount of space to allocate, i.e.,

  Since POSIX.1 does not specify the size of the d_name field, and other
  nonstandard fields may precede that field within the dirent structure,
  portable applications that use readdir_r() should allocate the buffer whose
  address is passed in entry as follows:

     len = offsetof(struct dirent, d_name) +
               pathconf(dirpath, _PC_NAME_MAX) + 1
     entryp = malloc(len);


Is a different calculation recommended to ensure adequate space is allocated for the d_name entry?  If so, will you please provide a pointer to the description in the documentation so we can give that to the customer for reference?
Comment 9 Eric Bachalo 2010-12-17 16:24:16 EST
I my opinion the line:

      reclen = MIN (reclen, NAME_MAX);

should be inserted before line:

      *result = memcpy (entry, dp, reclen);

In function __READDIR_R () in file /sysdeps/unix/readdir_r.c


Reasoning is

According to Posix docs

from http://pubs.opengroup.org/onlinepubs/009695399/basedefs/dirent.h.html
"The character array d_name is of unspecified size, but the number of bytes preceding the terminating null byte shall not exceed {NAME_MAX}."

in same doc under Application Usage: 
"The name of an array of char of an unspecified size should not be used as an lvalue. Use of:
sizeof(d_name)
is incorrect; use:
strlen(d_name)
instead."

and 

from http://pubs.opengroup.org/onlinepubs/009695399/functions/readdir.html
"The storage pointed to by entry shall be large enough for a dirent with an array of char d_name members containing at least {NAME_MAX}+1 elements."

To me it says that if user allocates NAME_MAX + 1 to d_name they can assume they are safe, which isn't the case currently in Linux as proven by the case in thiz BZ.

I can understand why glibc would not use strlen as that is time consuming but I think it should never copy more than NAME_MAX characters.
Comment 10 Eric Bachalo 2010-12-20 13:40:39 EST
Ulrich Drepper informed me on 2010-12-19 that he checked in a patch upstream for this issue.
Comment 11 Andreas Schwab 2011-01-10 04:19:41 EST
You must never pass less than struct dirent.
Comment 13 Eric Bachalo 2011-01-12 13:13:15 EST
My suggested fix in https://bugzilla.redhat.com/show_bug.cgi?id=621953#c9 was incorrect.

It should be:

      reclen = MIN (reclen, sizeof(struct dirent));

be inserted before line:

      *result = memcpy (entry, dp, reclen);

there is probably better way of insuring reclen is not larger than maximum possible set by POSIX.  I believe Uli submitted a patch to do this, I haven't confirmed.


In the version of readdir_r.c I have (dated 2010-02-22),

on line 90
      reclen = dp->d_reclen;

dp->d_reclen comes from GETDENTS call on line 64
	  bytes = __GETDENTS (dirp->fd, dirp->data, maxread);

According to Edward Shishkin in https://bugzilla.redhat.com/show_bug.cgi?id=621953#c3

The kernel's GETDENTS call can set the reclen to larger than maximum size allowed for struct dirent by POSIX standards due to rounding up the size to nearest size of long.

simple example with old simple dirent the max size is:

    unsigned short int d_reclen; (2 bytes)
    unsigned char d_type; (1 byte)
    char d_name[NAME_MAX+1]; (256 bytes)

total (259 bytes)  

with this simple example GETDENTS would allocate 260 bytes and return d_reclen value of 260 when pathname was NAME_MAX long.

readdir_r.c would then copy 260 bytes into a dirent structure passed in by user with direct struct of 259 bytes in line 117

    *result = memcpy (entry, dp, reclen);

causing overrun of the entry dirent struct.
Comment 14 Eric Bachalo 2011-01-12 13:37:32 EST
Here is a link to Uli's patch for this issue:

http://sourceware.org/git/?p=glibc.git;a=commitdiff;h=1a81139728494810f65aaa0d0c538ff8c2783dd5#patch1
Comment 15 Andreas Schwab 2011-01-13 05:28:44 EST
The test program is simply INVALID.  The buffer size is smaller than struct dirent.
Comment 16 Eric Sandeen 2011-01-13 12:20:19 EST
In my testing on x86_64, the allocated buffer is 291 bytes, while the size of struct dirent is 280.  What am I missing?

        bufsize = offsetof(struct dirent, d_name) + name_max + 1;
        if ((buf = (struct dirent *)malloc(bufsize + BUF_EXTRA)) == NULL) {
                perror("malloc");
        }
        printf("bufsize %d allocated %d dirent %d\n", bufsize, bufsize + BUF_EXTRA, sizeof(struct dirent));

bufsize 275 allocated 291 dirent 280
Comment 17 D. Marlin 2011-03-30 13:10:03 EDT
Since this is not considered a bug, but fails when using the suggested code from the man page, I am switching this BZ to the man-page component and requesting the following text:

       Since POSIX.1 does not specify the size of the d_name field, and other
       nonstandard fields may precede that field within the dirent structure,
       portable applications that use readdir_r() should allocate the buffer whose
       address is passed in entry as follows:

           len = offsetof(struct dirent, d_name) +
                     pathconf(dirpath, _PC_NAME_MAX) + 1
           entryp = malloc(len);

       (POSIX.1 requires that d_name is the last field in a struct dirent.)

be changed to recommend:

                  :
           len = sizeof(struct dirent);
           entryp = malloc(len);
                  :

This appears to work for some test cases tried, but I wanted to confirm that this is the correct change, and if so, have it applied to the man page.
Comment 18 RHEL Product and Program Management 2011-09-22 20:08:30 EDT
This request was evaluated by Red Hat Product Management for
inclusion in the current release of Red Hat Enterprise Linux.
Because the affected component is not scheduled to be updated in the
current release, Red Hat is unfortunately unable to address this
request at this time. Red Hat invites you to ask your support
representative to propose this request, if appropriate and relevant,
in the next release of Red Hat Enterprise Linux.
Comment 19 Peter Schiffer 2011-10-19 06:21:11 EDT
Please, can somebody confirm that comment #17 is correct, so I can apply it to the man page?
Thanks.
Comment 20 Jeff Law 2012-03-07 15:08:25 EST
Eric, in response to c#16:

sizeof (anything) is going to be rounded up to its natural boundary which will necessitate padding.  This is vital so that allocating and iterating on an array of these structures will work properly.

I don't think the change requested in c#17 is correct.  What really needs to happen is to account for the rounding/tail padding.

AFAICT, this has been fixed as man readdir_r shows the following on my F16 system:

      Since POSIX.1 does not specify the size of the d_name field, and other nonstandard fields may precede that field within the dirent structure, portable applications that use read‐
       dir_r() should allocate the buffer whose address is passed in entry as follows:

           len = (offsetof(struct dirent, d_name) +
                     pathconf(dirpath, _PC_NAME_MAX) + 1 + sizeof(long))
                     & -sizeof(long);

           entryp = malloc(len);

Note the "(... + sizeof (long)) & -sizeof(long)

That will account for the rounding.
Comment 21 Peter Schiffer 2012-05-28 12:48:19 EDT
Created attachment 587263 [details]
readdir.patch

Patch adding the note mentioned in the comment 20. Is this patch sufficient to fix this bug?
Comment 22 RHEL Product and Program Management 2012-06-11 21:01:51 EDT
This request was evaluated by Red Hat Product Management for
inclusion in the current release of Red Hat Enterprise Linux.
Because the affected component is not scheduled to be updated in the
current release, Red Hat is unfortunately unable to address this
request at this time. Red Hat invites you to ask your support
representative to propose this request, if appropriate and relevant,
in the next release of Red Hat Enterprise Linux.
Comment 24 RHEL Product and Program Management 2012-07-17 07:08:23 EDT
This request was evaluated by Red Hat Product Management for inclusion
in a Red Hat Enterprise Linux release.  Product Management has
requested further review of this request by Red Hat Engineering, for
potential inclusion in a Red Hat Enterprise Linux release for currently
deployed products.  This request is not yet committed for inclusion in
a release.
Comment 26 Peter Schiffer 2012-07-17 09:47:45 EDT
Created attachment 598637 [details]
readdir.patch
Comment 27 Peter Schiffer 2012-07-17 11:15:11 EDT
Created attachment 598670 [details]
readdir.patch

one last update of attached patch - I've back-ported readdir.3 man page from RHEL-6 as it contains much more useful information
Comment 28 Peter Schiffer 2012-07-17 11:33:05 EDT
    Technical note added. If any revisions are required, please edit the "Technical Notes" field
    accordingly. All revisions will be proofread by the Engineering Content Services team.
    
    New Contents:
Cause: undocumented size of the buffer "entry" for the readdir_r function in the readdir.3 manual page
Consequence: possible overflow in user programs
Fix: back-port of the readdir.3 manual page from RHEL-6
Result: complete documentation of readdir and readdir_r functions
Comment 33 errata-xmlrpc 2013-01-08 02:36:39 EST
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

http://rhn.redhat.com/errata/RHBA-2013-0073.html