[PATCH 1/2] Change security level on demand when reading characteristic value

[PATCH 1/2] Change security level on demand when reading characteristic value

[Claudio Takahasi]

If a characteristic requires a higher security level, change it on
demand and re-send the GATT Charateristic Value Read. Request will not
be sent until the SMP negotiation finishes. This change doesn't affect
GATT over BR/EDR, since encryption is mandatory for BR/EDR.
---
 attrib/client.c  |   11 +++++++++++
 attrib/gattrib.c |    8 ++++++++
 attrib/gattrib.h |    2 ++
 3 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/attrib/client.c b/attrib/client.c
index 10bbf7d..3297a0c 100644
--- a/attrib/client.c
+++ b/attrib/client.c
@@ -744,6 +744,17 @@ static void update_char_value(guint8 status, const guint8 *pdu,
 
 	if (status == 0)
 		characteristic_set_value(chr, pdu + 1, len - 1);
+	else if (status == ATT_ECODE_INSUFF_ENC) {
+		GIOChannel *io = g_attrib_get_channel(gatt->attrib);
+
+		if (bt_io_set(io, BT_IO_L2CAP, NULL,
+				BT_IO_OPT_SEC_LEVEL, BT_IO_SEC_HIGH,
+				BT_IO_OPT_INVALID)) {
+			gatt_read_char(gatt->attrib, chr->handle,
+					update_char_value, current);
+			return;
+		}
+	}
 
 	g_attrib_unref(gatt->attrib);
 	g_free(current);
diff --git a/attrib/gattrib.c b/attrib/gattrib.c
index 9268001..dd7b1d7 100644
--- a/attrib/gattrib.c
+++ b/attrib/gattrib.c
@@ -202,6 +202,14 @@ void g_attrib_unref(GAttrib *attrib)
 	g_free(attrib);
 }
 
+GIOChannel *g_attrib_get_channel(GAttrib *attrib)
+{
+	if (!attrib)
+		return NULL;
+
+	return attrib->io;
+}
+
 gboolean g_attrib_set_disconnect_function(GAttrib *attrib,
 		GAttribDisconnectFunc disconnect, gpointer user_data)
 {
diff --git a/attrib/gattrib.h b/attrib/gattrib.h
index 0940289..5c50bc9 100644
--- a/attrib/gattrib.h
+++ b/attrib/gattrib.h
@@ -44,6 +44,8 @@ GAttrib *g_attrib_new(GIOChannel *io);
 GAttrib *g_attrib_ref(GAttrib *attrib);
 void g_attrib_unref(GAttrib *attrib);
 
+GIOChannel *g_attrib_get_channel(GAttrib *attrib);
+
 gboolean g_attrib_set_disconnect_function(GAttrib *attrib,
 		GAttribDisconnectFunc disconnect, gpointer user_data);
 
-- 
1.7.3.4

[PATCH 2/2] Change security level on demand when reading characteristic descriptor

[Claudio Takahasi]

---
 attrib/client.c |   33 ++++++++++++++++++++++-----------
 1 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/attrib/client.c b/attrib/client.c
index 3297a0c..644cd62 100644
--- a/attrib/client.c
+++ b/attrib/client.c
@@ -693,18 +693,29 @@ static void update_char_desc(guint8 status, const guint8 *pdu, guint16 len,
 	struct gatt_service *gatt = current->prim->gatt;
 	struct characteristic *chr = current->chr;
 
-	if (status != 0)
-		goto done;
+	if (status == 0) {
 
-	g_free(chr->desc);
+		g_free(chr->desc);
 
-	chr->desc = g_malloc(len);
-	memcpy(chr->desc, pdu + 1, len - 1);
-	chr->desc[len - 1] = '\0';
+		chr->desc = g_malloc(len);
+		memcpy(chr->desc, pdu + 1, len - 1);
+		chr->desc[len - 1] = '\0';
+
+		store_attribute(gatt, current->handle,
+				GATT_CHARAC_USER_DESC_UUID,
+				(void *) chr->desc, len);
+	} else if (status == ATT_ECODE_INSUFF_ENC) {
+		GIOChannel *io = g_attrib_get_channel(gatt->attrib);
+
+		if (bt_io_set(io, BT_IO_L2CAP, NULL,
+				BT_IO_OPT_SEC_LEVEL, BT_IO_SEC_HIGH,
+				BT_IO_OPT_INVALID)) {
+			gatt_read_char(gatt->attrib, current->handle,
+					update_char_desc, current);
+			return;
+		}
+	}
 
-	store_attribute(gatt, current->handle, GATT_CHARAC_USER_DESC_UUID,
-						(void *) chr->desc, len);
-done:
 	g_attrib_unref(gatt->attrib);
 	g_free(current);
 }
-- 
1.7.3.4

Re: [PATCH 1/2] Change security level on demand when reading characteristic value

[Johan Hedberg]

Hi Claudio,

On Thu, Dec 30, 2010, Claudio Takahasi wrote:
> If a characteristic requires a higher security level, change it on
> demand and re-send the GATT Charateristic Value Read. Request will not
> be sent until the SMP negotiation finishes. This change doesn't affect
> GATT over BR/EDR, since encryption is mandatory for BR/EDR.
> ---
>  attrib/client.c  |   11 +++++++++++
>  attrib/gattrib.c |    8 ++++++++
>  attrib/gattrib.h |    2 ++
>  3 files changed, 21 insertions(+), 0 deletions(-)
> 
> diff --git a/attrib/client.c b/attrib/client.c
> index 10bbf7d..3297a0c 100644
> --- a/attrib/client.c
> +++ b/attrib/client.c
> @@ -744,6 +744,17 @@ static void update_char_value(guint8 status, const guint8 *pdu,
>  
>  	if (status == 0)
>  		characteristic_set_value(chr, pdu + 1, len - 1);
> +	else if (status == ATT_ECODE_INSUFF_ENC) {
> +		GIOChannel *io = g_attrib_get_channel(gatt->attrib);
> +
> +		if (bt_io_set(io, BT_IO_L2CAP, NULL,
> +				BT_IO_OPT_SEC_LEVEL, BT_IO_SEC_HIGH,
> +				BT_IO_OPT_INVALID)) {
> +			gatt_read_char(gatt->attrib, chr->handle,
> +					update_char_value, current);
> +			return;
> +		}
> +	}

The main problem with this is that it seems to be blocking. We can't
have that in a single threaded process like bluetoothd.

What I think the kernel should do when increasing the security level
with setsockopt is to mark the socket in a special state where neither
reads nor writes will succeed. Once the authentication is complete the
kernel would indicate POLLOUT on the socket, i.e. more or less the same
behavior as with non-blocking connects. If the authentication fails the
kernel would disconnect the socket.

We should also implement the same behavior for BR/EDR L2CAP sockets.
It'll particularly be useful for the HID implementation which right now
uses a raw HCI socket to request authentication and encryption when it
notices that the connected device is a keyboard (and not a mouse). The
whole thing could be abstracted by BtIO using something like
bt_io_set_security which would take a BtIOConnect callback pointer to
notify completion.

Johan

Re: [PATCH 1/2] Change security level on demand when reading characteristic value

[Claudio Takahasi]

Hi Johan,

On Fri, Dec 31, 2010 at 5:31 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Claudio,
>
> On Thu, Dec 30, 2010, Claudio Takahasi wrote:
>> If a characteristic requires a higher security level, change it on
>> demand and re-send the GATT Charateristic Value Read. Request will not
>> be sent until the SMP negotiation finishes. This change doesn't affect
>> GATT over BR/EDR, since encryption is mandatory for BR/EDR.
>> ---
>>  attrib/client.c  |   11 +++++++++++
>>  attrib/gattrib.c |    8 ++++++++
>>  attrib/gattrib.h |    2 ++
>>  3 files changed, 21 insertions(+), 0 deletions(-)
>>
>> diff --git a/attrib/client.c b/attrib/client.c
>> index 10bbf7d..3297a0c 100644
>> --- a/attrib/client.c
>> +++ b/attrib/client.c
>> @@ -744,6 +744,17 @@ static void update_char_value(guint8 status, const guint8 *pdu,
>>
>>       if (status == 0)
>>               characteristic_set_value(chr, pdu + 1, len - 1);
>> +     else if (status == ATT_ECODE_INSUFF_ENC) {
>> +             GIOChannel *io = g_attrib_get_channel(gatt->attrib);
>> +
>> +             if (bt_io_set(io, BT_IO_L2CAP, NULL,
>> +                             BT_IO_OPT_SEC_LEVEL, BT_IO_SEC_HIGH,
>> +                             BT_IO_OPT_INVALID)) {
>> +                     gatt_read_char(gatt->attrib, chr->handle,
>> +                                     update_char_value, current);
>> +                     return;
>> +             }
>> +     }
>
> The main problem with this is that it seems to be blocking. We can't
> have that in a single threaded process like bluetoothd.
>
> What I think the kernel should do when increasing the security level
> with setsockopt is to mark the socket in a special state where neither
> reads nor writes will succeed. Once the authentication is complete the
> kernel would indicate POLLOUT on the socket, i.e. more or less the same
> behavior as with non-blocking connects. If the authentication fails the
> kernel would disconnect the socket.

It is not blocking. The previous request is added again in the GAttrib
queue, but it is not sen't. POLLOUT is only sent when SMP finishes.
There is only one minor problem: we always add a new request in the
tail of the queue. Meaning that if there is more than one request
queued, the initial request will be the last request to be sent after
the SMP negotiation.

>
> We should also implement the same behavior for BR/EDR L2CAP sockets.
> It'll particularly be useful for the HID implementation which right now
> uses a raw HCI socket to request authentication and encryption when it
> notices that the connected device is a keyboard (and not a mouse). The
> whole thing could be abstracted by BtIO using something like
> bt_io_set_security which would take a BtIOConnect callback pointer to
> notify completion.
>
> Johan

I agree! New task for 2011 ;-)

Best Regards,
Claudio

Re: [PATCH 1/2] Change security level on demand when reading characteristic value

[Johan Hedberg]

Hi Claudio,

On Thu, Dec 30, 2010, Claudio Takahasi wrote:
> If a characteristic requires a higher security level, change it on
> demand and re-send the GATT Charateristic Value Read. Request will not
> be sent until the SMP negotiation finishes. This change doesn't affect
> GATT over BR/EDR, since encryption is mandatory for BR/EDR.
> ---
>  attrib/client.c  |   11 +++++++++++
>  attrib/gattrib.c |    8 ++++++++
>  attrib/gattrib.h |    2 ++
>  3 files changed, 21 insertions(+), 0 deletions(-)

I went ahead and pushed these two patches upstream, but like I said it'd
be nice to get some proper BtIO abstraction for all this including also
BR/EDR support. And even before that I wouldn't mind a patch to add a
proper code comment that explains how this works (with the POLLOUT).

Johan