[PATCH] network: Check full BNEP UUID

[PATCH] network: Check full BNEP UUID

[Par-Gunnar Hjalmdahl]

This patch fixes an issue where only the 2 bytes containing
the service ID was checked from the BNEP UUID.
Fixes behavior for BT testcases TP/PAN/MISC/UUID/BV-01-C &
TP/PAN/MISC/UUID/BV-02-C.
---
 profiles/network/server.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/profiles/network/server.c b/profiles/network/server.c
index 480c7e2..15ea1cb 100644
--- a/profiles/network/server.c
+++ b/profiles/network/server.c
@@ -54,6 +54,11 @@
 #define NETWORK_SERVER_INTERFACE "org.bluez.NetworkServer"
 #define SETUP_TIMEOUT		1
 
+static uint128_t bluetooth_base_uuid = {
+	.data = {0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10, 0x00,
+			0x80, 0x00, 0x00, 0x80, 0x5F, 0x9B, 0x34, 0xFB}
+};
+
 /* Pending Authorization */
 struct network_session {
 	bdaddr_t	dst;		/* Remote Bluetooth Address */
@@ -313,6 +318,22 @@ static uint16_t bnep_setup_decode(struct bnep_setup_conn_req *req,
 		break;
 	case 4: /* UUID32 */
 	case 16: /* UUID128 */
+		/*
+		 * Check that the bytes in the UUID, except the service ID itself, are
+		 * correct. The service ID is checked in bnep_setup_chk().
+		 */
+		if (memcmp(dest, bluetooth_base_uuid.data, 2))
+			return BNEP_CONN_INVALID_DST;
+		if (memcmp(source, bluetooth_base_uuid.data, 2))
+			return BNEP_CONN_INVALID_SRC;
+
+		if (req->uuid_size == 16) {
+			if (memcmp(&dest[4], &bluetooth_base_uuid.data[4], 12))
+				return BNEP_CONN_INVALID_DST;
+			if (memcmp(&source[4], &bluetooth_base_uuid.data[4], 12))
+				return BNEP_CONN_INVALID_SRC;
+		}
+
 		*dst_role = bt_get_be32(dest);
 		*src_role = bt_get_be32(source);
 		break;
-- 
1.7.11.1

Re: [PATCH] network: Check full BNEP UUID

[Johan Hedberg]

[-- Attachment #1: Type: text/plain, Size: 2916 bytes --]

Hi,

On Fri, Jul 27, 2012, Par-Gunnar Hjalmdahl wrote:
> This patch fixes an issue where only the 2 bytes containing
> the service ID was checked from the BNEP UUID.
> Fixes behavior for BT testcases TP/PAN/MISC/UUID/BV-01-C &
> TP/PAN/MISC/UUID/BV-02-C.
> ---
>  profiles/network/server.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/profiles/network/server.c b/profiles/network/server.c
> index 480c7e2..15ea1cb 100644
> --- a/profiles/network/server.c
> +++ b/profiles/network/server.c
> @@ -54,6 +54,11 @@
>  #define NETWORK_SERVER_INTERFACE "org.bluez.NetworkServer"
>  #define SETUP_TIMEOUT		1
>  
> +static uint128_t bluetooth_base_uuid = {
> +	.data = {0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10, 0x00,
> +			0x80, 0x00, 0x00, 0x80, 0x5F, 0x9B, 0x34, 0xFB}
> +};
> +
>  /* Pending Authorization */
>  struct network_session {
>  	bdaddr_t	dst;		/* Remote Bluetooth Address */
> @@ -313,6 +318,22 @@ static uint16_t bnep_setup_decode(struct bnep_setup_conn_req *req,
>  		break;
>  	case 4: /* UUID32 */
>  	case 16: /* UUID128 */
> +		/*
> +		 * Check that the bytes in the UUID, except the service ID itself, are
> +		 * correct. The service ID is checked in bnep_setup_chk().
> +		 */
> +		if (memcmp(dest, bluetooth_base_uuid.data, 2))
> +			return BNEP_CONN_INVALID_DST;
> +		if (memcmp(source, bluetooth_base_uuid.data, 2))
> +			return BNEP_CONN_INVALID_SRC;
> +
> +		if (req->uuid_size == 16) {
> +			if (memcmp(&dest[4], &bluetooth_base_uuid.data[4], 12))
> +				return BNEP_CONN_INVALID_DST;
> +			if (memcmp(&source[4], &bluetooth_base_uuid.data[4], 12))
> +				return BNEP_CONN_INVALID_SRC;
> +		}
> +
>  		*dst_role = bt_get_be32(dest);
>  		*src_role = bt_get_be32(source);
>  		break;

There are a couple of things that bug me a bit with this patch. One is
the re-definition of bluetooth_base_uuid which really should only exist
in one central place (lib/uuid.c). Another is the way the code logic
goes. It seems you want to test for two things:

	1) That the UUID is a Bluetooth UUID. This is the memcmp with
	the last 12 bytes of the Bluetooth base UUID for the UUID128
	case. 16 and 32 bit values are already implicitly assumed to be
	Bluetooth UUIDs as they don't contain any information to reveal
	this as such.

	2) That the (now determined) Bluetooth UUID has a value less
	than or equal to 0xffff. This is quite awkwardly tested for by
	comparing with the two first bytes from bluetooth_base_uuid,
	which are both zeros. Based on your code comment it sounds like
	this test should really be covered by bnep_setup_chk().

If you really want to have the "<= 0xffff" test be in bnep_setup_decode
then I'd propose something like the attached patch. However, testing for
the exact value of the Bluetooth UUID (once the UUID has been determined
to be a Bluetooth UUID) seems like the task of bnep_setup_chk from the
way you've laid out the functions.

Johan

[-- Attachment #2: 0001-network-Check-full-BNEP-UUID.patch --]
[-- Type: text/plain, Size: 2064 bytes --]

>From 52fd85377f6784d69beec0ec7911bdf4b9621077 Mon Sep 17 00:00:00 2001
From: Par-Gunnar Hjalmdahl <par-gunnar.hjalmdahl@stericsson.com>
Date: Fri, 27 Jul 2012 11:06:40 +0200
Subject: [PATCH] network: Check full BNEP UUID

This patch fixes an issue where only the 2 bytes containing the service
ID was checked from the BNEP UUID. Fixes behavior for BT test cases
TP/PAN/MISC/UUID/BV-01-C & TP/PAN/MISC/UUID/BV-02-C.
---
 profiles/network/server.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/profiles/network/server.c b/profiles/network/server.c
index 480c7e2..8ae608c 100644
--- a/profiles/network/server.c
+++ b/profiles/network/server.c
@@ -301,7 +301,10 @@ static uint16_t bnep_setup_chk(uint16_t dst_role, uint16_t src_role)
 static uint16_t bnep_setup_decode(struct bnep_setup_conn_req *req,
 				uint16_t *dst_role, uint16_t *src_role)
 {
+	const uint8_t bt_base[] = { 0x00, 0x00, 0x10, 0x00, 0x80, 0x00,
+				0x00, 0x80, 0x5F, 0x9B, 0x34, 0xFB };
 	uint8_t *dest, *source;
+	uint32_t val;
 
 	dest = req->service;
 	source = req->service + req->uuid_size;
@@ -311,10 +314,27 @@ static uint16_t bnep_setup_decode(struct bnep_setup_conn_req *req,
 		*dst_role = bt_get_be16(dest);
 		*src_role = bt_get_be16(source);
 		break;
-	case 4: /* UUID32 */
 	case 16: /* UUID128 */
-		*dst_role = bt_get_be32(dest);
-		*src_role = bt_get_be32(source);
+		/* Check that the bytes in the UUID, except the service ID
+		 * itself, are correct. The service ID is checked in
+		 * bnep_setup_chk(). */
+		if (memcmp(&dest[4], bt_base, sizeof(bt_base)) != 0)
+			return BNEP_CONN_INVALID_DST;
+		if (memcmp(&source[4], bt_base, sizeof(bt_base)) != 0)
+			return BNEP_CONN_INVALID_SRC;
+
+		/* Intentional no-break */
+
+	case 4: /* UUID32 */
+		val = bt_get_be32(dest);
+		if (val > 0xffff)
+			return BNEP_CONN_INVALID_DST;
+		*dst_role = val;
+
+		val = bt_get_be32(source);
+		if (val > 0xffff)
+			return BNEP_CONN_INVALID_SRC;
+		*src_role = val;
 		break;
 	default:
 		return BNEP_CONN_INVALID_SVC;
-- 
1.7.11.2

RE: [PATCH] network: Check full BNEP UUID

[Par-Gunnar HJALMDAHL]

Hi and thanks for you comments Johan,

> -----Original Message-----
> From: Johan Hedberg [mailto:johan.hedberg@gmail.com]
> Sent: den 15 augusti 2012 12:39
> To: Par-Gunnar HJALMDAHL
> Cc: linux-bluetooth@vger.kernel.org; Anurag GUPTA-1
> Subject: Re: [PATCH] network: Check full BNEP UUID
> 
> There are a couple of things that bug me a bit with this patch. One is
> the re-definition of bluetooth_base_uuid which really should only exist
> in one central place (lib/uuid.c). Another is the way the code logic
> goes. It seems you want to test for two things:
> 
> 	1) That the UUID is a Bluetooth UUID. This is the memcmp
> with
> 	the last 12 bytes of the Bluetooth base UUID for the UUID128
> 	case. 16 and 32 bit values are already implicitly assumed to
> be
> 	Bluetooth UUIDs as they don't contain any information to
> reveal
> 	this as such.
> 
> 	2) That the (now determined) Bluetooth UUID has a value less
> 	than or equal to 0xffff. This is quite awkwardly tested for
> by
> 	comparing with the two first bytes from bluetooth_base_uuid,
> 	which are both zeros. Based on your code comment it sounds
> like
> 	this test should really be covered by bnep_setup_chk().
> 
> If you really want to have the "<= 0xffff" test be in bnep_setup_decode
> then I'd propose something like the attached patch. However, testing
> for the exact value of the Bluetooth UUID (once the UUID has been
> determined to be a Bluetooth UUID) seems like the task of
> bnep_setup_chk from the way you've laid out the functions.
> 
> Johan

Basically I tried to keep the patch to a minimum since it was quite a 
simple issue to fix.

I have no problems to use the UUID in lib/uuid.c. I could add a 2 new
exported functions for retrieving base UUID, one for little endian and
one for big endian, since uuid.c endian depends on CPU endian while
PAN endian is always big endian. So:
void get_le_bt_base_uuid(uint128_t *uuid);
void get_be_bt_base_uuid(uint128_t *uuid);

Your suggestion looks fine. It's a bit more effective than my original
code. The reason I did the check in bnep_setup_decode() rather than in
bnep_setup_chk is that only 16 bits are kept when exiting the function
(bnep_setup_decode is the only function aware of the UUID size) and I
thought that was a quite straightforward design.

If it's OK with you I can create a new patch with suggestion above
plus your patch.

/P-G

Re: [PATCH] network: Check full BNEP UUID

[Johan Hedberg]

Hi P-G,

On Wed, Aug 15, 2012, Par-Gunnar HJALMDAHL wrote:
> Basically I tried to keep the patch to a minimum since it was quite a 
> simple issue to fix.
> 
> I have no problems to use the UUID in lib/uuid.c. I could add a 2 new
> exported functions for retrieving base UUID, one for little endian and
> one for big endian, since uuid.c endian depends on CPU endian while
> PAN endian is always big endian. So:
> void get_le_bt_base_uuid(uint128_t *uuid);
> void get_be_bt_base_uuid(uint128_t *uuid);

Note that the uuid.{c,h} name space is bt_uuid_*

So far uuid.c has been fully hiding any endianness issues in its public
API by only talking host-endianness. So, if this was to be really done
in a clean way I'd say the calling code should look like:

	uint128_t u128;
	bt_uuid_t uuid;

	u128 = bt_get_be128(dest);

	bt_uuid128_create(&uuid, u128);
	if (!bt_uuid_is_bluetooth(&uuid))
		return BNEP_CONN_INVALID_DST;

	/* same for source */

However, as this would require at least two new functions
(bt_get_be128() in lib/bluetooth.h and bt_uuid_is_bluetooth() in
lib/uuid.c) and like you say the use case is quite trivial, I think this
might be a bit overkill.

So, since you were fine with my alteration of your patch I think I'll
just go ahead and push it upstream. If you still want to add the
necessary APIs for the "truly clean" approach, feel free, but I won't be
bugging you about it ;)

Johan

RE: [PATCH] network: Check full BNEP UUID

[Par-Gunnar HJALMDAHL]

Hi Johan,

> -----Original Message-----
> From: Johan Hedberg [mailto:johan.hedberg@gmail.com]
> Sent: den 15 augusti 2012 14:41
> To: Par-Gunnar HJALMDAHL
> Cc: linux-bluetooth@vger.kernel.org; Anurag GUPTA-1
> Subject: Re: [PATCH] network: Check full BNEP UUID
> 
> Hi P-G,
> 
> On Wed, Aug 15, 2012, Par-Gunnar HJALMDAHL wrote:
> > Basically I tried to keep the patch to a minimum since it was quite a
> > simple issue to fix.
> >
> > I have no problems to use the UUID in lib/uuid.c. I could add a 2 new
> > exported functions for retrieving base UUID, one for little endian
> and
> > one for big endian, since uuid.c endian depends on CPU endian while
> > PAN endian is always big endian. So:
> > void get_le_bt_base_uuid(uint128_t *uuid);
> > void get_be_bt_base_uuid(uint128_t *uuid);
> 
> Note that the uuid.{c,h} name space is bt_uuid_*
> 
> So far uuid.c has been fully hiding any endianness issues in its public
> API by only talking host-endianness. So, if this was to be really done
> in a clean way I'd say the calling code should look like:
> 
> 	uint128_t u128;
> 	bt_uuid_t uuid;
> 
> 	u128 = bt_get_be128(dest);
> 
> 	bt_uuid128_create(&uuid, u128);
> 	if (!bt_uuid_is_bluetooth(&uuid))
> 		return BNEP_CONN_INVALID_DST;
> 
> 	/* same for source */
> 
> However, as this would require at least two new functions
> (bt_get_be128() in lib/bluetooth.h and bt_uuid_is_bluetooth() in
> lib/uuid.c) and like you say the use case is quite trivial, I think
> this
> might be a bit overkill.
> 
> So, since you were fine with my alteration of your patch I think I'll
> just go ahead and push it upstream. If you still want to add the
> necessary APIs for the "truly clean" approach, feel free, but I won't
> be
> bugging you about it ;)
> 
> Johan

Yes, I'm fine with your suggested patch as-is.
It's OK to just push it. :-)

/P-G