Undefined behavior in lib/canonicalize-lgpl.c

Undefined behavior in lib/canonicalize-lgpl.c

[Florian Weimer]

We have received a report that the glibc realpath implementation
exhibits undefined behavior:

  <https://sourceware.org/bugzilla/show_bug.cgi?id=26592>

In gnulib, the code is in lib/canonicalize-lgpl.c:

    234           if (!ISSLASH (dest[-1]))
    235             *dest++ = '/';
    236 
    237           if (dest + (end - start) >= rpath_limit)
    238             {
    239               ptrdiff_t dest_offset = dest - rpath;
    240               char *new_rpath;

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

Re: Undefined behavior in lib/canonicalize-lgpl.c

[Adhemerval Zanella]

On 10/09/2020 05:32, Florian Weimer wrote:
> We have received a report that the glibc realpath implementation
> exhibits undefined behavior:
> 
>   <https://sourceware.org/bugzilla/show_bug.cgi?id=26592>
> 
> In gnulib, the code is in lib/canonicalize-lgpl.c:
> 
>     234           if (!ISSLASH (dest[-1]))
>     235             *dest++ = '/';
>     236 
>     237           if (dest + (end - start) >= rpath_limit)
>     238             {
>     239               ptrdiff_t dest_offset = dest - rpath;
>     240               char *new_rpath;

I think the lib/canonicalize.c is also affected:

204           if (!ISSLASH (dest[-1]))                                                                   
205             *dest++ = '/';                                                                           
206 
207           if (dest + (end - start) >= rname_limit)                                                   
208             {                                                                                        
209               ptrdiff_t dest_offset = dest - rname;                                                  
210               size_t new_size = rname_limit - rname;


On glibc side I will check if BZ#10635 is still applicable (to basically
sync gnulib and glibc implementation) and send some other realtime fixes
as well (BZ #26241, BZ #26592, and BZ #24970).

Re: Undefined behavior in lib/canonicalize-lgpl.c

[Paul Eggert]

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

On 9/10/20 6:34 AM, Adhemerval Zanella wrote:
> I think the lib/canonicalize.c is also affected:

Thanks for the heads-ups. I installed the attached patch to Gnulib, which fixes 
a related bug I noticed while in the neighborhood (I don't think glibc has this 
bug).

This patch uses the expression (rpath_limit - dest <= end - start) which I found 
a little easier to grok.

[-- Attachment #2: 0001-canonicalize-fix-pointer-indexing-bugs.patch --]
[-- Type: text/x-patch, Size: 3266 bytes --]

From d468be5b5950bc5f2e0a5a4fbaeb0ea6a88c4c9f Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Thu, 10 Sep 2020 14:25:51 -0700
Subject: [PATCH] canonicalize: fix pointer indexing bugs

Problem reported by Florian Weimer in:
https://lists.gnu.org/r/bug-gnulib/2020-09/msg00025.html
* lib/canonicalize-lgpl.c (__realpath):
* lib/canonicalize.c (canonicalize_filename_mode):
Do not generate a pointer past the end of the array.
* lib/canonicalize.c (canonicalize_filename_mode):
Do not use a pointer after passing it to realloc.
---
 ChangeLog               | 11 +++++++++++
 lib/canonicalize-lgpl.c |  2 +-
 lib/canonicalize.c      | 19 ++++++++-----------
 3 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 8b6f62bb1..bf39cc512 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,14 @@
+2020-09-10  Paul Eggert  <eggert@cs.ucla.edu>
+
+	canonicalize: fix pointer indexing bugs
+	Problem reported by Florian Weimer in:
+	https://lists.gnu.org/r/bug-gnulib/2020-09/msg00025.html
+	* lib/canonicalize-lgpl.c (__realpath):
+	* lib/canonicalize.c (canonicalize_filename_mode):
+	Do not generate a pointer past the end of the array.
+	* lib/canonicalize.c (canonicalize_filename_mode):
+	Do not use a pointer after passing it to realloc.
+
 2020-09-09  Paul Eggert  <eggert@cs.ucla.edu>
 
 	tempname: help merge with glibc
diff --git a/lib/canonicalize-lgpl.c b/lib/canonicalize-lgpl.c
index 0b89d2a18..cc42662db 100644
--- a/lib/canonicalize-lgpl.c
+++ b/lib/canonicalize-lgpl.c
@@ -234,7 +234,7 @@ __realpath (const char *name, char *resolved)
           if (!ISSLASH (dest[-1]))
             *dest++ = '/';
 
-          if (dest + (end - start) >= rpath_limit)
+          if (rpath_limit - dest <= end - start)
             {
               ptrdiff_t dest_offset = dest - rpath;
               char *new_rpath;
diff --git a/lib/canonicalize.c b/lib/canonicalize.c
index 8bb325414..aa0c3bd28 100644
--- a/lib/canonicalize.c
+++ b/lib/canonicalize.c
@@ -138,18 +138,15 @@ canonicalize_filename_mode (const char *name, canonicalize_mode_t can_mode)
       rname = xgetcwd ();
       if (!rname)
         return NULL;
-      dest = strchr (rname, '\0');
-      if (dest - rname < PATH_MAX)
+      size_t rnamelen = strlen (rname);
+      size_t rnamesize = rnamelen;  /* Lower bound on size; good enough.  */
+      if (rnamesize < PATH_MAX)
         {
-          char *p = xrealloc (rname, PATH_MAX);
-          dest = p + (dest - rname);
-          rname = p;
-          rname_limit = rname + PATH_MAX;
-        }
-      else
-        {
-          rname_limit = dest;
+          rnamesize = PATH_MAX;
+          rname = xrealloc (rname, rnamesize);
         }
+      dest = rname + rnamelen;
+      rname_limit = rname + rnamesize;
       start = name;
       prefix_len = FILE_SYSTEM_PREFIX_LEN (rname);
     }
@@ -204,7 +201,7 @@ canonicalize_filename_mode (const char *name, canonicalize_mode_t can_mode)
           if (!ISSLASH (dest[-1]))
             *dest++ = '/';
 
-          if (dest + (end - start) >= rname_limit)
+          if (rname_limit - dest <= end - start)
             {
               ptrdiff_t dest_offset = dest - rname;
               size_t new_size = rname_limit - rname;
-- 
2.17.1