bluetoothd failure after a "malloc retun NULL" injection (attachment fix)

bluetoothd failure after a "malloc retun NULL" injection (attachment fix)

[Oprisenescu, CatalinX]

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

Hello,

Following is an issue encountered and handled using bluez-5.10 on a 32bit Tizen 2.0 IVI based distribution.

If malloc returns a random NULL bluetoothd exits with core dumped.
In this case we do the following:
      -start bluetoothd,
      -at a random malloc call from any library (*lib*.so*), a random NULL will be returned by bluetoothd with a ld_preloaded library.

~# LD_PRELOAD=/root/lib_wrapper.so /usr/libexec/bluetooth/bluetoothd -E

Analyzing the dump, it points directly to a DBUS_ERROR_NO_MEMORY which, after handling, keeps bluetoothd from dumping and allowing it to return the fatal error occurred as exit status, thus failing gracefully.

A fix proposal, handling the use-case is attached.


Regards,
Calin Oprisenescu.
--------------------------------------------------------------
Intel Shannon Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263
Business address: Dromore House, East Park, Shannon, Co. Clare

This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.


[-- Attachment #2: 0001-Provide-fix-for-malloc-null-injection.patch --]
[-- Type: application/octet-stream, Size: 967 bytes --]

From 567732c84278f94fc3d8c749c0e40de349e9f00a Mon Sep 17 00:00:00 2001
From: Calin Oprisenescu <catalinx.oprisenescu@intel.com>
Date: Tue, 5 Nov 2013 12:27:20 +0200
Subject: [PATCH] Provide fix for malloc null injection ASD100001565

Change-Id: I7a249f815335e65ae6771d5d1ff58497dbdbb7db
Author: Calin Oprisenescu <catalinx.oprisenescu@intel.com>
Committer: Calin Oprisenescu <catalinx.oprisenescu@intel.com>
---
 src/main.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/main.c b/src/main.c
index 91d90b4..3bf2ae4 100644
--- a/src/main.c
+++ b/src/main.c
@@ -427,6 +427,10 @@ static int connect_dbus(void)
 	conn = g_dbus_setup_bus(DBUS_BUS_SYSTEM, BLUEZ_NAME, &err);
 	if (!conn) {
 		if (dbus_error_is_set(&err)) {
+			if (dbus_error_has_name (&err, DBUS_ERROR_NO_MEMORY)) {
+				dbus_error_free(&err);
+				return -ENOMEM;
+			}
 			g_printerr("D-Bus setup failed: %s\n", err.message);
 			dbus_error_free(&err);
 			return -EIO;
-- 
1.8.4.msysgit.0

Re: bluetoothd failure after a "malloc retun NULL" injection (attachment fix)

[Luiz Augusto von Dentz]

Hi,

On Wed, Nov 13, 2013 at 4:09 PM, Oprisenescu, CatalinX
<catalinx.oprisenescu@intel.com> wrote:
> Hello,
>
> Following is an issue encountered and handled using bluez-5.10 on a 32bit Tizen 2.0 IVI based distribution.
>
> If malloc returns a random NULL bluetoothd exits with core dumped.
> In this case we do the following:
>       -start bluetoothd,
>       -at a random malloc call from any library (*lib*.so*), a random NULL will be returned by bluetoothd with a ld_preloaded library.
>
> ~# LD_PRELOAD=/root/lib_wrapper.so /usr/libexec/bluetooth/bluetoothd -E
>
> Analyzing the dump, it points directly to a DBUS_ERROR_NO_MEMORY which, after handling, keeps bluetoothd from dumping and allowing it to return the fatal error occurred as exit status, thus failing gracefully.
>
> A fix proposal, handling the use-case is attached.

Im afraid if we start treating all the out of memory cases we wont
have time to do anything else, and quite frankly if this happens for
real the least concern would be bluetoothd, so I believe we should
just treat OOM cases as unrecoverable. Since this can happen with any
dbus_error, I would probably suggest to wrap it perhaps we a
g_dbus_error API that does assert in OOM.

-- 
Luiz Augusto von Dentz

Re: bluetoothd failure after a "malloc retun NULL" injection (attachment fix)

[Marcel Holtmann]

Hi Luiz,

>> Following is an issue encountered and handled using bluez-5.10 on a 32bit Tizen 2.0 IVI based distribution.
>> 
>> If malloc returns a random NULL bluetoothd exits with core dumped.
>> In this case we do the following:
>>      -start bluetoothd,
>>      -at a random malloc call from any library (*lib*.so*), a random NULL will be returned by bluetoothd with a ld_preloaded library.
>> 
>> ~# LD_PRELOAD=/root/lib_wrapper.so /usr/libexec/bluetooth/bluetoothd -E
>> 
>> Analyzing the dump, it points directly to a DBUS_ERROR_NO_MEMORY which, after handling, keeps bluetoothd from dumping and allowing it to return the fatal error occurred as exit status, thus failing gracefully.
>> 
>> A fix proposal, handling the use-case is attached.
> 
> Im afraid if we start treating all the out of memory cases we wont
> have time to do anything else, and quite frankly if this happens for
> real the least concern would be bluetoothd, so I believe we should
> just treat OOM cases as unrecoverable. Since this can happen with any
> dbus_error, I would probably suggest to wrap it perhaps we a
> g_dbus_error API that does assert in OOM.

I am fine with fixing obvious ones. However long-term these tiny sized memory allocation must just abort the program if we run out of memory. Since there is no recovery from it anyway.

Regards

Marcel

Re: bluetoothd failure after a "malloc retun NULL" injection (attachment fix)

[Johan Hedberg]

Hi Marcel,

On Thu, Nov 14, 2013, Marcel Holtmann wrote:
> >> Following is an issue encountered and handled using bluez-5.10 on a
> >> 32bit Tizen 2.0 IVI based distribution.
> >> 
> >> If malloc returns a random NULL bluetoothd exits with core dumped.
> >> In this case we do the following:
> >>      -start bluetoothd,
> >>      -at a random malloc call from any library (*lib*.so*), a random NULL will be returned by bluetoothd with a ld_preloaded library.
> >> 
> >> ~# LD_PRELOAD=/root/lib_wrapper.so /usr/libexec/bluetooth/bluetoothd -E
> >> 
> >> Analyzing the dump, it points directly to a DBUS_ERROR_NO_MEMORY
> >> which, after handling, keeps bluetoothd from dumping and allowing
> >> it to return the fatal error occurred as exit status, thus failing
> >> gracefully.
> >> 
> >> A fix proposal, handling the use-case is attached.
> > 
> > Im afraid if we start treating all the out of memory cases we wont
> > have time to do anything else, and quite frankly if this happens for
> > real the least concern would be bluetoothd, so I believe we should
> > just treat OOM cases as unrecoverable. Since this can happen with any
> > dbus_error, I would probably suggest to wrap it perhaps we a
> > g_dbus_error API that does assert in OOM.
> 
> I am fine with fixing obvious ones. However long-term these tiny sized
> memory allocation must just abort the program if we run out of memory.
> Since there is no recovery from it anyway.

Did you actually take a look at the patch? It's not exactly fixing any
missing error checks but only modifying the behavior if the error is OOM
by not doing a g_printerr call in that case. However, the main()
function still does an error() call when connect_dbus() returns failure
(even if it's -ENOMEM) so I don't really see how this particular patch
would fix any obvious OOM handling issue.

Johan

Re: bluetoothd failure after a "malloc retun NULL" injection (attachment fix)

[Luiz Augusto von Dentz]

Hi Johan,

On Thu, Nov 14, 2013 at 10:19 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Marcel,
>
> On Thu, Nov 14, 2013, Marcel Holtmann wrote:
>> >> Following is an issue encountered and handled using bluez-5.10 on a
>> >> 32bit Tizen 2.0 IVI based distribution.
>> >>
>> >> If malloc returns a random NULL bluetoothd exits with core dumped.
>> >> In this case we do the following:
>> >>      -start bluetoothd,
>> >>      -at a random malloc call from any library (*lib*.so*), a random NULL will be returned by bluetoothd with a ld_preloaded library.
>> >>
>> >> ~# LD_PRELOAD=/root/lib_wrapper.so /usr/libexec/bluetooth/bluetoothd -E
>> >>
>> >> Analyzing the dump, it points directly to a DBUS_ERROR_NO_MEMORY
>> >> which, after handling, keeps bluetoothd from dumping and allowing
>> >> it to return the fatal error occurred as exit status, thus failing
>> >> gracefully.
>> >>
>> >> A fix proposal, handling the use-case is attached.
>> >
>> > Im afraid if we start treating all the out of memory cases we wont
>> > have time to do anything else, and quite frankly if this happens for
>> > real the least concern would be bluetoothd, so I believe we should
>> > just treat OOM cases as unrecoverable. Since this can happen with any
>> > dbus_error, I would probably suggest to wrap it perhaps we a
>> > g_dbus_error API that does assert in OOM.
>>
>> I am fine with fixing obvious ones. However long-term these tiny sized
>> memory allocation must just abort the program if we run out of memory.
>> Since there is no recovery from it anyway.
>
> Did you actually take a look at the patch? It's not exactly fixing any
> missing error checks but only modifying the behavior if the error is OOM
> by not doing a g_printerr call in that case. However, the main()
> function still does an error() call when connect_dbus() returns failure
> (even if it's -ENOMEM) so I don't really see how this particular patch
> would fix any obvious OOM handling issue.

Yep, I was thinking more in the following:

diff --git a/gdbus/gdbus.h b/gdbus/gdbus.h
index 9542109..ced55d9 100644
--- a/gdbus/gdbus.h
+++ b/gdbus/gdbus.h
@@ -216,6 +216,7 @@ struct GDBusSecurityTable {
        .flags = G_DBUS_SIGNAL_FLAG_EXPERIMENTAL

 void g_dbus_set_flags(int flags);
+gboolean g_dbus_error_is_set(DBusError *err);

 gboolean g_dbus_register_interface(DBusConnection *connection,
                                        const char *path, const char *name,
diff --git a/gdbus/object.c b/gdbus/object.c
index b248cbb..699c7e2 100644
--- a/gdbus/object.c
+++ b/gdbus/object.c
@@ -27,6 +27,7 @@

 #include <stdio.h>
 #include <string.h>
+#include <assert.h>

 #include <glib.h>
 #include <dbus/dbus.h>
@@ -1820,3 +1821,16 @@ void g_dbus_set_flags(int flags)
 {
        global_flags = flags;
 }
+
+gboolean g_dbus_error_is_set(DBusError *err)
+{
+       dbus_bool_t ret;
+
+       ret = dbus_error_is_set(err);
+       if (!ret)
+               return FALSE;
+
+       assert(!dbus_error_has_name(err, DBUS_ERROR_NO_MEMORY));
+
+       return TRUE;
+}

But perhaps there is a way to set libdbus to not handle OOM cases and
just assert/exit, the interesting part is that even in dbus code
itself there are comments like this:

     /* FIXME have less lame handling for OOM, we just silently fail to
      * watch.  (In reality though, the whole OOM handling in dbus is
      * stupid but we won't go into that in this comment =) )
      */


-- 
Luiz Augusto von Dentz