Re: [PATCH] Fix use after free in closedir

[Federico Terraneo <fede dot tft at hotmail dot it>]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11/13/2013 04:17 PM, Corinna Vinschen wrote:
> 
> Thanks!  I think you found something very suspicious here.  The
> test for fd != -1 is supposed to check... what?
> 
> Opendir fails if open on the directory fails, so a valid DIR* also
> has a valid dd_fd value.  The only time dd_fd can be -1
> programatically is when closedir has been called and in this case
> DIR* is invalid, too. Or, if two threads call closedir for the same
> DIR* at about the same time, one of them could find that dd_fd is
> -1 and skip the close/free stuff.  However, the code
> 
> fd = dirp->dd_fd; if (fd != -1) { dirp->dd_fd = -1;
> 
> is definitely not thread-safe, and closedir is not *supposed* to
> be reentrant or thread-safe.

Indeed, having a closer look at the opendir logic, I agree that dd_fd
can never be -1, and the the code is definitely not thread safe
anyways unless HAVE_DD_LOCK is defined.

> 
> This test doesn't make sense.  libc/sys/sparc64/closedir.c and 
> libc/sys/sysvi386/closedir.c both don't perform it either.
> 
> What I would suggest is to simplify the call to this code instead:
> 
> int _DEFUN(closedir, (dirp), register DIR *dirp) { int fd, rc;
> 
> #ifdef HAVE_DD_LOCK __lock_acquire_recursive(dirp->dd_lock); 
> #endif fd = dirp->dd_fd; rc = close(fd); _cleanupdir(dirp); 
> (void)free((void *)dirp->dd_buf); #ifdef HAVE_DD_LOCK 
> __lock_release_recursive(dirp->dd_lock); 
> __lock_close_recursive(dirp->dd_lock); #endif (void)free((void
> *)dirp); return rc; }
> 
> Does that make sense?  Did I miss something?
> 

Yes, but I would remove useles tests for -1 also in readdir.c and
readdir_r.c, see the attached patch.

2013-11-13  Terraneo Federico  <fede.tft@hotmail.it>

	* libc/posix/closedir.c: Fix use after free.
	  Remove useless test dd_fd != -1
	* libc/posix/readdir.c: Remove useless test dd_fd == -1
	* libc/posix/readdir_r.c: Ditto.

> 
> Thanks, Corinna
> 

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJSg5/FAAoJECkLFtN5Xr9fv4oH/2uShvGWKYd36t3Oa6qgM6kc
FCAeKulz45oxfPv5BknKK9tzGc0ReEK/n2keSFj0ibFh/MiOBWZIl9XWaItGwIPC
EV2BkYBtybHTQuHpBCvCyM56mA1m/zHExVNy5ypVKCMnG6MnmrlqBIQqKwufs2sU
qrBQ+znCaN54zxLKBd3KVx1KYeWd84v1cSWrvKaYRK0ywa4yWzCE1o9hMJmGDoyz
TkLFj2OUpSgI8z3S1D5HdoT8RjSQpIV+o3GK9VDfZaNrcrQIvCP9AfHr1fUAt13+
rLjMNFEwr79VqjqdDA3e+EzuSpLqctF0pMkvu+D/KIL6sSiaNtXwQeQrC/Xuzz4=
=zW7h
-----END PGP SIGNATURE-----

diff -ruN newlib-old/newlib/libc/posix/closedir.c newlib/newlib/libc/posix/closedir.c
--- newlib-old/newlib/libc/posix/closedir.c	2003-06-06 21:57:51.000000000 +0200
+++ newlib/newlib/libc/posix/closedir.c	2013-11-13 16:41:27.277621610 +0100
@@ -52,25 +52,19 @@
 _DEFUN(closedir, (dirp),
        register DIR *dirp)
 {
-	int fd, rc;
+	int rc;
 
 #ifdef HAVE_DD_LOCK
 	__lock_acquire_recursive(dirp->dd_lock);
 #endif
-	rc = 0;
-	fd = dirp->dd_fd;
-	if (fd != -1) {
-		dirp->dd_fd = -1;
-		dirp->dd_loc = 0;
-		(void)free((void *)dirp->dd_buf);
-		(void)free((void *)dirp);
-		rc = close(fd);
-		_cleanupdir(dirp);
-	}
+	rc = close(dirp->dd_fd);
+	_cleanupdir(dirp);
+	free((void *)dirp->dd_buf);
 #ifdef HAVE_DD_LOCK
 	__lock_release_recursive(dirp->dd_lock);
 	__lock_close_recursive(dirp->dd_lock);
 #endif
+	free((void *)dirp);
 	return rc;
 }
 
diff -ruN newlib-old/newlib/libc/posix/readdir.c newlib/newlib/libc/posix/readdir.c
--- newlib-old/newlib/libc/posix/readdir.c	2012-07-20 18:55:19.000000000 +0200
+++ newlib/newlib/libc/posix/readdir.c	2013-11-13 16:43:11.686139336 +0100
@@ -53,9 +53,6 @@
 #ifdef HAVE_DD_LOCK
   __lock_acquire_recursive(dirp->dd_lock);
 #endif
-
-  if (dirp->dd_fd == -1)
-    return NULL;
  
   for (;;) {
     if (dirp->dd_loc == 0) {
diff -ruN newlib-old/newlib/libc/posix/readdir_r.c newlib/newlib/libc/posix/readdir_r.c
--- newlib-old/newlib/libc/posix/readdir_r.c	2013-06-19 17:54:20.000000000 +0200
+++ newlib/newlib/libc/posix/readdir_r.c	2013-11-13 16:42:00.605786873 +0100
@@ -60,11 +60,6 @@
 #ifdef HAVE_DD_LOCK
   __lock_acquire_recursive(dirp->dd_lock);
 #endif
-
-  if (dirp->dd_fd == -1) {
-    *dpp = NULL;
-    return errno = EBADF;
-  }
  
   for (;;) {
     if (dirp->dd_loc == 0) {

Attachment: closedir.patch.sig
Description: Binary data