Hi Archie,
On Fri, Mar 27, 2020 at 5:19 AM Archie Pusaka <apusaka@xxxxxxxxxx> wrote:
>
> From: Archie Pusaka <apusaka@xxxxxxxxxxxx>
>
> 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
