889795 - [Bluetooth] Any A2DP incoming connection will be rejected because we do not handle "Authorize" signal

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

We need to define the user behavior and check that we need to pop-up incoming connection request dialog or by default grant A2DP incoming connection.
The concern here is for bluetooth security. Currently we did not handle this dbus signal (Authorize). Maybe user story is required.

Comment 1

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 1(v1): Reply 'Authorize' signal for supported profile

Comment 2

[Gina Yeh [:gyeh] [:ginayeh]]

Currently, we should only accept the Authorize request for A2DP. After HID implementation is completed, it should be also put in |sAuthorizedServiceClass|. So, I'd like to make |sAuthorizedServiceClass| as an array.

Comment 3

[Gina Yeh [:gyeh] [:ginayeh]]

For those authorized profiles, we just reply the DBus message immediately. So, there's no need to broadcast a system message anyway, and "bluetooth-authorize" is removed in SystemMessagePermissionChecker.

Comment 4

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 1(v1): Reply 'Authorize' signal for supported profile

Hi mrbkap and echou, please help to review the patch. Thanks.

Comment 5

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 1(v1): Reply 'Authorize' signal for supported profile

Comment 6

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 785648 [details] [diff] [review]
Patch 1(v1): Reply 'Authorize' signal for supported profile

Review of attachment 785648 [details] [diff] [review]:
-----------------------------------------------------------------

Overall looks good. Happy to review again when the new patch is ready.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +566,5 @@
>  
>    if ((receivedType != expectedType) && !convert) {
> +    NS_WARNING(nsPrintfCString("Iterator not type we expect! Property name: %s,
> +      Property Type Expected: %d, Property Type Received: %d",
> +      NS_ConvertUTF16toUTF8(propertyName), expectedType, receivedType).get());

NS_ConvertUTF16toUTF8(propertyName) should be NS_ConvertUTF16toUTF8(propertyName).get()

@@ +962,5 @@
>      }
>  
> +    DBusMessage *reply = dbus_message_new_method_return(msg);
> +    if (!reply) {
> +      errorStr.AssignLiteral("Memory can't be allocated for the message.");

How about 'goto handle_error;' here?

@@ +974,5 @@
> +      int i;
> +      for (i = 0; i < sAuthorizedServiceClass.Length(); i++) {
> +        if (serviceClass == sAuthorizedServiceClass[i]) {
> +          dbus_connection_send(conn, reply, NULL);
> +          dbus_message_unref(reply);

reply should be unref'ed even if serviceClass != sAuthorizedServiceClass[i].

@@ +983,5 @@
> +      if (i == sAuthorizedServiceClass.Length()) {
> +        errorStr = NS_ConvertUTF8toUTF16(
> +          nsPrintfCString("Uuid (%s) isn't authorized.", uuid));
> +      }
> +    }

We should call 'return' here, otherwise the event would be sent to Gaia.

@@ +1736,3 @@
>    sIsPairing = 0;
>  
> +  sAuthorizedServiceClass.RemoveElement(BluetoothServiceClass::A2DP);

sAuthorizedServiceClass.Clear() might make more sense.

Comment 7

[Gina Yeh [:gyeh] [:ginayeh]]

(In reply to Eric Chou [:ericchou] [:echou] from comment #6)
> @@ +983,5 @@
> > +      if (i == sAuthorizedServiceClass.Length()) {
> > +        errorStr = NS_ConvertUTF8toUTF16(
> > +          nsPrintfCString("Uuid (%s) isn't authorized.", uuid));
> > +      }
> > +    }
> 
> We should call 'return' here, otherwise the event would be sent to Gaia.

Thanks for pointing out this. This signal is sent to BluetoothService and is ignored then. It wouldn't be distributed to Gaia but I agree that we should return here.

Comment 8

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 1(v2): Reply 'Authorize' signal for supported profile

Refined based on Eric's comment. Please help to review. Thanks.

Comment 9

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 1(v2): Reply 'Authorize' signal for supported profile

Updated.

Comment 10

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 1(v2): Reply 'Authorize' signal for supported profile

One line is missed in the latest attachment. Please review this patch. Thanks.

Comment 11

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 1(v2): Reply 'Authorize' signal for supported profile

Comment 12

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 787337 [details] [diff] [review]
Patch 1(v2): Reply 'Authorize' signal for supported profile

Review of attachment 787337 [details] [diff] [review]:
-----------------------------------------------------------------

Please fix the only problem. Thanks.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +983,2 @@
>  
> +      if (i == sAuthorizedServiceClass.Length()) {

If this fits (i == sAuthorizedServiceClass.Length()), it means that the requested profile is not on the whitelist, so we should reject this authorization request via replying a DBusMessage variable which is generated from dbus_message_new_error().

Please see the original BluetoothDBusService::SetAuthorizationInternal() for more information.

Comment 13

[Gina Yeh [:gyeh] [:ginayeh]]

(In reply to Eric Chou [:ericchou] [:echou] from comment #12)
> Comment on attachment 787337 [details] [diff] [review]
> Patch 1(v2): Reply 'Authorize' signal for supported profile
> 
> Review of attachment 787337 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please fix the only problem. Thanks.
> 
> ::: dom/bluetooth/linux/BluetoothDBusService.cpp
> @@ +983,2 @@
> >  
> > +      if (i == sAuthorizedServiceClass.Length()) {
> 
> If this fits (i == sAuthorizedServiceClass.Length()), it means that the
> requested profile is not on the whitelist, so we should reject this
> authorization request via replying a DBusMessage variable which is generated
> from dbus_message_new_error().
> 
> Please see the original BluetoothDBusService::SetAuthorizationInternal() for
> more information.

Ok. Let me check the original implementation.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 787337 [details] [diff] [review]
Patch 1(v2): Reply 'Authorize' signal for supported profile

Review of attachment 787337 [details] [diff] [review]:
-----------------------------------------------------------------

sr=mrbkap assuming that Eric is happy with this.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +964,5 @@
> +    DBusMessage *reply = dbus_message_new_method_return(msg);
> +    if (!reply) {
> +      errorStr.AssignLiteral("Memory can't be allocated for the message.");
> +    } else {
> +      nsAutoString uuidStr = NS_ConvertUTF8toUTF16(uuid);

This can simply be:

  NS_ConvertUTF8toUTF16 uuidStr(uuid);

to avoid a string copy.

Comment 15

[Gina Yeh [:gyeh] [:ginayeh]]

Thanks, mrbkap.

Comment 16

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 1(v3): Reply 'Authorize' signal for supported profile

Updated according to review comments.

Comment 17

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 789325 [details] [diff] [review]
Patch 1(v3): Reply 'Authorize' signal for supported profile

Review of attachment 789325 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits addressed.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +971,4 @@
>  
> +    DBusMessage* reply;
> +    int i;
> +    for (i = 0; i < sAuthorizedServiceClass.Length(); i++) {

nit: since sAuthorizedServiceClass.Length() is used here and below, please use a variable to hold the value.

@@ +980,5 @@
>  
> +    // The uuid isn't authorized
> +    if (i == sAuthorizedServiceClass.Length()) {
> +      BT_WARNING("Uuid is not authorized.");
> +      reply = dbus_message_new_error(msg, "obj1rg.bluez.Error.Rejected",

Seems a typo. "obj1rg.bluez.Error.Rejected" => "org.bluez.Error.Rejected"

Comment 18

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: [Final] Reply 'Authorize' signal for supported profile, r=echou, sr=mrbkap

Since HID has been landed (bug 899014), I added it into sAuthorizedServiceClass.

Comment 19

[Gina Yeh [:gyeh] [:ginayeh]]

try:
https://tbpl.mozilla.org/?tree=Try&rev=2041fc74c4fd
https://tbpl.mozilla.org/?tree=Try&rev=6045949d033c

Comment 20

[Gina Yeh [:gyeh] [:ginayeh]]

https://hg.mozilla.org/integration/b2g-inbound/rev/fab433e89f6d

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fab433e89f6d