[PATCH BlueZ 0/2] Add device_set_trusted()

[PATCH BlueZ 0/2] Add device_set_trusted()

[Antonio Ospite]

Hi,

In patch 1 a device_set_trusted() function is proposed, which I plan to
use for the playstation-peripheral plugin; I am in the process of
rebasing the plugin on top of BlueZ 5.2.

In patch 2 the newly introduced function is used in order to avoid some
duplication.

device_set_trusted() looks a lot like device_set_temporary() and
device_set_legacy(), I hope it makes sense to you too.

Thanks,
   Antonio

Antonio Ospite (2):
  device: add a device_set_trusted() function
  device: use device_set_trusted() in set_trust()

 src/device.c |   30 +++++++++++++++++++-----------
 src/device.h |    1 +
 2 files changed, 20 insertions(+), 11 deletions(-)

-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

[PATCH BlueZ 1/2] device: add a device_set_trusted() function

[Antonio Ospite]

---
 src/device.c |   18 ++++++++++++++++++
 src/device.h |    1 +
 2 files changed, 19 insertions(+)

diff --git a/src/device.c b/src/device.c
index 49f8957..87e5eff 100644
--- a/src/device.c
+++ b/src/device.c
@@ -3462,6 +3462,24 @@ void device_set_temporary(struct btd_device *device, gboolean temporary)
 	device->temporary = temporary;
 }
 
+void device_set_trusted(struct btd_device *device, gboolean trusted)
+{
+	if (!device)
+		return;
+
+	if (device->trusted == trusted)
+		return;
+
+	DBG("trusted %d", trusted);
+
+	device->trusted = trusted;
+
+	store_device_info(device);
+
+	g_dbus_emit_property_changed(dbus_conn, device->path,
+					DEVICE_INTERFACE, "Trusted");
+}
+
 void device_set_bonded(struct btd_device *device, gboolean bonded)
 {
 	if (!device)
diff --git a/src/device.h b/src/device.h
index dc11e2c..d072015 100644
--- a/src/device.h
+++ b/src/device.h
@@ -68,6 +68,7 @@ gboolean device_is_bonded(struct btd_device *device);
 gboolean device_is_trusted(struct btd_device *device);
 void device_set_paired(struct btd_device *device, gboolean paired);
 void device_set_temporary(struct btd_device *device, gboolean temporary);
+void device_set_trusted(struct btd_device *device, gboolean trusted);
 void device_set_bonded(struct btd_device *device, gboolean bonded);
 void device_set_legacy(struct btd_device *device, bool legacy);
 void device_set_rssi(struct btd_device *device, int8_t rssi);
-- 
1.7.10.4

[PATCH BlueZ 2/2] device: use device_set_trusted() in set_trust()

[Antonio Ospite]

---
 src/device.c |   12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/src/device.c b/src/device.c
index 87e5eff..9e4c41f 100644
--- a/src/device.c
+++ b/src/device.c
@@ -724,17 +724,7 @@ static void set_trust(GDBusPendingPropertySet id, gboolean value, void *data)
 {
 	struct btd_device *device = data;
 
-	if (device->trusted == value) {
-		g_dbus_pending_property_success(id);
-		return;
-	}
-
-	device->trusted = value;
-
-	store_device_info(device);
-
-	g_dbus_emit_property_changed(dbus_conn, device->path,
-						DEVICE_INTERFACE, "Trusted");
+	device_set_trusted(device, value);
 
 	g_dbus_pending_property_success(id);
 }
-- 
1.7.10.4

Re: [PATCH BlueZ 2/2] device: use device_set_trusted() in set_trust()

[Anderson Lizardo]

Hi Antonio,

On Sun, Feb 3, 2013 at 12:14 PM, Antonio Ospite
<ospite@studenti.unina.it> wrote:
> @@ -724,17 +724,7 @@ static void set_trust(GDBusPendingPropertySet id, gboolean value, void *data)
>  {
>         struct btd_device *device = data;
>
> -       if (device->trusted == value) {
> -               g_dbus_pending_property_success(id);
> -               return;
> -       }
> -
> -       device->trusted = value;
> -
> -       store_device_info(device);
> -
> -       g_dbus_emit_property_changed(dbus_conn, device->path,
> -                                               DEVICE_INTERFACE, "Trusted");
> +       device_set_trusted(device, value);
>
>         g_dbus_pending_property_success(id);

g_dbus_pending_property_success() is now called always (whether the
property changed or not). Not sure if this is an issue, but it is a
change that needs at least a clarification on the commit message.

Best Regards,
-- 
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil

Re: [PATCH BlueZ 2/2] device: use device_set_trusted() in set_trust()

[Antonio Ospite]

On Sun, 3 Feb 2013 13:32:12 -0400
Anderson Lizardo <anderson.lizardo@openbossa.org> wrote:

> Hi Antonio,
> 
> On Sun, Feb 3, 2013 at 12:14 PM, Antonio Ospite
> <ospite@studenti.unina.it> wrote:
> > @@ -724,17 +724,7 @@ static void set_trust(GDBusPendingPropertySet id, gboolean value, void *data)
> >  {
> >         struct btd_device *device = data;
> >
> > -       if (device->trusted == value) {
> > -               g_dbus_pending_property_success(id);
> > -               return;
> > -       }
> > -
> > -       device->trusted = value;
> > -
> > -       store_device_info(device);
> > -
> > -       g_dbus_emit_property_changed(dbus_conn, device->path,
> > -                                               DEVICE_INTERFACE, "Trusted");
> > +       device_set_trusted(device, value);
> >
> >         g_dbus_pending_property_success(id);
> 
> g_dbus_pending_property_success() is now called always (whether the
> property changed or not). Not sure if this is an issue, but it is a
> change that needs at least a clarification on the commit message.
> 

The logic is equivalent to the previous one, as device_set_trusted()
returns without doing anything when (device->trusted == trusted), and
the previous implementation was already calling
g_dbus_pending_property_success() just before returning in any exit
path. Or was it meant to be g_dbus_pending_property_error() in the
first exit path? I don't know about that.

The previous set_trust() code didn't consider the !device case so we
should be OK if this function assumes the device is always defined.

If the code is OK I can improve the commit message, yes.

Thanks,
   Antonio

-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

Re: [PATCH BlueZ 2/2] device: use device_set_trusted() in set_trust()

[Anderson Lizardo]

Hi Antonio,

On Sun, Feb 3, 2013 at 1:51 PM, Antonio Ospite <ospite@studenti.unina.it> wrote:
> The logic is equivalent to the previous one, as device_set_trusted()
> returns without doing anything when (device->trusted == trusted), and
> the previous implementation was already calling
> g_dbus_pending_property_success() just before returning in any exit
> path. Or was it meant to be g_dbus_pending_property_error() in the
> first exit path? I don't know about that.

My mistake. I incorrectly read the diff. And I think the _success()
function is correct as it is.

>
> The previous set_trust() code didn't consider the !device case so we
> should be OK if this function assumes the device is always defined.
>
> If the code is OK I can improve the commit message, yes.

Having more text on the commit message is always better than less text
:). But in this case, the commit is quite simple, so it is up to
you/Johan/Marcel.

Best Regards,
-- 
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil

Re: [PATCH BlueZ 0/2] Add device_set_trusted()

[Antonio Ospite]

On Sun,  3 Feb 2013 17:14:29 +0100
Antonio Ospite <ospite@studenti.unina.it> wrote:

> Hi,
> 
> In patch 1 a device_set_trusted() function is proposed, which I plan to
> use for the playstation-peripheral plugin; I am in the process of
> rebasing the plugin on top of BlueZ 5.2.
> 
> In patch 2 the newly introduced function is used in order to avoid some
> duplication.
> 
> device_set_trusted() looks a lot like device_set_temporary() and
> device_set_legacy(), I hope it makes sense to you too.
>
> Thanks,
>    Antonio
> 
> Antonio Ospite (2):
>   device: add a device_set_trusted() function
>   device: use device_set_trusted() in set_trust()
>

Ping for these two patches.
 
>  src/device.c |   30 +++++++++++++++++++-----------
>  src/device.h |    1 +
>  2 files changed, 20 insertions(+), 11 deletions(-)
> 

-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

Re: [PATCH BlueZ 0/2] Add device_set_trusted()

[Johan Hedberg]

Hi Antonio,

On Sun, Feb 03, 2013, Antonio Ospite wrote:
> In patch 1 a device_set_trusted() function is proposed, which I plan to
> use for the playstation-peripheral plugin; I am in the process of
> rebasing the plugin on top of BlueZ 5.2.
> 
> In patch 2 the newly introduced function is used in order to avoid some
> duplication.
> 
> device_set_trusted() looks a lot like device_set_temporary() and
> device_set_legacy(), I hope it makes sense to you too.
> 
> Thanks,
>    Antonio
> 
> Antonio Ospite (2):
>   device: add a device_set_trusted() function
>   device: use device_set_trusted() in set_trust()
> 
>  src/device.c |   30 +++++++++++++++++++-----------
>  src/device.h |    1 +
>  2 files changed, 20 insertions(+), 11 deletions(-)

This patch set makes me a bit uneasy since setting a device as trusted
is a security sensitive operation. My initial reaction is that this
should only be done through explicit user interaction, i.e. through the
D-Bus interface. I'm also worried that plugins will start misusing this
API once it's available.

Feel free to try to convince me otherwise though.

Johan

Re: [PATCH BlueZ 0/2] Add device_set_trusted()

[Bastien Nocera]

On Fri, 2013-02-15 at 10:36 +0200, Johan Hedberg wrote:
> Hi Antonio,
> 
> On Sun, Feb 03, 2013, Antonio Ospite wrote:
> > In patch 1 a device_set_trusted() function is proposed, which I plan to
> > use for the playstation-peripheral plugin; I am in the process of
> > rebasing the plugin on top of BlueZ 5.2.
> > 
> > In patch 2 the newly introduced function is used in order to avoid some
> > duplication.
> > 
> > device_set_trusted() looks a lot like device_set_temporary() and
> > device_set_legacy(), I hope it makes sense to you too.
> > 
> > Thanks,
> >    Antonio
> > 
> > Antonio Ospite (2):
> >   device: add a device_set_trusted() function
> >   device: use device_set_trusted() in set_trust()
> > 
> >  src/device.c |   30 +++++++++++++++++++-----------
> >  src/device.h |    1 +
> >  2 files changed, 20 insertions(+), 11 deletions(-)
> 
> This patch set makes me a bit uneasy since setting a device as trusted
> is a security sensitive operation. My initial reaction is that this
> should only be done through explicit user interaction, i.e. through the
> D-Bus interface.

How is using D-Bus interface "user interaction"? It's not any more user
interaction than doing it this way, which avoid going out through the
public interface for something we are setting up ourselves.

>  I'm also worried that plugins will start misusing this
> API once it's available.

I think that it's completely fair for plugins that *do* set up devices
to call this function. That's what the plugin is all about. Seeing as
devices should be marked as trusted to be usable, I see no reason that
this shouldn't be done automatically.

If the problem is other plugins abusing the function, then they could
just as well poke the files directly, as they already have all the
rights they need for that.

> Feel free to try to convince me otherwise though.

I don't see what using the D-Bus API would gain us, apart from more work
and indirection on the plugin side.

Re: [PATCH BlueZ 0/2] Add device_set_trusted()

[Johan Hedberg]

Hi Bastien,

On Fri, Feb 15, 2013, Bastien Nocera wrote:
> > This patch set makes me a bit uneasy since setting a device as trusted
> > is a security sensitive operation. My initial reaction is that this
> > should only be done through explicit user interaction, i.e. through the
> > D-Bus interface.
> 
> How is using D-Bus interface "user interaction"? It's not any more user
> interaction than doing it this way, which avoid going out through the
> public interface for something we are setting up ourselves.

Typically you'd get a pop-up dialog on the next connect attempt from
this device "accept connection from foo?" with a check-box or similar to
elect setting it as trusted. Alternatively the stuff that this plugin
does upon initial device setup could cause a similar pop-up dialog to be
presented to the user.

> >  I'm also worried that plugins will start misusing this
> > API once it's available.
> 
> I think that it's completely fair for plugins that *do* set up devices
> to call this function. That's what the plugin is all about. Seeing as
> devices should be marked as trusted to be usable, I see no reason that
> this shouldn't be done automatically.

How does the plugin that this API is primarily targeted for setup the
device? Does it do it through some physical connection like USB? In such
a case it's probably fine to skip the user interaction part since if
you've got physical access to the device there are much severe security
issues to consider.

Johan

Re: [PATCH BlueZ 0/2] Add device_set_trusted()

[Bastien Nocera]

On Fri, 2013-02-15 at 10:57 +0200, Johan Hedberg wrote:
> Hi Bastien,
> 
> On Fri, Feb 15, 2013, Bastien Nocera wrote:
> > > This patch set makes me a bit uneasy since setting a device as trusted
> > > is a security sensitive operation. My initial reaction is that this
> > > should only be done through explicit user interaction, i.e. through the
> > > D-Bus interface.
> > 
> > How is using D-Bus interface "user interaction"? It's not any more user
> > interaction than doing it this way, which avoid going out through the
> > public interface for something we are setting up ourselves.
> 
> Typically you'd get a pop-up dialog on the next connect attempt from
> this device "accept connection from foo?" with a check-box or similar to
> elect setting it as trusted. Alternatively the stuff that this plugin
> does upon initial device setup could cause a similar pop-up dialog to be
> presented to the user.

That's possible, technically, but it's not how the device gets set up
when used in its original environment (eg. when plugged into a PS3).

> > >  I'm also worried that plugins will start misusing this
> > > API once it's available.
> > 
> > I think that it's completely fair for plugins that *do* set up devices
> > to call this function. That's what the plugin is all about. Seeing as
> > devices should be marked as trusted to be usable, I see no reason that
> > this shouldn't be done automatically.
> 
> How does the plugin that this API is primarily targeted for setup the
> device? Does it do it through some physical connection like USB?

Yes. When you plug the device in, we tell it which adapter to connect to
when used without a cable, and set ourselves up locally so that we can
accept its connection without any more interaction.

>  In such
> a case it's probably fine to skip the user interaction part since if
> you've got physical access to the device there are much severe security
> issues to consider.

Yes, the user would already have had the joypad plugged into the
computer before any setup would have been made.

Re: [PATCH BlueZ 0/2] Add device_set_trusted()

[Johan Hedberg]

Hi Bastien,

On Fri, Feb 15, 2013, Bastien Nocera wrote:
> On Fri, 2013-02-15 at 10:57 +0200, Johan Hedberg wrote:
> > > > This patch set makes me a bit uneasy since setting a device as trusted
> > > > is a security sensitive operation. My initial reaction is that this
> > > > should only be done through explicit user interaction, i.e. through the
> > > > D-Bus interface.
> > > 
> > > How is using D-Bus interface "user interaction"? It's not any more user
> > > interaction than doing it this way, which avoid going out through the
> > > public interface for something we are setting up ourselves.
> > 
> > Typically you'd get a pop-up dialog on the next connect attempt from
> > this device "accept connection from foo?" with a check-box or similar to
> > elect setting it as trusted. Alternatively the stuff that this plugin
> > does upon initial device setup could cause a similar pop-up dialog to be
> > presented to the user.
> 
> That's possible, technically, but it's not how the device gets set up
> when used in its original environment (eg. when plugged into a PS3).
> 
> > > >  I'm also worried that plugins will start misusing this
> > > > API once it's available.
> > > 
> > > I think that it's completely fair for plugins that *do* set up devices
> > > to call this function. That's what the plugin is all about. Seeing as
> > > devices should be marked as trusted to be usable, I see no reason that
> > > this shouldn't be done automatically.
> > 
> > How does the plugin that this API is primarily targeted for setup the
> > device? Does it do it through some physical connection like USB?
> 
> Yes. When you plug the device in, we tell it which adapter to connect to
> when used without a cable, and set ourselves up locally so that we can
> accept its connection without any more interaction.
> 
> >  In such
> > a case it's probably fine to skip the user interaction part since if
> > you've got physical access to the device there are much severe security
> > issues to consider.
> 
> Yes, the user would already have had the joypad plugged into the
> computer before any setup would have been made.

Fair enough. This does seem like a valid reason for setting the trusted
setting internally.

Johan

Re: [PATCH BlueZ 0/2] Add device_set_trusted()

[Johan Hedberg]

Hi Antonio,

On Sun, Feb 03, 2013, Antonio Ospite wrote:
> In patch 1 a device_set_trusted() function is proposed, which I plan to
> use for the playstation-peripheral plugin; I am in the process of
> rebasing the plugin on top of BlueZ 5.2.
> 
> In patch 2 the newly introduced function is used in order to avoid some
> duplication.
> 
> device_set_trusted() looks a lot like device_set_temporary() and
> device_set_legacy(), I hope it makes sense to you too.
> 
> Thanks,
>    Antonio
> 
> Antonio Ospite (2):
>   device: add a device_set_trusted() function
>   device: use device_set_trusted() in set_trust()
> 
>  src/device.c |   30 +++++++++++++++++++-----------
>  src/device.h |    1 +
>  2 files changed, 20 insertions(+), 11 deletions(-)

Both patches are now applied (after adding some needed info to the
commit messages for the justification of these changes).

Johan

Re: [PATCH BlueZ 0/2] Add device_set_trusted()

[Antonio Ospite]

On Fri, 15 Feb 2013 11:27:40 +0200
Johan Hedberg <johan.hedberg@gmail.com> wrote:

> Hi Antonio,
> 
> On Sun, Feb 03, 2013, Antonio Ospite wrote:
> > In patch 1 a device_set_trusted() function is proposed, which I plan to
> > use for the playstation-peripheral plugin; I am in the process of
> > rebasing the plugin on top of BlueZ 5.2.
> > 
> > In patch 2 the newly introduced function is used in order to avoid some
> > duplication.
> > 
> > device_set_trusted() looks a lot like device_set_temporary() and
> > device_set_legacy(), I hope it makes sense to you too.
> > 
> > Thanks,
> >    Antonio
> > 
> > Antonio Ospite (2):
> >   device: add a device_set_trusted() function
> >   device: use device_set_trusted() in set_trust()
> > 
> >  src/device.c |   30 +++++++++++++++++++-----------
> >  src/device.h |    1 +
> >  2 files changed, 20 insertions(+), 11 deletions(-)
> 
> Both patches are now applied (after adding some needed info to the
> commit messages for the justification of these changes).
> 

Thanks a lot, mentioning USB-based pairing in the commit message is a
useful hint indeed.

And thanks to Bastien for backing up the changes better than I would
have.

See you soon with a couple of more controversial patches :)

Regards,
   Antonio

-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?