[Bluez PATCH v3 0/3] Check the signature of att packets

[Bluez PATCH v3 0/3] Check the signature of att packets

[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 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 (3):
  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

 src/shared/att.c    | 25 +++++++++----------
 src/shared/crypto.c | 28 +++++++++++++++++++--
 src/shared/crypto.h |  2 ++
 unit/test-crypto.c  | 59 +++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 99 insertions(+), 15 deletions(-)

-- 
2.26.0.292.g33ef6b2f38-goog

[Bluez PATCH v3 1/3] shared/crypto: Add bt_crypto_verify_att_sign

[Archie Pusaka]

From: Archie Pusaka <apusaka@chromium.org>

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

---

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

[Bluez PATCH v3 2/3] unit/test-crypto: test for bt_crypto_verify_att_sign

[Archie Pusaka]

From: Archie Pusaka <apusaka@chromium.org>

Adding tests for verifying att signature

---

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..3bc944be8 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_bad_sign,
+	.msg_len = sizeof(msg_to_verify_bad_sign),
+	.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

[Bluez PATCH v3 3/3] shared/att: Check the signature of att packets

[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 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

Re: [Bluez PATCH v3 2/3] unit/test-crypto: test for bt_crypto_verify_att_sign

[Luiz Augusto von Dentz]

Hi Archie,

On Mon, Apr 6, 2020 at 4:49 AM Archie Pusaka <apusaka@google.com> wrote:
>
> From: Archie Pusaka <apusaka@chromium.org>
>
> Adding tests for verifying att signature
>
> ---
>
> 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..3bc944be8 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 = {

These should be msg_to_verify_too_short.

> +       .msg = msg_to_verify_bad_sign,
> +       .msg_len = sizeof(msg_to_verify_bad_sign),
> +       .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
>


-- 
Luiz Augusto von Dentz

Re: [Bluez PATCH v3 3/3] shared/att: Check the signature of att packets

[Luiz Augusto von Dentz]

Hi Archie,

On Mon, Apr 6, 2020 at 4:49 AM Archie Pusaka <apusaka@google.com> wrote:
>
> 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 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

This actually make unit/test-gatt get stuck for some reason, so you
will need to investigate and make it work with existing tests or fix
the tests if there are actually not conforming to the spec.

-- 
Luiz Augusto von Dentz

Re: [Bluez PATCH v3 3/3] shared/att: Check the signature of att packets

[Archie Pusaka]

Hi Luiz,

The reason test-gatt.c is stuck is because there exists a test which
sends pdu with opcode = 0xBF, an invalid opcode, to test the
robustness of the system of an invalid request.
By applying the patch, instead of replying to the pdu, we silently
ignore it, which makes the test stuck.
The opcode has the "signed" bit (0x80) on and "command" bit (0x40)
off, which makes it in a difficult situation where it shouldn't be
possible and as far as I know is not explicitly discussed in the spec.

I shall fall back to this part from the BT spec, vol 3. Part C section 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."

Therefore, I shall modify the failing test, and add another case which
is just "invalid request" but without the signed bit.

Thanks,
Archie

On Tue, 7 Apr 2020 at 05:16, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Archie,
>
> On Mon, Apr 6, 2020 at 4:49 AM Archie Pusaka <apusaka@google.com> wrote:
> >
> > 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 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
>
> This actually make unit/test-gatt get stuck for some reason, so you
> will need to investigate and make it work with existing tests or fix
> the tests if there are actually not conforming to the spec.
>
> --
> Luiz Augusto von Dentz

Re: [Bluez PATCH v3 2/3] unit/test-crypto: test for bt_crypto_verify_att_sign

[Archie Pusaka]

Hi Luiz,

Thanks, you're correct.

On Tue, 7 Apr 2020 at 05:14, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Archie,
>
> On Mon, Apr 6, 2020 at 4:49 AM Archie Pusaka <apusaka@google.com> wrote:
> >
> > From: Archie Pusaka <apusaka@chromium.org>
> >
> > Adding tests for verifying att signature
> >
> > ---
> >
> > 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..3bc944be8 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 = {
>
> These should be msg_to_verify_too_short.
>
> > +       .msg = msg_to_verify_bad_sign,
> > +       .msg_len = sizeof(msg_to_verify_bad_sign),
> > +       .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
> >
>
>
> --
> Luiz Augusto von Dentz