[PATCH 2/2 v3] sixaxis: Ask user whether cable configuration should be allowed

[PATCH 2/2 v3] sixaxis: Ask user whether cable configuration should be allowed

[Bastien Nocera]

Previously, users doing cable configuration of Sixaxis PS3 controllers
would only get asked whether a device was allowed to connect to the
computer when switching it to Bluetooth mode: unplugging it, and
pressing the PS button.

Instead, we should ask the user straight away, through the agent,
whether the pad should be allowed to connect.

This makes it easier to setup those devices, while keeping security.
---
 plugins/sixaxis.c | 80 +++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 63 insertions(+), 17 deletions(-)

diff --git a/plugins/sixaxis.c b/plugins/sixaxis.c
index fcc93bc..032ff62 100644
--- a/plugins/sixaxis.c
+++ b/plugins/sixaxis.c
@@ -44,6 +44,7 @@
 
 #include "src/adapter.h"
 #include "src/device.h"
+#include "src/agent.h"
 #include "src/plugin.h"
 #include "src/log.h"
 #include "src/shared/util.h"
@@ -71,6 +72,13 @@ static const struct {
 	},
 };
 
+struct authentication_closure {
+	struct btd_adapter *adapter;
+	struct btd_device *device;
+	int fd;
+	char device_addr[18];
+};
+
 struct leds_data {
 	char *syspath_prefix;
 	uint8_t bitmap;
@@ -255,19 +263,55 @@ out:
 	return FALSE;
 }
 
+static void agent_auth_cb(DBusError *derr,
+				void *user_data)
+{
+	struct authentication_closure *closure = user_data;
+	char master_addr[18], adapter_addr[18];
+	bdaddr_t master_bdaddr;
+	const bdaddr_t *adapter_bdaddr;
+
+	if (derr != NULL) {
+		DBG("Agent replied negatively, removing temporary device");
+		goto error;
+	}
+
+	btd_device_set_temporary(closure->device, false);
+
+	if (get_master_bdaddr(closure->fd, &master_bdaddr) < 0)
+		goto error;
+
+	adapter_bdaddr = btd_adapter_get_address(closure->adapter);
+	if (bacmp(adapter_bdaddr, &master_bdaddr)) {
+		if (set_master_bdaddr(closure->fd, adapter_bdaddr) < 0)
+			goto error;
+	}
+
+	ba2str(&master_bdaddr, master_addr);
+	ba2str(adapter_bdaddr, adapter_addr);
+	DBG("remote %s old_master %s new_master %s",
+				closure->device_addr, master_addr, adapter_addr);
+
+	g_free(closure);
+
+	return;
+
+error:
+	btd_adapter_remove_device(closure->adapter, closure->device);
+	g_free(closure);
+}
+
 static bool setup_device(int fd, int index, struct btd_adapter *adapter)
 {
-	char device_addr[18], master_addr[18], adapter_addr[18];
-	bdaddr_t device_bdaddr, master_bdaddr;
+	char device_addr[18];
+	bdaddr_t device_bdaddr;
 	const bdaddr_t *adapter_bdaddr;
 	struct btd_device *device;
+	struct authentication_closure *closure;
 
 	if (get_device_bdaddr(fd, &device_bdaddr) < 0)
 		return false;
 
-	if (get_master_bdaddr(fd, &master_bdaddr) < 0)
-		return false;
-
 	/* This can happen if controller was plugged while already connected
 	 * eg. to charge up battery.
 	 * Don't set LEDs in that case, hence return false */
@@ -276,18 +320,7 @@ static bool setup_device(int fd, int index, struct btd_adapter *adapter)
 	if (device && btd_device_is_connected(device))
 		return false;
 
-	adapter_bdaddr = btd_adapter_get_address(adapter);
-
-	if (bacmp(adapter_bdaddr, &master_bdaddr)) {
-		if (set_master_bdaddr(fd, adapter_bdaddr) < 0)
-			return false;
-	}
-
 	ba2str(&device_bdaddr, device_addr);
-	ba2str(&master_bdaddr, master_addr);
-	ba2str(adapter_bdaddr, adapter_addr);
-	DBG("remote %s old_master %s new_master %s",
-				device_addr, master_addr, adapter_addr);
 
 	device = btd_adapter_get_device(adapter, &device_bdaddr, BDADDR_BREDR);
 
@@ -302,7 +335,20 @@ static bool setup_device(int fd, int index, struct btd_adapter *adapter)
 	btd_device_device_set_name(device, devices[index].name);
 	btd_device_set_pnpid(device, devices[index].source, devices[index].vid,
 				devices[index].pid, devices[index].version);
-	btd_device_set_temporary(device, false);
+	btd_device_set_temporary(device, true);
+
+	closure = g_try_new0(struct authentication_closure, 1);
+	if (!closure) {
+		btd_adapter_remove_device(adapter, device);
+		return false;
+	}
+	closure->adapter = adapter;
+	closure->device = device;
+	closure->fd = fd;
+	memcpy(&closure->device_addr, device_addr, sizeof(device_addr));
+	adapter_bdaddr = btd_adapter_get_address(adapter);
+	btd_request_authorization_cable_configured(adapter_bdaddr, &device_bdaddr,
+				HID_UUID, agent_auth_cb, closure);
 
 	return true;
 }
-- 
2.4.3

Re: [PATCH 2/2 v3] sixaxis: Ask user whether cable configuration should be allowed

[Szymon Janc]

Hi Bastien,

On Tuesday 07 July 2015 16:14:25 Bastien Nocera wrote:
> Previously, users doing cable configuration of Sixaxis PS3 controllers
> would only get asked whether a device was allowed to connect to the
> computer when switching it to Bluetooth mode: unplugging it, and
> pressing the PS button.
> 
> Instead, we should ask the user straight away, through the agent,
> whether the pad should be allowed to connect.
> 
> This makes it easier to setup those devices, while keeping security.

Wouldn't this confuse user so that he may think device is already connected 
over BT? Also what would happen if user remove this from usb before 
confirming? And if PS button is pressed then, second authorization request for 
same UUID would be send?

Since this change plugin behavior in end user visible way this needs to be 
carefully thought out. It looks like people have different requirements for 
sixaxis security... so maybe it should have a sort of policy settings in 
config file? Opinions?

> ---
>  plugins/sixaxis.c | 80
> +++++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 63
> insertions(+), 17 deletions(-)
> 
> diff --git a/plugins/sixaxis.c b/plugins/sixaxis.c
> index fcc93bc..032ff62 100644
> --- a/plugins/sixaxis.c
> +++ b/plugins/sixaxis.c
> @@ -44,6 +44,7 @@
> 
>  #include "src/adapter.h"
>  #include "src/device.h"
> +#include "src/agent.h"
>  #include "src/plugin.h"
>  #include "src/log.h"
>  #include "src/shared/util.h"
> @@ -71,6 +72,13 @@ static const struct {
>  	},
>  };
> 
> +struct authentication_closure {
> +	struct btd_adapter *adapter;
> +	struct btd_device *device;

You can get adapter from device, no need to store both.

> +	int fd;
> +	char device_addr[18];

Why is device_addr needed for? If only for logs then I'd just convert bdaddr 
to string where needed.

> +};
> +
>  struct leds_data {
>  	char *syspath_prefix;
>  	uint8_t bitmap;
> @@ -255,19 +263,55 @@ out:
>  	return FALSE;
>  }
> 
> +static void agent_auth_cb(DBusError *derr,
> +				void *user_data)
> +{
> +	struct authentication_closure *closure = user_data;
> +	char master_addr[18], adapter_addr[18];
> +	bdaddr_t master_bdaddr;
> +	const bdaddr_t *adapter_bdaddr;
> +
> +	if (derr != NULL) {
> +		DBG("Agent replied negatively, removing temporary device");
> +		goto error;
> +	}
> +
> +	btd_device_set_temporary(closure->device, false);
> +
> +	if (get_master_bdaddr(closure->fd, &master_bdaddr) < 0)
> +		goto error;
> +
> +	adapter_bdaddr = btd_adapter_get_address(closure->adapter);
> +	if (bacmp(adapter_bdaddr, &master_bdaddr)) {
> +		if (set_master_bdaddr(closure->fd, adapter_bdaddr) < 0)
> +			goto error;
> +	}
> +
> +	ba2str(&master_bdaddr, master_addr);
> +	ba2str(adapter_bdaddr, adapter_addr);
> +	DBG("remote %s old_master %s new_master %s",
> +				closure->device_addr, master_addr, adapter_addr);
> +
> +	g_free(closure);
> +
> +	return;
> +
> +error:
> +	btd_adapter_remove_device(closure->adapter, closure->device);
> +	g_free(closure);
> +}
> +
>  static bool setup_device(int fd, int index, struct btd_adapter *adapter)
>  {
> -	char device_addr[18], master_addr[18], adapter_addr[18];
> -	bdaddr_t device_bdaddr, master_bdaddr;
> +	char device_addr[18];
> +	bdaddr_t device_bdaddr;
>  	const bdaddr_t *adapter_bdaddr;
>  	struct btd_device *device;
> +	struct authentication_closure *closure;
> 
>  	if (get_device_bdaddr(fd, &device_bdaddr) < 0)
>  		return false;
> 
> -	if (get_master_bdaddr(fd, &master_bdaddr) < 0)
> -		return false;
> -
>  	/* This can happen if controller was plugged while already connected
>  	 * eg. to charge up battery.
>  	 * Don't set LEDs in that case, hence return false */
> @@ -276,18 +320,7 @@ static bool setup_device(int fd, int index, struct
> btd_adapter *adapter) if (device && btd_device_is_connected(device))
>  		return false;
> 
> -	adapter_bdaddr = btd_adapter_get_address(adapter);
> -
> -	if (bacmp(adapter_bdaddr, &master_bdaddr)) {
> -		if (set_master_bdaddr(fd, adapter_bdaddr) < 0)
> -			return false;
> -	}
> -
>  	ba2str(&device_bdaddr, device_addr);
> -	ba2str(&master_bdaddr, master_addr);
> -	ba2str(adapter_bdaddr, adapter_addr);
> -	DBG("remote %s old_master %s new_master %s",
> -				device_addr, master_addr, adapter_addr);
> 
>  	device = btd_adapter_get_device(adapter, &device_bdaddr, BDADDR_BREDR);
> 
> @@ -302,7 +335,20 @@ static bool setup_device(int fd, int index, struct
> btd_adapter *adapter) btd_device_device_set_name(device,
> devices[index].name);
>  	btd_device_set_pnpid(device, devices[index].source, devices[index].vid,
>  				devices[index].pid, devices[index].version);
> -	btd_device_set_temporary(device, false);
> +	btd_device_set_temporary(device, true);
> +
> +	closure = g_try_new0(struct authentication_closure, 1);
> +	if (!closure) {
> +		btd_adapter_remove_device(adapter, device);
> +		return false;
> +	}
> +	closure->adapter = adapter;
> +	closure->device = device;
> +	closure->fd = fd;
> +	memcpy(&closure->device_addr, device_addr, sizeof(device_addr));
> +	adapter_bdaddr = btd_adapter_get_address(adapter);
> +	btd_request_authorization_cable_configured(adapter_bdaddr, &device_bdaddr,
> +				HID_UUID, agent_auth_cb, closure);
> 
>  	return true;
>  }



-- 
Szymon K. Janc
szymon.janc@gmail.com

Re: [PATCH 2/2 v3] sixaxis: Ask user whether cable configuration should be allowed

[Bastien Nocera]

On Fri, 2015-07-24 at 00:33 +0200, Szymon Janc wrote:
> Hi Bastien,
> 
> On Tuesday 07 July 2015 16:14:25 Bastien Nocera wrote:
> > Previously, users doing cable configuration of Sixaxis PS3 
> > controllers
> > would only get asked whether a device was allowed to connect to the
> > computer when switching it to Bluetooth mode: unplugging it, and
> > pressing the PS button.
> > 
> > Instead, we should ask the user straight away, through the agent,
> > whether the pad should be allowed to connect.
> > 
> > This makes it easier to setup those devices, while keeping 
> > security.
> 
> Wouldn't this confuse user so that he may think device is already 
> connected 
> over BT?

No, because either:
- you don't want to pair the device with your computer, which is
impossible to do right now, and you can now do if you don't have an
agent, or reject the association
- you do want to be able to use it via Bluetooth, and we can have the
association happen in one go, instead of being 2 separate actions.

>  Also what would happen if user remove this from usb before 
> confirming?

I didn't implement this, but we should cancel the existing
authentication request indeed.

>  And if PS button is pressed then, second authorization request for 
> same UUID would be send?

It wouldn't do anything, as there's already an auth request in flux.

> Since this change plugin behavior in end user visible way this needs 
> to be 
> carefully thought out. It looks like people have different 
> requirements for 
> sixaxis security...

This is actually more secure than what came before, and it's also far
more predictable. It's the same security as before, but requires an
agent to be available when the device is plugged in.

>  so maybe it should have a sort of policy settings in 
> config file? Opinions?

What would the other policy be? I don't see a difference between the
current security behaviour, and the one set out in this patch.