From: Archie Pusaka <[email protected]>
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
---
src/shared/att.c | 26 +++++++++++++++++++++-----
1 file changed, 21 insertions(+), 5 deletions(-)
diff --git a/src/shared/att.c b/src/shared/att.c
index 948a5548b..0faac4d1d 100644
--- a/src/shared/att.c
+++ b/src/shared/att.c
@@ -886,6 +886,8 @@ static bool handle_signed(struct bt_att *att, uint8_t opcode, uint8_t *pdu,
{
uint8_t *signature;
uint32_t sign_cnt;
+ uint8_t *copy_pdu = NULL;
+ uint8_t *generated_signature;
struct sign_info *sign;
/* Check if there is enough data for a signature */
@@ -903,15 +905,29 @@ 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))
+ /* Generate signature */
+ copy_pdu = malloc(pdu_len + 1);
+ if (!copy_pdu)
goto fail;
+ copy_pdu[0] = opcode;
+ memcpy(copy_pdu + 1, pdu, pdu_len - BT_ATT_SIGNATURE_LEN);
+ generated_signature = copy_pdu + pdu_len - BT_ATT_SIGNATURE_LEN + 1;
+
+ if (!bt_crypto_sign_att(att->crypto, sign->key, copy_pdu,
+ pdu_len - BT_ATT_SIGNATURE_LEN + 1, sign_cnt,
+ generated_signature))
+ goto fail;
+
+ /* Verify received signature */
+ if (memcmp(generated_signature, signature, BT_ATT_SIGNATURE_LEN))
+ goto fail;
+
+ free(copy_pdu);
return true;
fail:
+ free(copy_pdu);
util_debug(att->debug_callback, att->debug_data,
"ATT failed to verify signature: 0x%02x", opcode);
@@ -925,7 +941,7 @@ static void handle_notify(struct bt_att_chan *chan, uint8_t opcode,
const struct queue_entry *entry;
bool found;
- if ((opcode & ATT_OP_SIGNED_MASK) && !att->crypto) {
+ if ((opcode & ATT_OP_SIGNED_MASK) && att->crypto) {
if (!handle_signed(att, opcode, pdu, pdu_len))
return;
pdu_len -= BT_ATT_SIGNATURE_LEN;
--
2.25.1.696.g5e7596f4ac-goog
Hi Archie,
On Fri, Mar 27, 2020 at 5:19 AM Archie Pusaka <[email protected]> wrote:
>
> From: Archie Pusaka <[email protected]>
>
> 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
Nice catch, looks like we have never passed this test properly before.
> ---
>
> src/shared/att.c | 26 +++++++++++++++++++++-----
> 1 file changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/src/shared/att.c b/src/shared/att.c
> index 948a5548b..0faac4d1d 100644
> --- a/src/shared/att.c
> +++ b/src/shared/att.c
> @@ -886,6 +886,8 @@ static bool handle_signed(struct bt_att *att, uint8_t opcode, uint8_t *pdu,
> {
> uint8_t *signature;
> uint32_t sign_cnt;
> + uint8_t *copy_pdu = NULL;
> + uint8_t *generated_signature;
> struct sign_info *sign;
>
> /* Check if there is enough data for a signature */
> @@ -903,15 +905,29 @@ 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))
> + /* Generate signature */
> + copy_pdu = malloc(pdu_len + 1);
> + if (!copy_pdu)
> goto fail;
>
> + copy_pdu[0] = opcode;
> + memcpy(copy_pdu + 1, pdu, pdu_len - BT_ATT_SIGNATURE_LEN);
> + generated_signature = copy_pdu + pdu_len - BT_ATT_SIGNATURE_LEN + 1;
> +
> + if (!bt_crypto_sign_att(att->crypto, sign->key, copy_pdu,
> + pdu_len - BT_ATT_SIGNATURE_LEN + 1, sign_cnt,
> + generated_signature))
> + goto fail;
> +
> + /* Verify received signature */
> + if (memcmp(generated_signature, signature, BT_ATT_SIGNATURE_LEN))
> + goto fail;
>
> + free(copy_pdu);
While this seems to do a proper check perhaps it is better to have a
helper function in crypto to do that for us, so we can unit test it as
well, also I would consider the possibility of doing the comparison in
place since you don't seem to modify the PDU contents at any point we
just want to compare the signatures match.
> return true;
>
> fail:
> + free(copy_pdu);
> util_debug(att->debug_callback, att->debug_data,
> "ATT failed to verify signature: 0x%02x", opcode);
>
> @@ -925,7 +941,7 @@ static void handle_notify(struct bt_att_chan *chan, uint8_t opcode,
> const struct queue_entry *entry;
> bool found;
>
> - if ((opcode & ATT_OP_SIGNED_MASK) && !att->crypto) {
> + if ((opcode & ATT_OP_SIGNED_MASK) && att->crypto) {
> if (!handle_signed(att, opcode, pdu, pdu_len))
> return;
> pdu_len -= BT_ATT_SIGNATURE_LEN;
> --
> 2.25.1.696.g5e7596f4ac-goog
>
--
Luiz Augusto von Dentz
Hi Archie,
On Fri, Mar 27, 2020 at 11:15 AM Luiz Augusto von Dentz
<[email protected]> wrote:
>
> Hi Archie,
>
> On Fri, Mar 27, 2020 at 5:19 AM Archie Pusaka <[email protected]> wrote:
> >
> > From: Archie Pusaka <[email protected]>
> >
> > 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
>
> Nice catch, looks like we have never passed this test properly before.
>
> > ---
> >
> > src/shared/att.c | 26 +++++++++++++++++++++-----
> > 1 file changed, 21 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/shared/att.c b/src/shared/att.c
> > index 948a5548b..0faac4d1d 100644
> > --- a/src/shared/att.c
> > +++ b/src/shared/att.c
> > @@ -886,6 +886,8 @@ static bool handle_signed(struct bt_att *att, uint8_t opcode, uint8_t *pdu,
> > {
> > uint8_t *signature;
> > uint32_t sign_cnt;
> > + uint8_t *copy_pdu = NULL;
> > + uint8_t *generated_signature;
> > struct sign_info *sign;
> >
> > /* Check if there is enough data for a signature */
> > @@ -903,15 +905,29 @@ 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))
> > + /* Generate signature */
> > + copy_pdu = malloc(pdu_len + 1);
> > + if (!copy_pdu)
> > goto fail;
> >
> > + copy_pdu[0] = opcode;
> > + memcpy(copy_pdu + 1, pdu, pdu_len - BT_ATT_SIGNATURE_LEN);
> > + generated_signature = copy_pdu + pdu_len - BT_ATT_SIGNATURE_LEN + 1;
> > +
> > + if (!bt_crypto_sign_att(att->crypto, sign->key, copy_pdu,
> > + pdu_len - BT_ATT_SIGNATURE_LEN + 1, sign_cnt,
> > + generated_signature))
> > + goto fail;
> > +
> > + /* Verify received signature */
> > + if (memcmp(generated_signature, signature, BT_ATT_SIGNATURE_LEN))
> > + goto fail;
> >
> > + free(copy_pdu);
>
> While this seems to do a proper check perhaps it is better to have a
> helper function in crypto to do that for us, so we can unit test it as
> well, also I would consider the possibility of doing the comparison in
> place since you don't seem to modify the PDU contents at any point we
> just want to compare the signatures match.
I realize this may not be clear, what I meant is that we don't need a
extra copy of the PDU if its contents is no altered at any point.
> > return true;
> >
> > fail:
> > + free(copy_pdu);
> > util_debug(att->debug_callback, att->debug_data,
> > "ATT failed to verify signature: 0x%02x", opcode);
> >
> > @@ -925,7 +941,7 @@ static void handle_notify(struct bt_att_chan *chan, uint8_t opcode,
> > const struct queue_entry *entry;
> > bool found;
> >
> > - if ((opcode & ATT_OP_SIGNED_MASK) && !att->crypto) {
> > + if ((opcode & ATT_OP_SIGNED_MASK) && att->crypto) {
> > if (!handle_signed(att, opcode, pdu, pdu_len))
> > return;
> > pdu_len -= BT_ATT_SIGNATURE_LEN;
> > --
> > 2.25.1.696.g5e7596f4ac-goog
> >
>
>
> --
> Luiz Augusto von Dentz
--
Luiz Augusto von Dentz
Hi Luiz,
I will move the whole check to crypto.c.
The problem with generating the signature without copying the pdu is,
the function to generate the signature requires the opcode to be
prepended to the pdu. Therefore, I make a copy of the pdu just for the
sake to prepend the opcode to it.
However, the opcode was actually prepended to the pdu in the first
place, but we separate it when invoking handle_notify(). Because this
function is local to only this file, I will change it so the opcode is
not separated when entering handle_notify(), but we will do so for
every other function that is called within handle_notify().
Thanks,
Archie
On Sat, 28 Mar 2020 at 02:17, Luiz Augusto von Dentz
<[email protected]> wrote:
>
> Hi Archie,
>
> On Fri, Mar 27, 2020 at 11:15 AM Luiz Augusto von Dentz
> <[email protected]> wrote:
> >
> > Hi Archie,
> >
> > On Fri, Mar 27, 2020 at 5:19 AM Archie Pusaka <[email protected]> wrote:
> > >
> > > From: Archie Pusaka <[email protected]>
> > >
> > > 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
> >
> > Nice catch, looks like we have never passed this test properly before.
> >
> > > ---
> > >
> > > src/shared/att.c | 26 +++++++++++++++++++++-----
> > > 1 file changed, 21 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/src/shared/att.c b/src/shared/att.c
> > > index 948a5548b..0faac4d1d 100644
> > > --- a/src/shared/att.c
> > > +++ b/src/shared/att.c
> > > @@ -886,6 +886,8 @@ static bool handle_signed(struct bt_att *att, uint8_t opcode, uint8_t *pdu,
> > > {
> > > uint8_t *signature;
> > > uint32_t sign_cnt;
> > > + uint8_t *copy_pdu = NULL;
> > > + uint8_t *generated_signature;
> > > struct sign_info *sign;
> > >
> > > /* Check if there is enough data for a signature */
> > > @@ -903,15 +905,29 @@ 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))
> > > + /* Generate signature */
> > > + copy_pdu = malloc(pdu_len + 1);
> > > + if (!copy_pdu)
> > > goto fail;
> > >
> > > + copy_pdu[0] = opcode;
> > > + memcpy(copy_pdu + 1, pdu, pdu_len - BT_ATT_SIGNATURE_LEN);
> > > + generated_signature = copy_pdu + pdu_len - BT_ATT_SIGNATURE_LEN + 1;
> > > +
> > > + if (!bt_crypto_sign_att(att->crypto, sign->key, copy_pdu,
> > > + pdu_len - BT_ATT_SIGNATURE_LEN + 1, sign_cnt,
> > > + generated_signature))
> > > + goto fail;
> > > +
> > > + /* Verify received signature */
> > > + if (memcmp(generated_signature, signature, BT_ATT_SIGNATURE_LEN))
> > > + goto fail;
> > >
> > > + free(copy_pdu);
> >
> > While this seems to do a proper check perhaps it is better to have a
> > helper function in crypto to do that for us, so we can unit test it as
> > well, also I would consider the possibility of doing the comparison in
> > place since you don't seem to modify the PDU contents at any point we
> > just want to compare the signatures match.
>
> I realize this may not be clear, what I meant is that we don't need a
> extra copy of the PDU if its contents is no altered at any point.
>
> > > return true;
> > >
> > > fail:
> > > + free(copy_pdu);
> > > util_debug(att->debug_callback, att->debug_data,
> > > "ATT failed to verify signature: 0x%02x", opcode);
> > >
> > > @@ -925,7 +941,7 @@ static void handle_notify(struct bt_att_chan *chan, uint8_t opcode,
> > > const struct queue_entry *entry;
> > > bool found;
> > >
> > > - if ((opcode & ATT_OP_SIGNED_MASK) && !att->crypto) {
> > > + if ((opcode & ATT_OP_SIGNED_MASK) && att->crypto) {
> > > if (!handle_signed(att, opcode, pdu, pdu_len))
> > > return;
> > > pdu_len -= BT_ATT_SIGNATURE_LEN;
> > > --
> > > 2.25.1.696.g5e7596f4ac-goog
> > >
> >
> >
> > --
> > Luiz Augusto von Dentz
>
>
>
> --
> Luiz Augusto von Dentz