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>
In-Reply-To: <20200327201817.Bluez.v1.1.If919a39697a6506be273f879d752bd506e63b45b@changeid>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Fri, 27 Mar 2020 11:15:41 -0700
Message-ID: <CABBYNZ+nSx++GMZNEuUKDner+Aa92sjC9e2aSZ8vQE9iMTdoNA@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 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.

>         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