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: <20200407085610.231013-1-apusaka@google.com>
In-Reply-To: <20200407085610.231013-1-apusaka@google.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Tue, 7 Apr 2020 13:07:28 -0700
Message-ID: <CABBYNZ+3SUbZbO+BJ7BX3uj5dds=KzWpe316SghncrOE8ikLBw@mail.gmail.com>
Subject: Re: [Bluez PATCH v4 0/4] 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 Tue, Apr 7, 2020 at 1:56 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
>
> Changes in v4:
> - Fix wrong variable assignment
> - Fixing test-gatt.c
>
> Changes in v3:
> - Add check for the case where pdu_len < ATT_SIGN_LEN
> - Add unit test
> - 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.
>
> Archie Pusaka (4):
>   shared/crypto: Add bt_crypto_verify_att_sign
>   unit/test-crypto: test for bt_crypto_verify_att_sign
>   shared/att: Check the signature of att packets
>   unit/test-gatt: Fix unknown request with signed bit
>
>  src/shared/att.c    | 25 +++++++++----------
>  src/shared/crypto.c | 28 +++++++++++++++++++--
>  src/shared/crypto.h |  2 ++
>  unit/test-crypto.c  | 59 +++++++++++++++++++++++++++++++++++++++++++++
>  unit/test-gatt.c    | 32 ++++++++++++++++++++----
>  5 files changed, 126 insertions(+), 20 deletions(-)
>
> --
> 2.26.0.292.g33ef6b2f38-goog

Ive applied 1-3 and reworked a little bit the third patch so we
actually attempt to find a handler before we attempt to check the
signature.

-- 
Luiz Augusto von Dentz