All of lore.kernel.org
 help / color / mirror / Atom feed
* [Bluez PATCH v4 0/4] Check the signature of att packets
@ 2020-04-07  8:56 Archie Pusaka
  2020-04-07  8:56 ` [Bluez PATCH v4 1/4] shared/crypto: Add bt_crypto_verify_att_sign Archie Pusaka
                   ` (4 more replies)
  0 siblings, 5 replies; 8+ messages in thread
From: Archie Pusaka @ 2020-04-07  8:56 UTC (permalink / raw)
  To: linux-bluetooth, Luiz Augusto von Dentz; +Cc: Archie Pusaka

From: Archie Pusaka <apusaka@chromium.org>

According to bluetooth spec Ver 5.1, Vol 3, Part C (GAP), 10.4.2
A device receiving signed data shall authenticate it by performing
the Signing Algorithm. The signed data shall be authenticated by
performing the Signing Algorithm where m is the Data PDU to be
authenticated, k is the stored CSRK and the SignCounter is the
received counter value. If the MAC computed by the Signing
Algorithm does not match the received MAC, the verification fails
and the Host shall ignore the received Data PDU.

Currently bluez ignore the signature of received signed att
packets, as the function bt_crypto_sign_att() only generates the
signature, and not actually make any check about the genuineness
of the signature itself.

This patch also fix a wrong boolean condition which prevents
handle_signed() to be called.

Tested to pass these BT certification test
SM/MAS/SIGN/BV-03-C
SM/MAS/SIGN/BI-01-C

Changes in v4:
- Fix wrong variable assignment
- Fixing test-gatt.c

Changes in v3:
- Add check for the case where pdu_len < ATT_SIGN_LEN
- Add unit test
- Separate into three patches

Changes in v2:
- Move the signature verification part to crypto.c
- Attempt not to copy the whole pdu while verifying the signature
  by not separating the opcode from the rest of pdu too early, so
  we don't have to rejoin them later.

Archie Pusaka (4):
  shared/crypto: Add bt_crypto_verify_att_sign
  unit/test-crypto: test for bt_crypto_verify_att_sign
  shared/att: Check the signature of att packets
  unit/test-gatt: Fix unknown request with signed bit

 src/shared/att.c    | 25 +++++++++----------
 src/shared/crypto.c | 28 +++++++++++++++++++--
 src/shared/crypto.h |  2 ++
 unit/test-crypto.c  | 59 +++++++++++++++++++++++++++++++++++++++++++++
 unit/test-gatt.c    | 32 ++++++++++++++++++++----
 5 files changed, 126 insertions(+), 20 deletions(-)

-- 
2.26.0.292.g33ef6b2f38-goog


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [Bluez PATCH v4 1/4] shared/crypto: Add bt_crypto_verify_att_sign
  2020-04-07  8:56 [Bluez PATCH v4 0/4] Check the signature of att packets Archie Pusaka
@ 2020-04-07  8:56 ` Archie Pusaka
  2020-04-07  8:56 ` [Bluez PATCH v4 2/4] unit/test-crypto: test for bt_crypto_verify_att_sign Archie Pusaka
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 8+ messages in thread
From: Archie Pusaka @ 2020-04-07  8:56 UTC (permalink / raw)
  To: linux-bluetooth, Luiz Augusto von Dentz; +Cc: Archie Pusaka

From: Archie Pusaka <apusaka@chromium.org>

This is used to verify the signature of incoming ATT packets.
---

Changes in v4: None
Changes in v3:
- Add check for the case where pdu_len < ATT_SIGN_LEN

Changes in v2: None

 src/shared/crypto.c | 28 ++++++++++++++++++++++++++--
 src/shared/crypto.h |  2 ++
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/src/shared/crypto.c b/src/shared/crypto.c
index 5c5e1217d..5cc88ce4a 100644
--- a/src/shared/crypto.c
+++ b/src/shared/crypto.c
@@ -75,6 +75,8 @@ struct af_alg_iv {
 /* Maximum message length that can be passed to aes_cmac */
 #define CMAC_MSG_MAX	80
 
+#define ATT_SIGN_LEN	12
+
 struct bt_crypto {
 	int ref_count;
 	int ecb_aes;
@@ -265,7 +267,8 @@ static inline void swap_buf(const uint8_t *src, uint8_t *dst, uint16_t len)
 
 bool bt_crypto_sign_att(struct bt_crypto *crypto, const uint8_t key[16],
 				const uint8_t *m, uint16_t m_len,
-				uint32_t sign_cnt, uint8_t signature[12])
+				uint32_t sign_cnt,
+				uint8_t signature[ATT_SIGN_LEN])
 {
 	int fd;
 	int len;
@@ -319,10 +322,31 @@ bool bt_crypto_sign_att(struct bt_crypto *crypto, const uint8_t key[16],
 	 * 12 octets
 	 */
 	swap_buf(out, tmp, 16);
-	memcpy(signature, tmp + 4, 12);
+	memcpy(signature, tmp + 4, ATT_SIGN_LEN);
 
 	return true;
 }
+
+bool bt_crypto_verify_att_sign(struct bt_crypto *crypto, const uint8_t key[16],
+				const uint8_t *pdu, uint16_t pdu_len)
+{
+	uint8_t generated_sign[ATT_SIGN_LEN];
+	const uint8_t *sign;
+	uint32_t sign_cnt;
+
+	if (pdu_len < ATT_SIGN_LEN)
+		return false;
+
+	sign = pdu + pdu_len - ATT_SIGN_LEN;
+	sign_cnt = get_le32(sign);
+
+	if (!bt_crypto_sign_att(crypto, key, pdu, pdu_len - ATT_SIGN_LEN,
+						sign_cnt, generated_sign))
+		return false;
+
+	return memcmp(generated_sign, sign, ATT_SIGN_LEN) == 0;
+}
+
 /*
  * Security function e
  *
diff --git a/src/shared/crypto.h b/src/shared/crypto.h
index c58d2e104..d17daa835 100644
--- a/src/shared/crypto.h
+++ b/src/shared/crypto.h
@@ -62,5 +62,7 @@ bool bt_crypto_h6(struct bt_crypto *crypto, const uint8_t w[16],
 bool bt_crypto_sign_att(struct bt_crypto *crypto, const uint8_t key[16],
 				const uint8_t *m, uint16_t m_len,
 				uint32_t sign_cnt, uint8_t signature[12]);
+bool bt_crypto_verify_att_sign(struct bt_crypto *crypto, const uint8_t key[16],
+				const uint8_t *pdu, uint16_t pdu_len);
 bool bt_crypto_gatt_hash(struct bt_crypto *crypto, struct iovec *iov,
 				size_t iov_len, uint8_t res[16]);
-- 
2.26.0.292.g33ef6b2f38-goog


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [Bluez PATCH v4 2/4] unit/test-crypto: test for bt_crypto_verify_att_sign
  2020-04-07  8:56 [Bluez PATCH v4 0/4] Check the signature of att packets Archie Pusaka
  2020-04-07  8:56 ` [Bluez PATCH v4 1/4] shared/crypto: Add bt_crypto_verify_att_sign Archie Pusaka
@ 2020-04-07  8:56 ` Archie Pusaka
  2020-04-07  8:56 ` [Bluez PATCH v4 3/4] shared/att: Check the signature of att packets Archie Pusaka
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 8+ messages in thread
From: Archie Pusaka @ 2020-04-07  8:56 UTC (permalink / raw)
  To: linux-bluetooth, Luiz Augusto von Dentz; +Cc: Archie Pusaka

From: Archie Pusaka <apusaka@chromium.org>

Adding tests for verifying att signature
---

Changes in v4:
- Fix wrong variable assignment

Changes in v3:
- Add unit test

Changes in v2: None

 unit/test-crypto.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/unit/test-crypto.c b/unit/test-crypto.c
index e20b2fa66..46c7c0e5c 100644
--- a/unit/test-crypto.c
+++ b/unit/test-crypto.c
@@ -272,6 +272,58 @@ static void test_gatt_hash(gconstpointer data)
 	tester_test_passed();
 }
 
+struct verify_sign_test_data {
+	const uint8_t *msg;
+	uint16_t msg_len;
+	const uint8_t *key;
+	bool match;
+};
+
+static const uint8_t msg_to_verify_pass[] = {
+	0xd2, 0x12, 0x00, 0x13, 0x37, 0x01, 0x00, 0x00, 0x00, 0xF1, 0x87, 0x1E,
+	0x93, 0x3C, 0x90, 0x0F, 0xf2
+};
+
+static const struct verify_sign_test_data verify_sign_pass_data = {
+	.msg = msg_to_verify_pass,
+	.msg_len = sizeof(msg_to_verify_pass),
+	.key = key_5,
+	.match = true,
+};
+
+static const uint8_t msg_to_verify_bad_sign[] = {
+	0xd2, 0x12, 0x00, 0x13, 0x37, 0x01, 0x00, 0x00, 0x00, 0xF1, 0x87, 0x1E,
+	0x93, 0x3C, 0x90, 0x0F, 0xf1
+};
+
+static const struct verify_sign_test_data verify_sign_bad_sign_data = {
+	.msg = msg_to_verify_bad_sign,
+	.msg_len = sizeof(msg_to_verify_bad_sign),
+	.key = key_5,
+	.match = false,
+};
+
+static const uint8_t msg_to_verify_too_short[] = {
+	0xd2, 0x12, 0x00, 0x13, 0x37
+};
+
+static const struct verify_sign_test_data verify_sign_too_short_data = {
+	.msg = msg_to_verify_too_short,
+	.msg_len = sizeof(msg_to_verify_too_short),
+	.key = key_5,
+	.match = false,
+};
+
+static void test_verify_sign(gconstpointer data)
+{
+	const struct verify_sign_test_data *d = data;
+	bool result = bt_crypto_verify_att_sign(crypto, d->key, d->msg,
+						d->msg_len);
+	g_assert(result == d->match);
+
+	tester_test_passed();
+}
+
 int main(int argc, char *argv[])
 {
 	int exit_status;
@@ -292,6 +344,13 @@ int main(int argc, char *argv[])
 
 	tester_add("/crypto/gatt_hash", NULL, NULL, test_gatt_hash, NULL);
 
+	tester_add("/crypto/verify_sign_pass", &verify_sign_pass_data,
+						NULL, test_verify_sign, NULL);
+	tester_add("/crypto/verify_sign_bad_sign", &verify_sign_bad_sign_data,
+						NULL, test_verify_sign, NULL);
+	tester_add("/crypto/verify_sign_too_short", &verify_sign_too_short_data,
+						NULL, test_verify_sign, NULL);
+
 	exit_status = tester_run();
 
 	bt_crypto_unref(crypto);
-- 
2.26.0.292.g33ef6b2f38-goog


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [Bluez PATCH v4 3/4] shared/att: Check the signature of att packets
  2020-04-07  8:56 [Bluez PATCH v4 0/4] Check the signature of att packets Archie Pusaka
  2020-04-07  8:56 ` [Bluez PATCH v4 1/4] shared/crypto: Add bt_crypto_verify_att_sign Archie Pusaka
  2020-04-07  8:56 ` [Bluez PATCH v4 2/4] unit/test-crypto: test for bt_crypto_verify_att_sign Archie Pusaka
@ 2020-04-07  8:56 ` Archie Pusaka
  2020-04-07  8:56 ` [Bluez PATCH v4 4/4] unit/test-gatt: Fix unknown request with signed bit Archie Pusaka
  2020-04-07 20:07 ` [Bluez PATCH v4 0/4] Check the signature of att packets Luiz Augusto von Dentz
  4 siblings, 0 replies; 8+ messages in thread
From: Archie Pusaka @ 2020-04-07  8:56 UTC (permalink / raw)
  To: linux-bluetooth, Luiz Augusto von Dentz; +Cc: Archie Pusaka

From: Archie Pusaka <apusaka@chromium.org>

Tested to pass these BT certification test
SM/MAS/SIGN/BV-03-C
SM/MAS/SIGN/BI-01-C
---

Changes in v4: None
Changes in v3:
- Separate into three patches

Changes in v2:
- Move the signature verification part to crypto.c
- Attempt not to copy the whole pdu while verifying the signature
  by not separating the opcode from the rest of pdu too early, so
  we don't have to rejoin them later.

 src/shared/att.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/src/shared/att.c b/src/shared/att.c
index 948a5548b..31c6901fb 100644
--- a/src/shared/att.c
+++ b/src/shared/att.c
@@ -881,15 +881,15 @@ static void respond_not_supported(struct bt_att *att, uint8_t opcode)
 									NULL);
 }
 
-static bool handle_signed(struct bt_att *att, uint8_t opcode, uint8_t *pdu,
-								ssize_t pdu_len)
+static bool handle_signed(struct bt_att *att, uint8_t *pdu, ssize_t pdu_len)
 {
 	uint8_t *signature;
 	uint32_t sign_cnt;
 	struct sign_info *sign;
+	uint8_t opcode = pdu[0];
 
 	/* Check if there is enough data for a signature */
-	if (pdu_len < 2 + BT_ATT_SIGNATURE_LEN)
+	if (pdu_len < 3 + BT_ATT_SIGNATURE_LEN)
 		goto fail;
 
 	sign = att->remote_sign;
@@ -903,10 +903,8 @@ static bool handle_signed(struct bt_att *att, uint8_t opcode, uint8_t *pdu,
 	if (!sign->counter(&sign_cnt, sign->user_data))
 		goto fail;
 
-	/* Generate signature and verify it */
-	if (!bt_crypto_sign_att(att->crypto, sign->key, pdu,
-				pdu_len - BT_ATT_SIGNATURE_LEN, sign_cnt,
-				signature))
+	/* Verify received signature */
+	if (!bt_crypto_verify_att_sign(att->crypto, sign->key, pdu, pdu_len))
 		goto fail;
 
 	return true;
@@ -918,15 +916,16 @@ fail:
 	return false;
 }
 
-static void handle_notify(struct bt_att_chan *chan, uint8_t opcode,
-						uint8_t *pdu, ssize_t pdu_len)
+static void handle_notify(struct bt_att_chan *chan, uint8_t *pdu,
+							ssize_t pdu_len)
 {
 	struct bt_att *att = chan->att;
 	const struct queue_entry *entry;
 	bool found;
+	uint8_t opcode = pdu[0];
 
-	if ((opcode & ATT_OP_SIGNED_MASK) && !att->crypto) {
-		if (!handle_signed(att, opcode, pdu, pdu_len))
+	if ((opcode & ATT_OP_SIGNED_MASK) && att->crypto) {
+		if (!handle_signed(att, pdu, pdu_len))
 			return;
 		pdu_len -= BT_ATT_SIGNATURE_LEN;
 	}
@@ -963,7 +962,7 @@ static void handle_notify(struct bt_att_chan *chan, uint8_t opcode,
 		found = true;
 
 		if (notify->callback)
-			notify->callback(chan, opcode, pdu, pdu_len,
+			notify->callback(chan, opcode, pdu + 1, pdu_len - 1,
 							notify->user_data);
 
 		/* callback could remove all entries from notify list */
@@ -1054,7 +1053,7 @@ static bool can_read_data(struct io *io, void *user_data)
 		util_debug(att->debug_callback, att->debug_data,
 					"(chan %p) ATT PDU received: 0x%02x",
 					chan, opcode);
-		handle_notify(chan, opcode, pdu + 1, bytes_read - 1);
+		handle_notify(chan, pdu, bytes_read);
 		break;
 	}
 
-- 
2.26.0.292.g33ef6b2f38-goog


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [Bluez PATCH v4 4/4] unit/test-gatt: Fix unknown request with signed bit
  2020-04-07  8:56 [Bluez PATCH v4 0/4] Check the signature of att packets Archie Pusaka
                   ` (2 preceding siblings ...)
  2020-04-07  8:56 ` [Bluez PATCH v4 3/4] shared/att: Check the signature of att packets Archie Pusaka
@ 2020-04-07  8:56 ` Archie Pusaka
  2020-04-07 17:55   ` Luiz Augusto von Dentz
  2020-04-07 20:07 ` [Bluez PATCH v4 0/4] Check the signature of att packets Luiz Augusto von Dentz
  4 siblings, 1 reply; 8+ messages in thread
From: Archie Pusaka @ 2020-04-07  8:56 UTC (permalink / raw)
  To: linux-bluetooth, Luiz Augusto von Dentz; +Cc: Archie Pusaka

From: Archie Pusaka <apusaka@chromium.org>

The BT spec doesn't make it explicit of what should happen when
receiving a bad signed att request packet.

According to BT core spec Vol 3, Part C, Sec 10.4.2:
A device receiving signed data shall authenticate it by performing
the Signing Algorithm. If the MAC computed by the Signing Algorithm
does not match the received MAC, the verification fails and the Host
shall ignore the received Data PDU.

According to BT core spec Vol 3, Part F, Sec 3.3
If a server receives a request that it does not support, then the
server shall respond with the ATT_ERROR_RSP PDU with the error code
Request Not Supported.

This patch does this two things:
(1) Removing the signed bit to the existing tests so they are not
    in a conflicting state within the bluetooth spec, while still
    keeping the original intent of the test.
(2) Add another test that purposely fall within this grey area
    with some comments.
---

Changes in v4:
- Fixing test-gatt.c

Changes in v3: None
Changes in v2: None

 unit/test-gatt.c | 32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

diff --git a/unit/test-gatt.c b/unit/test-gatt.c
index 36dd2847c..139a6fc72 100644
--- a/unit/test-gatt.c
+++ b/unit/test-gatt.c
@@ -4473,16 +4473,38 @@ int main(int argc, char *argv[])
 			raw_pdu(0x18, 0x01),
 			raw_pdu(0x01, 0x18, 0x25, 0x00, 0x06));
 
-	define_test_server("/robustness/unkown-request",
+	define_test_server("/robustness/unknown-request",
 			test_server, service_db_1, NULL,
 			raw_pdu(0x03, 0x00, 0x02),
-			raw_pdu(0xbf, 0x00),
-			raw_pdu(0x01, 0xbf, 0x00, 0x00, 0x06));
+			raw_pdu(0x3f, 0x00),
+			raw_pdu(0x01, 0x3f, 0x00, 0x00, 0x06));
+
+	define_test_server("/robustness/unknown-command",
+			test_server, service_db_1, NULL,
+			raw_pdu(0x03, 0x00, 0x02),
+			raw_pdu(0x7f, 0x00),
+			raw_pdu());
 
-	define_test_server("/robustness/unkown-command",
+	/*
+	 * According to BT core spec Vol 3, Part C, Sec 10.4.2:
+	 * A device receiving signed data shall authenticate it by performing
+	 * the Signing Algorithm. If the MAC computed by the Signing Algorithm
+	 * does not match the received MAC, the verification fails and the Host
+	 * shall ignore the received Data PDU.
+	 *
+	 * However, according to BT core spec Vol 3, Part F, Sec 3.3
+	 * If a server receives a request that it does not support, then the
+	 * server shall respond with the ATT_ERROR_RSP PDU with the error code
+	 * Request Not Supported.
+	 *
+	 * Since there is no explicit instruction on what should be done in
+	 * case the server receives a bad signed unsupported request, here
+	 * we just ignore the received PDU.
+	 */
+	define_test_server("/robustness/signed-unknown-request",
 			test_server, service_db_1, NULL,
 			raw_pdu(0x03, 0x00, 0x02),
-			raw_pdu(0xff, 0x00),
+			raw_pdu(0xbf, 0x00),
 			raw_pdu());
 
 	return tester_run();
-- 
2.26.0.292.g33ef6b2f38-goog


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [Bluez PATCH v4 4/4] unit/test-gatt: Fix unknown request with signed bit
  2020-04-07  8:56 ` [Bluez PATCH v4 4/4] unit/test-gatt: Fix unknown request with signed bit Archie Pusaka
@ 2020-04-07 17:55   ` Luiz Augusto von Dentz
  0 siblings, 0 replies; 8+ messages in thread
From: Luiz Augusto von Dentz @ 2020-04-07 17:55 UTC (permalink / raw)
  To: Archie Pusaka; +Cc: linux-bluetooth, Archie Pusaka

Hi Archie,

On Tue, Apr 7, 2020 at 1:56 AM Archie Pusaka <apusaka@google.com> wrote:
>
> From: Archie Pusaka <apusaka@chromium.org>
>
> The BT spec doesn't make it explicit of what should happen when
> receiving a bad signed att request packet.
>
> According to BT core spec Vol 3, Part C, Sec 10.4.2:
> A device receiving signed data shall authenticate it by performing
> the Signing Algorithm. If the MAC computed by the Signing Algorithm
> does not match the received MAC, the verification fails and the Host
> shall ignore the received Data PDU.

In my interpretation 'ignore the received Data PDU' may not supersed
the followin statement:

> According to BT core spec Vol 3, Part F, Sec 3.3
> If a server receives a request that it does not support, then the
> server shall respond with the ATT_ERROR_RSP PDU with the error code
> Request Not Supported.

So what I think we should be doing is to ignore the received data,
meaning it should not even be processed as a signed data since we
don't understand the command, _and_ respond with ATT_ERROR_RSP,
otherwise we risk having the 30 seconds timeout and disconnecting
since the request is no responded.

> This patch does this two things:
> (1) Removing the signed bit to the existing tests so they are not
>     in a conflicting state within the bluetooth spec, while still
>     keeping the original intent of the test.
> (2) Add another test that purposely fall within this grey area
>     with some comments.
> ---
>
> Changes in v4:
> - Fixing test-gatt.c
>
> Changes in v3: None
> Changes in v2: None
>
>  unit/test-gatt.c | 32 +++++++++++++++++++++++++++-----
>  1 file changed, 27 insertions(+), 5 deletions(-)
>
> diff --git a/unit/test-gatt.c b/unit/test-gatt.c
> index 36dd2847c..139a6fc72 100644
> --- a/unit/test-gatt.c
> +++ b/unit/test-gatt.c
> @@ -4473,16 +4473,38 @@ int main(int argc, char *argv[])
>                         raw_pdu(0x18, 0x01),
>                         raw_pdu(0x01, 0x18, 0x25, 0x00, 0x06));
>
> -       define_test_server("/robustness/unkown-request",
> +       define_test_server("/robustness/unknown-request",
>                         test_server, service_db_1, NULL,
>                         raw_pdu(0x03, 0x00, 0x02),
> -                       raw_pdu(0xbf, 0x00),
> -                       raw_pdu(0x01, 0xbf, 0x00, 0x00, 0x06));
> +                       raw_pdu(0x3f, 0x00),
> +                       raw_pdu(0x01, 0x3f, 0x00, 0x00, 0x06));

Afaik we used 0xbf because that was what PTS was using.

> +       define_test_server("/robustness/unknown-command",
> +                       test_server, service_db_1, NULL,
> +                       raw_pdu(0x03, 0x00, 0x02),
> +                       raw_pdu(0x7f, 0x00),
> +                       raw_pdu());
>
> -       define_test_server("/robustness/unkown-command",
> +       /*
> +        * According to BT core spec Vol 3, Part C, Sec 10.4.2:
> +        * A device receiving signed data shall authenticate it by performing
> +        * the Signing Algorithm. If the MAC computed by the Signing Algorithm
> +        * does not match the received MAC, the verification fails and the Host
> +        * shall ignore the received Data PDU.
> +        *
> +        * However, according to BT core spec Vol 3, Part F, Sec 3.3
> +        * If a server receives a request that it does not support, then the
> +        * server shall respond with the ATT_ERROR_RSP PDU with the error code
> +        * Request Not Supported.
> +        *
> +        * Since there is no explicit instruction on what should be done in
> +        * case the server receives a bad signed unsupported request, here
> +        * we just ignore the received PDU.
> +        */
> +       define_test_server("/robustness/signed-unknown-request",
>                         test_server, service_db_1, NULL,
>                         raw_pdu(0x03, 0x00, 0x02),
> -                       raw_pdu(0xff, 0x00),
> +                       raw_pdu(0xbf, 0x00),
>                         raw_pdu());
>
>         return tester_run();
> --
> 2.26.0.292.g33ef6b2f38-goog
>


-- 
Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Bluez PATCH v4 0/4] Check the signature of att packets
  2020-04-07  8:56 [Bluez PATCH v4 0/4] Check the signature of att packets Archie Pusaka
                   ` (3 preceding siblings ...)
  2020-04-07  8:56 ` [Bluez PATCH v4 4/4] unit/test-gatt: Fix unknown request with signed bit Archie Pusaka
@ 2020-04-07 20:07 ` Luiz Augusto von Dentz
  2020-04-08  3:41   ` Archie Pusaka
  4 siblings, 1 reply; 8+ messages in thread
From: Luiz Augusto von Dentz @ 2020-04-07 20:07 UTC (permalink / raw)
  To: Archie Pusaka; +Cc: linux-bluetooth, Archie Pusaka

Hi Archie,

On Tue, Apr 7, 2020 at 1:56 AM Archie Pusaka <apusaka@google.com> wrote:
>
> From: Archie Pusaka <apusaka@chromium.org>
>
> According to bluetooth spec Ver 5.1, Vol 3, Part C (GAP), 10.4.2
> A device receiving signed data shall authenticate it by performing
> the Signing Algorithm. The signed data shall be authenticated by
> performing the Signing Algorithm where m is the Data PDU to be
> authenticated, k is the stored CSRK and the SignCounter is the
> received counter value. If the MAC computed by the Signing
> Algorithm does not match the received MAC, the verification fails
> and the Host shall ignore the received Data PDU.
>
> Currently bluez ignore the signature of received signed att
> packets, as the function bt_crypto_sign_att() only generates the
> signature, and not actually make any check about the genuineness
> of the signature itself.
>
> This patch also fix a wrong boolean condition which prevents
> handle_signed() to be called.
>
> Tested to pass these BT certification test
> SM/MAS/SIGN/BV-03-C
> SM/MAS/SIGN/BI-01-C
>
> Changes in v4:
> - Fix wrong variable assignment
> - Fixing test-gatt.c
>
> Changes in v3:
> - Add check for the case where pdu_len < ATT_SIGN_LEN
> - Add unit test
> - Separate into three patches
>
> Changes in v2:
> - Move the signature verification part to crypto.c
> - Attempt not to copy the whole pdu while verifying the signature
>   by not separating the opcode from the rest of pdu too early, so
>   we don't have to rejoin them later.
>
> Archie Pusaka (4):
>   shared/crypto: Add bt_crypto_verify_att_sign
>   unit/test-crypto: test for bt_crypto_verify_att_sign
>   shared/att: Check the signature of att packets
>   unit/test-gatt: Fix unknown request with signed bit
>
>  src/shared/att.c    | 25 +++++++++----------
>  src/shared/crypto.c | 28 +++++++++++++++++++--
>  src/shared/crypto.h |  2 ++
>  unit/test-crypto.c  | 59 +++++++++++++++++++++++++++++++++++++++++++++
>  unit/test-gatt.c    | 32 ++++++++++++++++++++----
>  5 files changed, 126 insertions(+), 20 deletions(-)
>
> --
> 2.26.0.292.g33ef6b2f38-goog

Ive applied 1-3 and reworked a little bit the third patch so we
actually attempt to find a handler before we attempt to check the
signature.

-- 
Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Bluez PATCH v4 0/4] Check the signature of att packets
  2020-04-07 20:07 ` [Bluez PATCH v4 0/4] Check the signature of att packets Luiz Augusto von Dentz
@ 2020-04-08  3:41   ` Archie Pusaka
  0 siblings, 0 replies; 8+ messages in thread
From: Archie Pusaka @ 2020-04-08  3:41 UTC (permalink / raw)
  To: Luiz Augusto von Dentz; +Cc: linux-bluetooth, Archie Pusaka

Hi Luiz,

On Wed, 8 Apr 2020 at 04:07, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Archie,
>
> On Tue, Apr 7, 2020 at 1:56 AM Archie Pusaka <apusaka@google.com> wrote:
> >
> > From: Archie Pusaka <apusaka@chromium.org>
> >
> > According to bluetooth spec Ver 5.1, Vol 3, Part C (GAP), 10.4.2
> > A device receiving signed data shall authenticate it by performing
> > the Signing Algorithm. The signed data shall be authenticated by
> > performing the Signing Algorithm where m is the Data PDU to be
> > authenticated, k is the stored CSRK and the SignCounter is the
> > received counter value. If the MAC computed by the Signing
> > Algorithm does not match the received MAC, the verification fails
> > and the Host shall ignore the received Data PDU.
> >
> > Currently bluez ignore the signature of received signed att
> > packets, as the function bt_crypto_sign_att() only generates the
> > signature, and not actually make any check about the genuineness
> > of the signature itself.
> >
> > This patch also fix a wrong boolean condition which prevents
> > handle_signed() to be called.
> >
> > Tested to pass these BT certification test
> > SM/MAS/SIGN/BV-03-C
> > SM/MAS/SIGN/BI-01-C
> >
> > Changes in v4:
> > - Fix wrong variable assignment
> > - Fixing test-gatt.c
> >
> > Changes in v3:
> > - Add check for the case where pdu_len < ATT_SIGN_LEN
> > - Add unit test
> > - Separate into three patches
> >
> > Changes in v2:
> > - Move the signature verification part to crypto.c
> > - Attempt not to copy the whole pdu while verifying the signature
> >   by not separating the opcode from the rest of pdu too early, so
> >   we don't have to rejoin them later.
> >
> > Archie Pusaka (4):
> >   shared/crypto: Add bt_crypto_verify_att_sign
> >   unit/test-crypto: test for bt_crypto_verify_att_sign
> >   shared/att: Check the signature of att packets
> >   unit/test-gatt: Fix unknown request with signed bit
> >
> >  src/shared/att.c    | 25 +++++++++----------
> >  src/shared/crypto.c | 28 +++++++++++++++++++--
> >  src/shared/crypto.h |  2 ++
> >  unit/test-crypto.c  | 59 +++++++++++++++++++++++++++++++++++++++++++++
> >  unit/test-gatt.c    | 32 ++++++++++++++++++++----
> >  5 files changed, 126 insertions(+), 20 deletions(-)
> >
> > --
> > 2.26.0.292.g33ef6b2f38-goog
>
> Ive applied 1-3 and reworked a little bit the third patch so we
> actually attempt to find a handler before we attempt to check the
> signature.

Ack, thanks a bunch!

Regards,
Archie

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-04-08  3:42 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-07  8:56 [Bluez PATCH v4 0/4] Check the signature of att packets Archie Pusaka
2020-04-07  8:56 ` [Bluez PATCH v4 1/4] shared/crypto: Add bt_crypto_verify_att_sign Archie Pusaka
2020-04-07  8:56 ` [Bluez PATCH v4 2/4] unit/test-crypto: test for bt_crypto_verify_att_sign Archie Pusaka
2020-04-07  8:56 ` [Bluez PATCH v4 3/4] shared/att: Check the signature of att packets Archie Pusaka
2020-04-07  8:56 ` [Bluez PATCH v4 4/4] unit/test-gatt: Fix unknown request with signed bit Archie Pusaka
2020-04-07 17:55   ` Luiz Augusto von Dentz
2020-04-07 20:07 ` [Bluez PATCH v4 0/4] Check the signature of att packets Luiz Augusto von Dentz
2020-04-08  3:41   ` Archie Pusaka

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.