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: <20200406114845.255532-1-apusaka@google.com> <20200406194749.Bluez.v3.3.I28a54f18ca82b58e44689a0c76663e735fefb6f1@changeid>
 <CABBYNZJh9O8xcyqXZYKtq9uV=UvSwf8GZBocKVzeh5Wtu2cSDg@mail.gmail.com>
In-Reply-To: <CABBYNZJh9O8xcyqXZYKtq9uV=UvSwf8GZBocKVzeh5Wtu2cSDg@mail.gmail.com>
From:   Archie Pusaka <apusaka@google.com>
Date:   Tue, 7 Apr 2020 15:23:20 +0800
Message-ID: <CAJQfnxHX2AnbKLQto_m89AsoKutp=Z6oBPL7UfOSFEPBrstu2Q@mail.gmail.com>
Subject: Re: [Bluez PATCH v3 3/3] shared/att: Check the signature of att packets
To:     Luiz Augusto von Dentz <luiz.dentz@gmail.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 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