GNU bug report logs - #19890
[PATCH] Use after free in dld_link on error path

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 19890 in the body.
You can then email your comments to 19890 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Tobias Stoeckmann <tobias <at> stoeckmann.org>
To: bug-libtool <at> gnu.org
Subject: [PATCH] Use after free in dld_link on error path
Date: Tue, 17 Feb 2015 22:42:45 +0100

When dld_link fails, the allocated memory for module (strdup) is
freed, but still returned by vm_open. vm_open is called in
ltdr.c line 444, which checks the error flag only if result is NULL.

Therefore, the error condition of vm_open is ignored and the memory
pointed to by module is used later on.

While fixing this, also set error condition and return NULL if
strdup is unable to allocate memory.
---
 libltdl/loaders/dld_link.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/libltdl/loaders/dld_link.c b/libltdl/loaders/dld_link.c
index a73880f..e95d5e4 100644
--- a/libltdl/loaders/dld_link.c
+++ b/libltdl/loaders/dld_link.c
@@ -111,11 +111,15 @@ vm_open (lt_user_data loader_data LT__UNUSED, const char *filename,
          lt_dladvise advise LT__UNUSED)
 {
   lt_module module = lt__strdup (filename);
-
-  if (dld_link (filename) != 0)
+  if (module == NULL)
+    {
+      LT__SETERROR (NO_MEMORY);
+    }
+  else if (dld_link (filename) != 0)
     {
       LT__SETERROR (CANNOT_OPEN);
       FREE (module);
+      module = NULL;
     }
 
   return module;
-- 
2.3.0

Message #8 received at 19890 <at> debbugs.gnu.org (full text, mbox):

From: Tobias Stoeckmann <tobias <at> stoeckmann.org>
To: 19890 <at> debbugs.gnu.org
Subject: [PATCH] Check for strdup NULL return value
Date: Tue, 17 Feb 2015 22:59:55 +0100

In contrast to previous mail, only check for strdup NULL value.
The FREE macro properly sets module to NULL, therefore this one is
the only issue left.
---
Sorry for the previous noise.
---
 libltdl/loaders/dld_link.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/libltdl/loaders/dld_link.c b/libltdl/loaders/dld_link.c
index a73880f..54053ee 100644
--- a/libltdl/loaders/dld_link.c
+++ b/libltdl/loaders/dld_link.c
@@ -112,7 +112,11 @@ vm_open (lt_user_data loader_data LT__UNUSED, const char *filename,
 {
   lt_module module = lt__strdup (filename);
 
-  if (dld_link (filename) != 0)
+  if (module == NULL)
+    {
+      LT__SETERROR (NO_MEMORY);
+    }
+  else if (dld_link (filename) != 0)
     {
       LT__SETERROR (CANNOT_OPEN);
       FREE (module);
-- 
2.3.0

Message #11 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Pavel Raiskup <praiskup <at> redhat.com>
To: bug-libtool <at> gnu.org
Cc: Tobias Stoeckmann <tobias <at> stoeckmann.org>, 19890 <at> debbugs.gnu.org
Subject: Re: bug#19890: [PATCH] Check for strdup NULL return value
Date: Fri, 12 Feb 2016 16:12:57 +0100

Thanks for the report, Tobias.

On Tuesday 17 of February 2015 22:59:55 Tobias Stoeckmann wrote:
> -  if (dld_link (filename) != 0)
> +  if (module == NULL)
> +    {
> +      LT__SETERROR (NO_MEMORY);
> +    }

There is 'lt__alloc_die = lt__alloc_die_callback;' in ltdl.c (properly
setting the lasterror).  Can you verify that this patch is good enough?

|  libltdl: handle ENOMEM sooner
|
|  * libltdl/loaders/dld_link.c (vm_open): Do not even try dld_link()
|  in case of ENOMEM.
|
|  diff --git a/libltdl/loaders/dld_link.c b/libltdl/loaders/dld_link.c
|  index a73880f..0edf0df 100644
|  --- a/libltdl/loaders/dld_link.c
|  +++ b/libltdl/loaders/dld_link.c
|  @@ -112,7 +112,7 @@ vm_open (lt_user_data loader_data LT__UNUSED, const char *filename,
|   {
|     lt_module module = lt__strdup (filename);
|
|  -  if (dld_link (filename) != 0)
|  +  if (module && dld_link (filename) != 0)
|       {
|         LT__SETERROR (CANNOT_OPEN);
|         FREE (module);

Pavel

Message #17 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Tobias Stoeckmann <tobias <at> stoeckmann.org>
To: Pavel Raiskup <praiskup <at> redhat.com>
Cc: bug-libtool <at> gnu.org, 19890 <at> debbugs.gnu.org
Subject: Re: bug#19890: [PATCH] Check for strdup NULL return value
Date: Fri, 12 Feb 2016 21:00:18 +0100

On Fri, Feb 12, 2016 at 04:12:57PM +0100, Pavel Raiskup wrote:
> There is 'lt__alloc_die = lt__alloc_die_callback;' in ltdl.c (properly
> setting the lasterror).  Can you verify that this patch is good enough?

Yes, that looks reasonable to me. Thanks for pointing that out!


Tobias

Message #23 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Pavel Raiskup <praiskup <at> redhat.com>
To: bug-libtool <at> gnu.org
Cc: control <at> debbugs.gnu.org, Tobias Stoeckmann <tobias <at> stoeckmann.org>,
 19890 <at> debbugs.gnu.org
Subject: Re: bug#19890: [PATCH] Check for strdup NULL return value
Date: Mon, 15 Feb 2016 15:30:17 +0100

close 19890 libtool-2.4.6.25
thanks

On Friday 12 of February 2016 21:00:18 Tobias Stoeckmann wrote:
> On Fri, Feb 12, 2016 at 04:12:57PM +0100, Pavel Raiskup wrote:
> > There is 'lt__alloc_die = lt__alloc_die_callback;' in ltdl.c (properly
> > setting the lasterror).  Can you verify that this patch is good enough?
> 
> Yes, that looks reasonable to me. Thanks for pointing that out!

Pushed as b5d44b844703.

Pavel

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.