Re: heap-use-after-free in rpl_glob

[Paul Eggert <eggert@cs.ucla.edu>]

[-- Attachment #1: Type: text/plain, Size: 401 bytes --]

On 1/17/20 9:00 AM, Bruno Haible wrote:
> Thank you very much for this report! I expect that the fix will also need
> to go into glibc.

Yes, thanks, I filed a bug report here:

https://sourceware.org/bugzilla/show_bug.cgi?id=25414

It seems that your patch is incomplete, as there's a memory leak in the 
unlikely case that the malloc call fails. Something like the attached 
patch instead, perhaps?

[-- Attachment #2: 0001-glob-Fix-use-after-free-bug.patch --]
[-- Type: text/x-patch, Size: 3328 bytes --]

From 8d5f7d7e43b8830acde4f82b93cc7b970d9ec435 Mon Sep 17 00:00:00 2001
From: Bruno Haible <bruno@clisp.org>
Date: Fri, 17 Jan 2020 11:51:26 -0800
Subject: [PATCH] glob: Fix use-after-free bug.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Reported by Tim Rühsen <tim.ruehsen@gmx.de> in
<https://lists.gnu.org/archive/html/bug-gnulib/2020-01/msg00102.html>.
* lib/glob.c (__glob): Delay freeing dirname until after the use of
end_name.
---
 ChangeLog  |  9 +++++++++
 lib/glob.c | 21 +++++++++++++--------
 2 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 643dba312..04b139a6a 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2020-01-17  Bruno Haible  <bruno@clisp.org>
+	    Paul Eggert  <eggert@cs.ucla.edu>
+
+	glob: Fix use-after-free bug.
+	Reported by Tim Rühsen <tim.ruehsen@gmx.de> in
+	<https://lists.gnu.org/archive/html/bug-gnulib/2020-01/msg00102.html>.
+	* lib/glob.c (__glob): Delay freeing dirname until after the use of
+	end_name.
+
 2020-01-16  Siddhesh Poyarekar  <siddhesh@gotplt.org>
 
 	vcs-to-changelog: Fix parsing of fndecl without args.
diff --git a/lib/glob.c b/lib/glob.c
index a67cbb67e..e0928312a 100644
--- a/lib/glob.c
+++ b/lib/glob.c
@@ -843,24 +843,27 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
               {
                 size_t home_len = strlen (p->pw_dir);
                 size_t rest_len = end_name == NULL ? 0 : strlen (end_name);
+                /* dirname contains end_name; we can't free it now.  */
+                char *prev_dirname =
+                  (__glibc_unlikely (malloc_dirname) ? dirname : NULL);
                 char *d;
 
-                if (__glibc_unlikely (malloc_dirname))
-                  free (dirname);
-                malloc_dirname = 0;
-
                 if (glob_use_alloca (alloca_used, home_len + rest_len + 1))
-                  dirname = alloca_account (home_len + rest_len + 1,
-                                            alloca_used);
+                  {
+                    dirname = alloca_account (home_len + rest_len + 1,
+                                              alloca_used);
+                    malloc_dirname = 0;
+                  }
                 else
                   {
-                    dirname = malloc (home_len + rest_len + 1);
-                    if (dirname == NULL)
+                    char *new_dirname = malloc (home_len + rest_len + 1);
+                    if (new_dirname == NULL)
                       {
                         scratch_buffer_free (&pwtmpbuf);
                         retval = GLOB_NOSPACE;
                         goto out;
                       }
+                    dirname = new_dirname;
                     malloc_dirname = 1;
                   }
                 d = mempcpy (dirname, p->pw_dir, home_len);
@@ -868,6 +871,8 @@ __glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
                   d = mempcpy (d, end_name, rest_len);
                 *d = '\0';
 
+                free (prev_dirname);
+
                 dirlen = home_len + rest_len;
                 dirname_modified = 1;
               }
-- 
2.24.1