2022-05-31 15:47:21

by Gopal Tiwari

Subject: [Bluez V2 10/13] Fixing use after free in src/device.c

From: Gopal Tiwari <[email protected]>

Following traces reported by covirty tool

Error: USE_AFTER_FREE (CWE-416):
bluez-5.64/src/device.c:2962: path: Condition
"!dbus_message_get_args(msg, NULL, 0 /* (int)0 */)", taking false branch.

bluez-5.64/src/device.c:2965: path:
Condition "device->bonding", taking false branch.

bluez-5.64/src/device.c:2968: path:
Condition "device->bredr_state.bonded", taking true branch.

bluez-5.64/src/device.c:2969: path: Falling through to end of
if statement.

bluez-5.64/src/device.c:2977: path: Condition "state->bonded",
taking false branch.

bluez-5.64/src/device.c:2983: path: Condition "agent", taking
true branch.

bluez-5.64/src/device.c:2984: path: Falling through to end of
if statement.

bluez-5.64/src/device.c:2990: path: Condition "agent", taking
true branch.

bluez-5.64/src/device.c:3005: path: Condition "bdaddr_type != 0",
taking true branch.

bluez-5.64/src/device.c:3006: path:

Condition "!state->connected", taking true branch.

bluez-5.64/src/device.c:3006: path:
Condition "btd_le_connect_before_pairing()", taking true branch.
bluez-5.64/src/device.c:3007: freed_arg: "device_connect_le" frees
"device->bonding".

bluez-5.64/src/device.c:3007: path: Falling through to end of
if statement.

bluez-5.64/src/device.c:3012: path: Falling through to end of
if statement.

bluez-5.64/src/device.c:3017: path: Condition "err < 0",
taking true branch.

bluez-5.64/src/device.c:3018: double_free: Calling "bonding_request_free"
frees pointer "device->bonding" which has already been freed.

Signed-off-by: Gopal Tiwari <[email protected]>
---
src/device.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/src/device.c b/src/device.c
index 8dc12d026..a0e5d40db 100644
--- a/src/device.c
+++ b/src/device.c
@@ -2942,6 +2942,7 @@ static void bonding_request_free(struct bonding_req *bonding)
bonding->device->bonding = NULL;

g_free(bonding);
+ bonding = NULL;
}

static DBusMessage *pair_device(DBusConnection *conn, DBusMessage *msg,
--
2.26.2

2022-06-01 19:18:31

by Luiz Augusto von Dentz

Subject: Re: [Bluez V2 10/13] Fixing use after free in src/device.c

Hi Gopal,

On Tue, May 31, 2022 at 12:42 AM Gopal Tiwari
<[email protected]> wrote:
>
> From: Gopal Tiwari <[email protected]>
>
> Following traces reported by covirty tool
>
> Error: USE_AFTER_FREE (CWE-416):
> bluez-5.64/src/device.c:2962: path: Condition
> "!dbus_message_get_args(msg, NULL, 0 /* (int)0 */)", taking false branch.
>
> bluez-5.64/src/device.c:2965: path:
> Condition "device->bonding", taking false branch.
>
> bluez-5.64/src/device.c:2968: path:
> Condition "device->bredr_state.bonded", taking true branch.
>
> bluez-5.64/src/device.c:2969: path: Falling through to end of
> if statement.
>
> bluez-5.64/src/device.c:2977: path: Condition "state->bonded",
> taking false branch.
>
> bluez-5.64/src/device.c:2983: path: Condition "agent", taking
> true branch.
>
> bluez-5.64/src/device.c:2984: path: Falling through to end of
> if statement.
>
> bluez-5.64/src/device.c:2990: path: Condition "agent", taking
> true branch.
>
> bluez-5.64/src/device.c:3005: path: Condition "bdaddr_type != 0",
> taking true branch.
>
> bluez-5.64/src/device.c:3006: path:
>
> Condition "!state->connected", taking true branch.
>
> bluez-5.64/src/device.c:3006: path:
> Condition "btd_le_connect_before_pairing()", taking true branch.
> bluez-5.64/src/device.c:3007: freed_arg: "device_connect_le" frees
> "device->bonding".
>
> bluez-5.64/src/device.c:3007: path: Falling through to end of
> if statement.
>
> bluez-5.64/src/device.c:3012: path: Falling through to end of
> if statement.
>
> bluez-5.64/src/device.c:3017: path: Condition "err < 0",
> taking true branch.
>
> bluez-5.64/src/device.c:3018: double_free: Calling "bonding_request_free"
> frees pointer "device->bonding" which has already been freed.
>
> Signed-off-by: Gopal Tiwari <[email protected]>
> ---
> src/device.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/src/device.c b/src/device.c
> index 8dc12d026..a0e5d40db 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -2942,6 +2942,7 @@ static void bonding_request_free(struct bonding_req *bonding)
> bonding->device->bonding = NULL;
>
> g_free(bonding);
> + bonding = NULL;

I don't think this fixes anything really, since bonding variable goes
out of scope this won't change anything, in fact this seems to be a
false positive since device->bonding shall be set to NULL in the if
statement just above any other call to bonding_request_free will bail
out when checking !bonding, it would be a problem if and only if the
code was using bonding pointer directly instead of device->bonding
with bonding_request_free.

> }
>
> static DBusMessage *pair_device(DBusConnection *conn, DBusMessage *msg,
> --
> 2.26.2
>


--
Luiz Augusto von Dentz