| commit 91ce40854d0b7f865cf5024ef95a8026b76096f3 |
| Author: Florian Weimer <fweimer@redhat.com> |
| Date: Fri Aug 16 09:38:52 2013 +0200 |
| |
| CVE-2013-4237, BZ #14699: Buffer overflow in readdir_r |
| |
| * sysdeps/posix/dirstream.h (struct __dirstream): Add errcode |
| member. |
| * sysdeps/posix/opendir.c (__alloc_dir): Initialize errcode |
| member. |
| * sysdeps/posix/rewinddir.c (rewinddir): Reset errcode member. |
| * sysdeps/posix/readdir_r.c (__READDIR_R): Enforce NAME_MAX limit. |
| Return delayed error code. Remove GETDENTS_64BIT_ALIGNED |
| conditional. |
| * sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c: Do not define |
| GETDENTS_64BIT_ALIGNED. |
| * sysdeps/unix/sysv/linux/i386/readdir64_r.c: Likewise. |
| * manual/filesys.texi (Reading/Closing Directory): Document |
| ENAMETOOLONG return value of readdir_r. Recommend readdir more |
| strongly. |
| * manual/conf.texi (Limits for Files): Add portability note to |
| NAME_MAX, PATH_MAX. |
| (Pathconf): Add portability note for _PC_NAME_MAX, _PC_PATH_MAX. |
| |
| diff --git a/manual/conf.texi b/manual/conf.texi |
| index 7eb8b36..c720063 100644 |
| |
| |
| @@ -1149,6 +1149,9 @@ typed ahead as input. @xref{I/O Queues}. |
| @deftypevr Macro int NAME_MAX |
| The uniform system limit (if any) for the length of a file name component, not |
| including the terminating null character. |
| + |
| +@strong{Portability Note:} On some systems, @theglibc{} defines |
| +@code{NAME_MAX}, but does not actually enforce this limit. |
| @end deftypevr |
| |
| @comment limits.h |
| @@ -1157,6 +1160,9 @@ including the terminating null character. |
| The uniform system limit (if any) for the length of an entire file name (that |
| is, the argument given to system calls such as @code{open}), including the |
| terminating null character. |
| + |
| +@strong{Portability Note:} @Theglibc{} does not enforce this limit |
| +even if @code{PATH_MAX} is defined. |
| @end deftypevr |
| |
| @cindex limits, pipe buffer size |
| @@ -1476,6 +1482,9 @@ Inquire about the value of @code{POSIX_REC_MIN_XFER_SIZE}. |
| Inquire about the value of @code{POSIX_REC_XFER_ALIGN}. |
| @end table |
| |
| +@strong{Portability Note:} On some systems, @theglibc{} does not |
| +enforce @code{_PC_NAME_MAX} or @code{_PC_PATH_MAX} limits. |
| + |
| @node Utility Limits |
| @section Utility Program Capacity Limits |
| |
| diff --git a/manual/filesys.texi b/manual/filesys.texi |
| index 1df9cf2..814c210 100644 |
| |
| |
| @@ -444,9 +444,9 @@ symbols are declared in the header file @file{dirent.h}. |
| @comment POSIX.1 |
| @deftypefun {struct dirent *} readdir (DIR *@var{dirstream}) |
| This function reads the next entry from the directory. It normally |
| -returns a pointer to a structure containing information about the file. |
| -This structure is statically allocated and can be rewritten by a |
| -subsequent call. |
| +returns a pointer to a structure containing information about the |
| +file. This structure is associated with the @var{dirstream} handle |
| +and can be rewritten by a subsequent call. |
| |
| @strong{Portability Note:} On some systems @code{readdir} may not |
| return entries for @file{.} and @file{..}, even though these are always |
| @@ -461,19 +461,61 @@ conditions are defined for this function: |
| The @var{dirstream} argument is not valid. |
| @end table |
| |
| -@code{readdir} is not thread safe. Multiple threads using |
| -@code{readdir} on the same @var{dirstream} may overwrite the return |
| -value. Use @code{readdir_r} when this is critical. |
| +To distinguish between an end-of-directory condition or an error, you |
| +must set @code{errno} to zero before calling @code{readdir}. To avoid |
| +entering an infinite loop, you should stop reading from the directory |
| +after the first error. |
| + |
| +In POSIX.1-2008, @code{readdir} is not thread-safe. In @theglibc{} |
| +implementation, it is safe to call @code{readdir} concurrently on |
| +different @var{dirstream}s, but multiple threads accessing the same |
| +@var{dirstream} result in undefined behavior. @code{readdir_r} is a |
| +fully thread-safe alternative, but suffers from poor portability (see |
| +below). It is recommended that you use @code{readdir}, with external |
| +locking if multiple threads access the same @var{dirstream}. |
| @end deftypefun |
| |
| @comment dirent.h |
| @comment GNU |
| @deftypefun int readdir_r (DIR *@var{dirstream}, struct dirent *@var{entry}, struct dirent **@var{result}) |
| -This function is the reentrant version of @code{readdir}. Like |
| -@code{readdir} it returns the next entry from the directory. But to |
| -prevent conflicts between simultaneously running threads the result is |
| -not stored in statically allocated memory. Instead the argument |
| -@var{entry} points to a place to store the result. |
| +This function is a version of @code{readdir} which performs internal |
| +locking. Like @code{readdir} it returns the next entry from the |
| +directory. To prevent conflicts between simultaneously running |
| +threads the result is stored inside the @var{entry} object. |
| + |
| +@strong{Portability Note:} It is recommended to use @code{readdir} |
| +instead of @code{readdir_r} for the following reasons: |
| + |
| +@itemize @bullet |
| +@item |
| +On systems which do not define @code{NAME_MAX}, it may not be possible |
| +to use @code{readdir_r} safely because the caller does not specify the |
| +length of the buffer for the directory entry. |
| + |
| +@item |
| +On some systems, @code{readdir_r} cannot read directory entries with |
| +very long names. If such a name is encountered, @theglibc{} |
| +implementation of @code{readdir_r} returns with an error code of |
| +@code{ENAMETOOLONG} after the final directory entry has been read. On |
| +other systems, @code{readdir_r} may return successfully, but the |
| +@code{d_name} member may not be NUL-terminated or may be truncated. |
| + |
| +@item |
| +POSIX-1.2008 does not guarantee that @code{readdir} is thread-safe, |
| +even when access to the same @var{dirstream} is serialized. But in |
| +current implementations (including @theglibc{}), it is safe to call |
| +@code{readdir} concurrently on different @var{dirstream}s, so there is |
| +no need to use @code{readdir_r} in most multi-threaded programs. In |
| +the rare case that multiple threads need to read from the same |
| +@var{dirstream}, it is still better to use @code{readdir} and external |
| +synchronization. |
| + |
| +@item |
| +It is expected that future versions of POSIX will obsolete |
| +@code{readdir_r} and mandate the level of thread safety for |
| +@code{readdir} which is provided by @theglibc{} and other |
| +implementations today. |
| +@end itemize |
| |
| Normally @code{readdir_r} returns zero and sets @code{*@var{result}} |
| to @var{entry}. If there are no more entries in the directory or an |
| @@ -481,15 +523,6 @@ error is detected, @code{readdir_r} sets @code{*@var{result}} to a |
| null pointer and returns a nonzero error code, also stored in |
| @code{errno}, as described for @code{readdir}. |
| |
| -@strong{Portability Note:} On some systems @code{readdir_r} may not |
| -return a NUL terminated string for the file name, even when there is no |
| -@code{d_reclen} field in @code{struct dirent} and the file |
| -name is the maximum allowed size. Modern systems all have the |
| -@code{d_reclen} field, and on old systems multi-threading is not |
| -critical. In any case there is no such problem with the @code{readdir} |
| -function, so that even on systems without the @code{d_reclen} member one |
| -could use multiple threads by using external locking. |
| - |
| It is also important to look at the definition of the @code{struct |
| dirent} type. Simply passing a pointer to an object of this type for |
| the second parameter of @code{readdir_r} might not be enough. Some |
| diff --git a/sysdeps/posix/dirstream.h b/sysdeps/posix/dirstream.h |
| index a7a074d..8e8570d 100644 |
| |
| |
| @@ -39,6 +39,8 @@ struct __dirstream |
| |
| off_t filepos; /* Position of next entry to read. */ |
| |
| + int errcode; /* Delayed error code. */ |
| + |
| /* Directory block. */ |
| char data[0] __attribute__ ((aligned (__alignof__ (void*)))); |
| }; |
| diff --git a/sysdeps/posix/opendir.c b/sysdeps/posix/opendir.c |
| index ddfc3a7..fc05b0f 100644 |
| |
| |
| @@ -231,6 +231,7 @@ __alloc_dir (int fd, bool close_fd, int flags, const struct stat64 *statp) |
| dirp->size = 0; |
| dirp->offset = 0; |
| dirp->filepos = 0; |
| + dirp->errcode = 0; |
| |
| return dirp; |
| } |
| diff --git a/sysdeps/posix/readdir_r.c b/sysdeps/posix/readdir_r.c |
| index b5a8e2e..8ed5c3f 100644 |
| |
| |
| @@ -40,6 +40,7 @@ __READDIR_R (DIR *dirp, DIRENT_TYPE *entry, DIRENT_TYPE **result) |
| DIRENT_TYPE *dp; |
| size_t reclen; |
| const int saved_errno = errno; |
| + int ret; |
| |
| __libc_lock_lock (dirp->lock); |
| |
| @@ -70,10 +71,10 @@ __READDIR_R (DIR *dirp, DIRENT_TYPE *entry, DIRENT_TYPE **result) |
| bytes = 0; |
| __set_errno (saved_errno); |
| } |
| + if (bytes < 0) |
| + dirp->errcode = errno; |
| |
| dp = NULL; |
| - /* Reclen != 0 signals that an error occurred. */ |
| - reclen = bytes != 0; |
| break; |
| } |
| dirp->size = (size_t) bytes; |
| @@ -106,29 +107,46 @@ __READDIR_R (DIR *dirp, DIRENT_TYPE *entry, DIRENT_TYPE **result) |
| dirp->filepos += reclen; |
| #endif |
| |
| - /* Skip deleted files. */ |
| +#ifdef NAME_MAX |
| + if (reclen > offsetof (DIRENT_TYPE, d_name) + NAME_MAX + 1) |
| + { |
| + /* The record is very long. It could still fit into the |
| + caller-supplied buffer if we can skip padding at the |
| + end. */ |
| + size_t namelen = _D_EXACT_NAMLEN (dp); |
| + if (namelen <= NAME_MAX) |
| + reclen = offsetof (DIRENT_TYPE, d_name) + namelen + 1; |
| + else |
| + { |
| + /* The name is too long. Ignore this file. */ |
| + dirp->errcode = ENAMETOOLONG; |
| + dp->d_ino = 0; |
| + continue; |
| + } |
| + } |
| +#endif |
| + |
| + /* Skip deleted and ignored files. */ |
| } |
| while (dp->d_ino == 0); |
| |
| if (dp != NULL) |
| { |
| -#ifdef GETDENTS_64BIT_ALIGNED |
| - /* The d_reclen value might include padding which is not part of |
| - the DIRENT_TYPE data structure. */ |
| - reclen = MIN (reclen, |
| - offsetof (DIRENT_TYPE, d_name) + sizeof (dp->d_name)); |
| -#endif |
| *result = memcpy (entry, dp, reclen); |
| -#ifdef GETDENTS_64BIT_ALIGNED |
| +#ifdef _DIRENT_HAVE_D_RECLEN |
| entry->d_reclen = reclen; |
| #endif |
| + ret = 0; |
| } |
| else |
| - *result = NULL; |
| + { |
| + *result = NULL; |
| + ret = dirp->errcode; |
| + } |
| |
| __libc_lock_unlock (dirp->lock); |
| |
| - return dp != NULL ? 0 : reclen ? errno : 0; |
| + return ret; |
| } |
| |
| #ifdef __READDIR_R_ALIAS |
| diff --git a/sysdeps/posix/rewinddir.c b/sysdeps/posix/rewinddir.c |
| index 2935a8e..d4991ad 100644 |
| |
| |
| @@ -33,6 +33,7 @@ rewinddir (dirp) |
| dirp->filepos = 0; |
| dirp->offset = 0; |
| dirp->size = 0; |
| + dirp->errcode = 0; |
| #ifndef NOT_IN_libc |
| __libc_lock_unlock (dirp->lock); |
| #endif |
| diff --git a/sysdeps/unix/sysv/linux/i386/readdir64_r.c b/sysdeps/unix/sysv/linux/i386/readdir64_r.c |
| index 8ebbcfd..a7d114e 100644 |
| |
| |
| @@ -18,7 +18,6 @@ |
| #define __READDIR_R __readdir64_r |
| #define __GETDENTS __getdents64 |
| #define DIRENT_TYPE struct dirent64 |
| -#define GETDENTS_64BIT_ALIGNED 1 |
| |
| #include <sysdeps/posix/readdir_r.c> |
| |
| diff --git a/sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c b/sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c |
| index 5ed8e95..290f2c8 100644 |
| |
| |
| @@ -1,5 +1,4 @@ |
| #define readdir64_r __no_readdir64_r_decl |
| -#define GETDENTS_64BIT_ALIGNED 1 |
| #include <sysdeps/posix/readdir_r.c> |
| #undef readdir64_r |
| weak_alias (__readdir_r, readdir64_r) |