Hi Archie,
On Fri, Mar 27, 2020 at 11:15 AM Luiz Augusto von Dentz
<luiz.dentz@xxxxxxxxx> wrote:
>
> Hi Archie,
>
> On Fri, Mar 27, 2020 at 5:19 AM Archie Pusaka <apusaka@xxxxxxxxxx> wrote:
> >
> > From: Archie Pusaka <apusaka@xxxxxxxxxxxx>
> >
> > 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
