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>
In-Reply-To: <20200406194749.Bluez.v3.3.I28a54f18ca82b58e44689a0c76663e735fefb6f1@changeid>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Mon, 6 Apr 2020 14:16:26 -0700
Message-ID: <CABBYNZJh9O8xcyqXZYKtq9uV=UvSwf8GZBocKVzeh5Wtu2cSDg@mail.gmail.com>
Subject: Re: [Bluez PATCH v3 3/3] 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 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