Closed Bug 889795 Opened 7 years ago Closed 6 years ago

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

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

x86_64
Linux
defect
Not set

Tracking

(blocking-b2g:koi+, firefox24 wontfix, firefox25 wontfix, firefox26 fixed, b2g-v1.2 fixed)

RESOLVED FIXED
1.2 FC (16sep)
blocking-b2g koi+
Tracking Status
firefox24 --- wontfix
firefox25 --- wontfix
firefox26 --- fixed
b2g-v1.2 --- fixed

People

(Reporter: shawnjohnjr, Assigned: gyeh)

References

Details

Attachments

(1 file, 8 obsolete files)

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.
Assignee: nobody → gyeh
Blocks: 880759
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.
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.
Hi mrbkap and echou, please help to review the patch. Thanks.
Attachment #785640 - Attachment is obsolete: true
Attachment #785647 - Flags: superreview?(mrbkap)
Attachment #785647 - Flags: review?(echou)
Attachment #785647 - Attachment is obsolete: true
Attachment #785647 - Flags: superreview?(mrbkap)
Attachment #785647 - Flags: review?(echou)
Attachment #785648 - Flags: superreview?(mrbkap)
Attachment #785648 - Flags: review?(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.
(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.
Refined based on Eric's comment. Please help to review. Thanks.
Attachment #785648 - Attachment is obsolete: true
Attachment #785648 - Flags: superreview?(mrbkap)
Attachment #785648 - Flags: review?(echou)
Attachment #787331 - Flags: superreview?(mrbkap)
Attachment #787331 - Flags: review?(echou)
Updated.
Attachment #787331 - Attachment is obsolete: true
Attachment #787331 - Flags: superreview?(mrbkap)
Attachment #787331 - Flags: review?(echou)
Attachment #787333 - Flags: superreview?(mrbkap)
Attachment #787333 - Flags: review?(echou)
One line is missed in the latest attachment. Please review this patch. Thanks.
Attachment #787333 - Attachment is obsolete: true
Attachment #787333 - Flags: superreview?(mrbkap)
Attachment #787333 - Flags: review?(echou)
Attachment #787335 - Flags: superreview?(mrbkap)
Attachment #787335 - Flags: review?(echou)
Attachment #787335 - Attachment is obsolete: true
Attachment #787335 - Flags: superreview?(mrbkap)
Attachment #787335 - Flags: review?(echou)
Attachment #787337 - Flags: superreview?(mrbkap)
Attachment #787337 - Flags: review?(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.
(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 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.
Attachment #787337 - Flags: superreview?(mrbkap) → superreview+
Thanks, mrbkap.
Updated according to review comments.
Attachment #787337 - Attachment is obsolete: true
Attachment #787337 - Flags: review?(echou)
Attachment #789325 - Flags: review?(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"
Attachment #789325 - Flags: review?(echou) → review+
Since HID has been landed (bug 899014), I added it into sAuthorizedServiceClass.
Attachment #789325 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/fab433e89f6d
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
blocking-b2g: --- → koi?
blocking-b2g: koi? → koi+
Target Milestone: --- → 1.2 FC (16sep)
You need to log in before you can comment on or make changes to this bug.