[libiberty] Fix read buffer overflow in split_directories()

Message ID e542f40a-8f73-7ab7-11af-2c62263a1897@gmx.de
State New
Headers show
Series
  • [libiberty] Fix read buffer overflow in split_directories()
Related show

Commit Message

Tim Rühsen Nov. 9, 2019, 11:18 p.m.
In line
  if (dirs[num_dirs - 1] == NULL)
'num_dirs' can be 0.

Regards, Tim

Comments

Orlando Arias Nov. 10, 2019, 4:51 a.m. | #1
Greetings

On 11/9/19 6:18 PM, Tim Rühsen wrote:
> In line

>   if (dirs[num_dirs - 1] == NULL)

> 'num_dirs' can be 0.

> 

> Regards, Tim

> 


I've looked at the control-flow on this file and I do not see a possible
path for num_dirs to be 0 at line 186, unless the file name in question
is the empty string "", at which point it may just be easier to return
earlier. Can you please verify this?

Thank you.

Cheers,
Orlando.
Tim Rühsen Nov. 10, 2019, 10:54 a.m. | #2
On 10.11.19 05:51, Orlando Arias wrote:
> Greetings

> 

> On 11/9/19 6:18 PM, Tim Rühsen wrote:

>> In line

>>   if (dirs[num_dirs - 1] == NULL)

>> 'num_dirs' can be 0.

>>

>> Regards, Tim

>>

> 

> I've looked at the control-flow on this file and I do not see a possible

> path for num_dirs to be 0 at line 186, unless the file name in question

> is the empty string "", at which point it may just be easier to return

> earlier. Can you please verify this?


Yes, I can confirm. Thanks for review !

Returning early on an empty 'name' also fixes another read overflow in

#ifdef HAVE_DOS_BASED_FILE_SYSTEM
  if (name[1] == ':' && IS_DIR_SEPARATOR (name[2]))
    {
      p += 3;
      num_dirs++;
    }
#endif /* HAVE_DOS_BASED_FILE_SYSTEM */

Patch v2 appended.

Regards, Tim
From 848cf430c4393555f35f4e4d27da8a555f6308c3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tim=20R=C3=BChsen?= <tim.ruehsen@gmx.de>

Date: Sun, 10 Nov 2019 11:51:00 +0100
Subject: [PATCH] [libiberty] Fix read buffer overflow in split_directories()

* libiberty/make-relative-prefix.c: Return early on empty 'name'
---
 libiberty/make-relative-prefix.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/libiberty/make-relative-prefix.c b/libiberty/make-relative-prefix.c
index ec0b0ee749..2ff2af8a59 100644
--- a/libiberty/make-relative-prefix.c
+++ b/libiberty/make-relative-prefix.c
@@ -122,6 +122,9 @@ split_directories (const char *name, int *ptr_num_dirs)
   const char *p, *q;
   int ch;
 
+  if (!*name)
+    return NULL;
+
   /* Count the number of directories.  Special case MSDOS disk names as part
      of the initial directory.  */
   p = name;
-- 
2.24.0
Orlando Arias Nov. 10, 2019, 5:33 p.m. | #3
Greetings,

On 11/10/19 5:54 AM, Tim Rühsen wrote:
> Yes, I can confirm. Thanks for review !


Thank you for checking. Please note that I am not a binutils maintainer
[or reviewer for that matter]. I am just an interested party. Having
said that

> Returning early on an empty 'name' also fixes another read overflow in

> 

> #ifdef HAVE_DOS_BASED_FILE_SYSTEM

>   if (name[1] == ':' && IS_DIR_SEPARATOR (name[2]))

>     {

>       p += 3;

>       num_dirs++;

>     }

> #endif /* HAVE_DOS_BASED_FILE_SYSTEM */

> 

> Patch v2 appended.

> 


I believe you also need to do something about the ptr_num_dirs that the
caller may work with at some point. It would also be good to check how
this function is called within binutils [and other projects that use
libiberty].

Thank you for your time and efforts.

Cheers,
Orlando.
Tim Rühsen Nov. 10, 2019, 5:41 p.m. | #4
On 10.11.19 18:33, Orlando Arias wrote:
> Greetings,

> 

> On 11/10/19 5:54 AM, Tim Rühsen wrote:

>> Yes, I can confirm. Thanks for review !

> 

> Thank you for checking. Please note that I am not a binutils maintainer

> [or reviewer for that matter]. I am just an interested party. Having

> said that

> 

>> Returning early on an empty 'name' also fixes another read overflow in

>>

>> #ifdef HAVE_DOS_BASED_FILE_SYSTEM

>>   if (name[1] == ':' && IS_DIR_SEPARATOR (name[2]))

>>     {

>>       p += 3;

>>       num_dirs++;

>>     }

>> #endif /* HAVE_DOS_BASED_FILE_SYSTEM */

>>

>> Patch v2 appended.

>>

> 

> I believe you also need to do something about the ptr_num_dirs that the

> caller may work with at some point. It would also be good to check how

> this function is called within binutils [and other projects that use

> libiberty].

> 

> Thank you for your time and efforts.


Thank you for looking into it :-)

The callers of split_directories() check for the return value. If NULL,
they 'goto bailout' (cleanup and return), and ptr_num_dirs is definitely
not used.

Regards, Tim
Nick Clifton Nov. 11, 2019, 11:42 a.m. | #5
Hi Tim,

> Yes, I can confirm. Thanks for review !


> Patch v2 appended.


Just to be clear: patches for libiberty need to be sent to gcc-patches@gcc.gnu.org
rather than this mailing list. (You can of course discuss the patches here if you
want to).  The libiberty library is maintained by gcc rather then the binutils...

Cheers
  Nick
Tim Rühsen Nov. 11, 2019, 12:05 p.m. | #6
Hi Nick,

On 11/11/19 12:42 PM, Nick Clifton wrote:
>> Patch v2 appended.

>

> Just to be clear: patches for libiberty need to be sent to gcc-patches@gcc.gnu.org

> rather than this mailing list. (You can of course discuss the patches here if you

> want to).  The libiberty library is maintained by gcc rather then the binutils...


Thanks, this is clear since one of your last emails - I oversaw that in
the README. Sorry :-)

Regards, Tim

Patch

From 65ca7c0688cb823ebde141c7cf74075db7cd0aa0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tim=20R=C3=BChsen?= <tim.ruehsen@gmx.de>
Date: Sun, 10 Nov 2019 00:12:44 +0100
Subject: [PATCH] [libiberty] Fix read buffer overflow in split_directories()

* libiberty/make-relative-prefix.c: Check array index before use
---
 libiberty/make-relative-prefix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libiberty/make-relative-prefix.c b/libiberty/make-relative-prefix.c
index ec0b0ee749..d53304eca7 100644
--- a/libiberty/make-relative-prefix.c
+++ b/libiberty/make-relative-prefix.c
@@ -186,7 +186,7 @@  split_directories (const char *name, int *ptr_num_dirs)
     dirs[num_dirs++] = save_string (q, p - 1 - q);
   dirs[num_dirs] = NULL;
 
-  if (dirs[num_dirs - 1] == NULL)
+  if (num_dirs && dirs[num_dirs - 1] == NULL)
     {
       free_split_directories (dirs);
       return NULL;
-- 
2.24.0