Received-SPF: pass (google.com: best guess record for domain of linux-bluetooth-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20200327201817.Bluez.v1.1.If919a39697a6506be273f879d752bd506e63b45b@changeid>
 <CABBYNZ+nSx++GMZNEuUKDner+Aa92sjC9e2aSZ8vQE9iMTdoNA@mail.gmail.com>
In-Reply-To: <CABBYNZ+nSx++GMZNEuUKDner+Aa92sjC9e2aSZ8vQE9iMTdoNA@mail.gmail.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Fri, 27 Mar 2020 11:17:32 -0700
Message-ID: <CABBYNZJFnR8Co8EghH+77eMoa90ePGEd6qY4BWgsEnyj0RgEWw@mail.gmail.com>
Subject: Re: [Bluez PATCH v1] shared/att: Check the signature of att packets
To:     Archie Pusaka <apusaka@google.com>
Cc:     linux-bluetooth <linux-bluetooth@vger.kernel.org>,
        Archie Pusaka <apusaka@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi Archie,

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