[PATCH 1/6] android/gatt: Fix client signed write on encrypted link

[PATCH 1/6] android/gatt: Fix client signed write on encrypted link

[Szymon Janc]

As defined in Core Specification 4.2:
"If a connection is already encrypted with LE security mode 1, level 2
or level 3 as defined in [Vol 3] Part C, Section 10.2 then, a Write
Without Response as defined in Section 4.9.1 shall be used instead of
a Signed Write Without Response."
---
 android/gatt.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index 53b1983..9e348d7 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -3358,14 +3358,14 @@ static void handle_client_write_characteristic(const void *buf, uint16_t len)
 			goto failed;
 		}
 
-		if (get_sec_level(conn->device) != BT_SECURITY_LOW) {
-			error("gatt: Cannot write signed on encrypted link");
-			status = HAL_STATUS_FAILED;
-			goto failed;
-		}
-
-		res = signed_write_cmd(conn->device, ch->ch.value_handle,
-							cmd->value, cmd->len);
+		if (get_sec_level(conn->device) > BT_SECURITY_LOW)
+			res = gatt_write_cmd(conn->device->attrib,
+						ch->ch.value_handle, cmd->value,
+						cmd->len, NULL, NULL);
+		else
+			res = signed_write_cmd(conn->device,
+						ch->ch.value_handle, cmd->value,
+						cmd->len);
 		break;
 	default:
 		error("gatt: Write type %d unsupported", cmd->write_type);
-- 
1.9.3

[PATCH 2/6] android/bluetooth: Store if CSRK is authenticated

[Szymon Janc]

This adds support for storing information if CSRK is authenticated.
---
 android/bluetooth.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/android/bluetooth.c b/android/bluetooth.c
index d017418..dca6f48 100644
--- a/android/bluetooth.c
+++ b/android/bluetooth.c
@@ -120,10 +120,12 @@ struct device {
 	unsigned int confirm_id; /* mgtm command id if command pending */
 
 	bool valid_remote_csrk;
+	bool remote_csrk_auth;
 	uint8_t remote_csrk[16];
 	uint32_t remote_sign_cnt;
 
 	bool valid_local_csrk;
+	bool local_csrk_auth;
 	uint8_t local_csrk[16];
 	uint32_t local_sign_cnt;
 	uint16_t gatt_ccc;
@@ -2320,6 +2322,9 @@ static void store_csrk(struct device *dev)
 							dev->local_csrk[i]);
 
 		g_key_file_set_string(key_file, addr, "LocalCSRK", key_str);
+
+		g_key_file_set_boolean(key_file, addr, "LocalCSRKAuthenticated",
+							dev->local_csrk_auth);
 	}
 
 	if (dev->valid_remote_csrk) {
@@ -2328,6 +2333,10 @@ static void store_csrk(struct device *dev)
 							dev->remote_csrk[i]);
 
 		g_key_file_set_string(key_file, addr, "RemoteCSRK", key_str);
+
+		g_key_file_set_boolean(key_file, addr,
+						"RemoteCSRKAuthenticated",
+						dev->remote_csrk_auth);
 	}
 
 	data = g_key_file_to_data(key_file, &length, NULL);
@@ -2360,12 +2369,14 @@ static void new_csrk_callback(uint16_t index, uint16_t length,
 		memcpy(dev->local_csrk, ev->key.val, 16);
 		dev->local_sign_cnt = 0;
 		dev->valid_local_csrk = true;
+		dev->local_csrk_auth = ev->key.type == 0x02;
 		break;
 	case 0x01:
 	case 0x03:
 		memcpy(dev->remote_csrk, ev->key.val, 16);
 		dev->remote_sign_cnt = 0;
 		dev->valid_remote_csrk = true;
+		dev->remote_csrk_auth = ev->key.type == 0x03;
 		break;
 	default:
 		error("Unknown CSRK key type 02%02x", ev->key.type);
@@ -2962,6 +2973,9 @@ static struct device *create_device_from_info(GKeyFile *key_file,
 
 		dev->local_sign_cnt = g_key_file_get_integer(key_file, peer,
 						"LocalCSRKSignCounter", NULL);
+
+		dev->local_csrk_auth = g_key_file_get_boolean(key_file, peer,
+						"LocalCSRKAuthenticated", NULL);
 	}
 
 	str = g_key_file_get_string(key_file, peer, "RemoteCSRK", NULL);
@@ -2976,6 +2990,10 @@ static struct device *create_device_from_info(GKeyFile *key_file,
 
 		dev->remote_sign_cnt = g_key_file_get_integer(key_file, peer,
 						"RemoteCSRKSignCounter", NULL);
+
+		dev->remote_csrk_auth = g_key_file_get_boolean(key_file, peer,
+						"RemoteCSRKAuthenticated",
+						NULL);
 	}
 
 	str = g_key_file_get_string(key_file, peer, "GattCCC", NULL);
-- 
1.9.3

[PATCH 3/6] android/bluetooth: Add support for CSRK authetication level

[Szymon Janc]

This allows to get information if CSRK is authenticated or not.
For client it is don't care if CSRK is authenticated or not.
---
 android/bluetooth.c | 35 ++++++++++++++++++++++-------------
 android/bluetooth.h | 12 +++---------
 android/gatt.c      |  8 ++++----
 3 files changed, 29 insertions(+), 26 deletions(-)

diff --git a/android/bluetooth.c b/android/bluetooth.c
index dca6f48..e684d31 100644
--- a/android/bluetooth.c
+++ b/android/bluetooth.c
@@ -4098,22 +4098,33 @@ bool bt_read_device_rssi(const bdaddr_t *addr, bt_read_device_rssi_done cb,
 	return true;
 }
 
-bool bt_get_csrk(const bdaddr_t *addr, enum bt_csrk_type type, uint8_t key[16],
-							uint32_t *sign_cnt)
+bool bt_get_csrk(const bdaddr_t *addr, bool local, uint8_t key[16],
+					uint32_t *sign_cnt, bool *authenticated)
 {
 	struct device *dev;
-	bool local = (type == LOCAL_CSRK);
 
 	dev = find_device(addr);
 	if (!dev)
 		return false;
 
 	if (local && dev->valid_local_csrk) {
-		memcpy(key, dev->local_csrk, 16);
-		*sign_cnt = dev->local_sign_cnt;
+		if (key)
+			memcpy(key, dev->local_csrk, 16);
+
+		if (sign_cnt)
+			*sign_cnt = dev->local_sign_cnt;
+
+		if (authenticated)
+			*authenticated = dev->local_csrk_auth;
 	} else if (!local && dev->valid_remote_csrk) {
-		memcpy(key, dev->remote_csrk, 16);
-		*sign_cnt = dev->remote_sign_cnt;
+		if (key)
+			memcpy(key, dev->remote_csrk, 16);
+
+		if (sign_cnt)
+			*sign_cnt = dev->remote_sign_cnt;
+
+		if (authenticated)
+			*authenticated = dev->remote_csrk_auth;
 	} else {
 		return false;
 	}
@@ -4121,12 +4132,11 @@ bool bt_get_csrk(const bdaddr_t *addr, enum bt_csrk_type type, uint8_t key[16],
 	return true;
 }
 
-static void store_sign_counter(struct device *dev, enum bt_csrk_type type)
+static void store_sign_counter(struct device *dev, bool local)
 {
 	const char *sign_cnt_s;
 	uint32_t sign_cnt;
 	GKeyFile *key_file;
-	bool local = (type == LOCAL_CSRK);
 
 	gsize length = 0;
 	char addr[18];
@@ -4152,8 +4162,7 @@ static void store_sign_counter(struct device *dev, enum bt_csrk_type type)
 	g_key_file_free(key_file);
 }
 
-void bt_update_sign_counter(const bdaddr_t *addr, enum bt_csrk_type type,
-								uint32_t val)
+void bt_update_sign_counter(const bdaddr_t *addr, bool local, uint32_t val)
 {
 	struct device *dev;
 
@@ -4161,12 +4170,12 @@ void bt_update_sign_counter(const bdaddr_t *addr, enum bt_csrk_type type,
 	if (!dev)
 		return;
 
-	if (type == LOCAL_CSRK)
+	if (local)
 		dev->local_sign_cnt = val;
 	else
 		dev->remote_sign_cnt = val;
 
-	store_sign_counter(dev, type);
+	store_sign_counter(dev, local);
 }
 
 static uint8_t set_adapter_scan_mode(const void *buf, uint16_t len)
diff --git a/android/bluetooth.h b/android/bluetooth.h
index 9e7ab2c..8b8a1f0 100644
--- a/android/bluetooth.h
+++ b/android/bluetooth.h
@@ -21,11 +21,6 @@
  *
  */
 
-enum bt_csrk_type {
-	LOCAL_CSRK,
-	REMOTE_CSRK,
-};
-
 typedef void (*bt_bluetooth_ready)(int err, const bdaddr_t *addr);
 bool bt_bluetooth_start(int index, bool mgmt_dbg, bt_bluetooth_ready cb);
 
@@ -69,11 +64,10 @@ typedef void (*bt_read_device_rssi_done)(uint8_t status, const bdaddr_t *addr,
 bool bt_read_device_rssi(const bdaddr_t *addr, bt_read_device_rssi_done cb,
 							void *user_data);
 
-bool bt_get_csrk(const bdaddr_t *addr, enum bt_csrk_type type,
-					uint8_t key[16], uint32_t *sign_cnt);
+bool bt_get_csrk(const bdaddr_t *addr, bool local, uint8_t key[16],
+				uint32_t *sign_cnt, bool *authenticated);
 
-void bt_update_sign_counter(const bdaddr_t *addr, enum bt_csrk_type type,
-								uint32_t val);
+void bt_update_sign_counter(const bdaddr_t *addr, bool local, uint32_t val);
 
 void bt_store_gatt_ccc(const bdaddr_t *addr, uint16_t value);
 
diff --git a/android/gatt.c b/android/gatt.c
index 9e348d7..3f98e71 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -3263,7 +3263,7 @@ static guint signed_write_cmd(struct gatt_device *dev, uint16_t handle,
 
 	memset(csrk, 0, 16);
 
-	if (!bt_get_csrk(&dev->bdaddr, LOCAL_CSRK, csrk, &sign_cnt)) {
+	if (!bt_get_csrk(&dev->bdaddr, true, csrk, &sign_cnt, NULL)) {
 		error("gatt: Could not get csrk key");
 		return 0;
 	}
@@ -3275,7 +3275,7 @@ static guint signed_write_cmd(struct gatt_device *dev, uint16_t handle,
 		return 0;
 	}
 
-	bt_update_sign_counter(&dev->bdaddr, LOCAL_CSRK, ++sign_cnt);
+	bt_update_sign_counter(&dev->bdaddr, true, ++sign_cnt);
 
 	return res;
 }
@@ -6396,7 +6396,7 @@ static void write_signed_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
 		return;
 	}
 
-	if (!bt_get_csrk(&dev->bdaddr, REMOTE_CSRK, csrk, &sign_cnt)) {
+	if (!bt_get_csrk(&dev->bdaddr, false, csrk, &sign_cnt, NULL)) {
 		error("gatt: No valid csrk from remote device");
 		return;
 	}
@@ -6438,7 +6438,7 @@ static void write_signed_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
 			return;
 		}
 		/* Signature OK, proceed with write */
-		bt_update_sign_counter(&dev->bdaddr, REMOTE_CSRK, r_sign_cnt);
+		bt_update_sign_counter(&dev->bdaddr, false, r_sign_cnt);
 		gatt_db_attribute_write(attrib, 0, value, vlen, cmd[0],
 						g_attrib_get_att(dev->attrib),
 						write_confirm, NULL);
-- 
1.9.3

[PATCH 4/6] android/gatt: Fix check if CSRK is authenticated

[Szymon Janc]

---
 android/gatt.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index 3f98e71..2e3a5c9 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -4683,9 +4683,15 @@ static uint8_t check_device_permissions(struct gatt_device *device,
 		if (!(permissions & GATT_PERM_WRITE_SIGNED))
 				return ATT_ECODE_WRITE_NOT_PERM;
 
-		if ((permissions & GATT_PERM_WRITE_SIGNED_MITM) &&
-						sec_level < BT_SECURITY_HIGH)
+		if (permissions & GATT_PERM_WRITE_SIGNED_MITM) {
+			bool auth;
+
+			if (bt_get_csrk(&device->bdaddr, true, NULL, NULL,
+					&auth) && auth)
+				break;
+
 			return ATT_ECODE_AUTHENTICATION;
+		}
 		break;
 	case ATT_OP_READ_BY_TYPE_REQ:
 	case ATT_OP_READ_REQ:
-- 
1.9.3

[PATCH 5/6] android/gatt: Fix error message in signed_write_cmd

[Szymon Janc]

---
 android/gatt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/android/gatt.c b/android/gatt.c
index 2e3a5c9..6e9181e 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -3271,7 +3271,7 @@ static guint signed_write_cmd(struct gatt_device *dev, uint16_t handle,
 	res = gatt_signed_write_cmd(dev->attrib, handle, value, vlen, crypto,
 						csrk, sign_cnt, NULL, NULL);
 	if (!res) {
-		error("gatt: Could write signed cmd");
+		error("gatt: Could not write signed cmd");
 		return 0;
 	}
 
-- 
1.9.3

[PATCH 6/6] shared/att: Fix signed write without transparent signing

[Szymon Janc]

If local key wasn't set by bt_att_set_local_key() assume that signing
is done by bt_att_send() caller ie. gattrib code.
---
 src/shared/att.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/shared/att.c b/src/shared/att.c
index 9787377..b20af21 100644
--- a/src/shared/att.c
+++ b/src/shared/att.c
@@ -308,7 +308,7 @@ static bool encode_pdu(struct bt_att *att, struct att_send_op *op,
 
 	sign = att->local_sign;
 	if (!sign)
-		goto fail;
+		true;
 
 	if (!sign->counter(&sign_cnt, sign->user_data))
 		goto fail;
-- 
1.9.3

Re: [PATCH 6/6] shared/att: Fix signed write without transparent signing

[Michael Janssen]

Hi Szymon,

On Fri, Feb 27, 2015 at 7:01 AM, Szymon Janc <szymon.janc@tieto.com> wrote:
> If local key wasn't set by bt_att_set_local_key() assume that signing
> is done by bt_att_send() caller ie. gattrib code.
> ---
>  src/shared/att.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/shared/att.c b/src/shared/att.c
> index 9787377..b20af21 100644
> --- a/src/shared/att.c
> +++ b/src/shared/att.c
> @@ -308,7 +308,7 @@ static bool encode_pdu(struct bt_att *att, struct att_send_op *op,
>
>         sign = att->local_sign;
>         if (!sign)
> -               goto fail;
> +               true;

This should be 'return true;' I think?

>
>         if (!sign->counter(&sign_cnt, sign->user_data))
>                 goto fail;
> --
> 1.9.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Michael Janssen

Re: [PATCH 6/6] shared/att: Fix signed write without transparent signing

[Szymon Janc]

Hi Michael,

On Friday 27 of February 2015 07:21:51 Michael Janssen wrote:
> Hi Szymon,
> 
> On Fri, Feb 27, 2015 at 7:01 AM, Szymon Janc <szymon.janc@tieto.com> wrote:
> > If local key wasn't set by bt_att_set_local_key() assume that signing
> > is done by bt_att_send() caller ie. gattrib code.
> > ---
> >  src/shared/att.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/src/shared/att.c b/src/shared/att.c
> > index 9787377..b20af21 100644
> > --- a/src/shared/att.c
> > +++ b/src/shared/att.c
> > @@ -308,7 +308,7 @@ static bool encode_pdu(struct bt_att *att, struct att_send_op *op,
> >
> >         sign = att->local_sign;
> >         if (!sign)
> > -               goto fail;
> > +               true;
> 
> This should be 'return true;' I think?

Good catch. I've got this right locally but forgot to amend before generating
patch :)

> 
> >
> >         if (!sign->counter(&sign_cnt, sign->user_data))
> >                 goto fail;
> > --
> > 1.9.3
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> 
> 

-- 
Best regards, 
Szymon Janc

Re: [PATCH 6/6] shared/att: Fix signed write without transparent signing

[Luiz Augusto von Dentz]

Hi Szymon,

On Fri, Feb 27, 2015 at 5:27 PM, Szymon Janc <szymon.janc@tieto.com> wrote:
> Hi Michael,
>
> On Friday 27 of February 2015 07:21:51 Michael Janssen wrote:
>> Hi Szymon,
>>
>> On Fri, Feb 27, 2015 at 7:01 AM, Szymon Janc <szymon.janc@tieto.com> wrote:
>> > If local key wasn't set by bt_att_set_local_key() assume that signing
>> > is done by bt_att_send() caller ie. gattrib code.
>> > ---
>> >  src/shared/att.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/src/shared/att.c b/src/shared/att.c
>> > index 9787377..b20af21 100644
>> > --- a/src/shared/att.c
>> > +++ b/src/shared/att.c
>> > @@ -308,7 +308,7 @@ static bool encode_pdu(struct bt_att *att, struct att_send_op *op,
>> >
>> >         sign = att->local_sign;
>> >         if (!sign)
>> > -               goto fail;
>> > +               true;
>>
>> This should be 'return true;' I think?
>
> Good catch. I've got this right locally but forgot to amend before generating
> patch :)

Yep, but as discussed in the IRC there are more problems with it
because we increase the data length before this, I guess we are doing
to need some refactoring to fix this properly.


-- 
Luiz Augusto von Dentz

Re: [PATCH 1/6] android/gatt: Fix client signed write on encrypted link

[Szymon Janc]

On Friday 27 of February 2015 16:01:09 Szymon Janc wrote:
> As defined in Core Specification 4.2:
> "If a connection is already encrypted with LE security mode 1, level 2
> or level 3 as defined in [Vol 3] Part C, Section 10.2 then, a Write
> Without Response as defined in Section 4.9.1 shall be used instead of
> a Signed Write Without Response."
> ---
>  android/gatt.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/android/gatt.c b/android/gatt.c
> index 53b1983..9e348d7 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -3358,14 +3358,14 @@ static void handle_client_write_characteristic(const
> void *buf, uint16_t len) goto failed;
>  		}
> 
> -		if (get_sec_level(conn->device) != BT_SECURITY_LOW) {
> -			error("gatt: Cannot write signed on encrypted link");
> -			status = HAL_STATUS_FAILED;
> -			goto failed;
> -		}
> -
> -		res = signed_write_cmd(conn->device, ch->ch.value_handle,
> -							cmd->value, cmd->len);
> +		if (get_sec_level(conn->device) > BT_SECURITY_LOW)
> +			res = gatt_write_cmd(conn->device->attrib,
> +						ch->ch.value_handle, cmd->value,
> +						cmd->len, NULL, NULL);
> +		else
> +			res = signed_write_cmd(conn->device,
> +						ch->ch.value_handle, cmd->value,
> +						cmd->len);
>  		break;
>  	default:
>  		error("gatt: Write type %d unsupported", cmd->write_type);

Patches 1-5 are applied.

-- 
BR
Szymon Janc