Hi Archie,
On Mon, Mar 30, 2020 at 3:36 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
> ---
>
> 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 ++++++++++++-------------
> src/shared/crypto.c | 22 ++++++++++++++++++++--
> src/shared/crypto.h | 2 ++
> 3 files changed, 34 insertions(+), 15 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;
> }
Lets have the crypto changes as a separate patch, also we should
probably have a unit test to validate it.
> diff --git a/src/shared/crypto.c b/src/shared/crypto.c
> index 5c5e1217d..879ebd35d 100644
> --- a/src/shared/crypto.c
> +++ b/src/shared/crypto.c
> @@ -75,6 +75,8 @@ struct af_alg_iv {
> /* Maximum message length that can be passed to aes_cmac */
> #define CMAC_MSG_MAX 80
>
> +#define ATT_SIGN_LEN 12
> +
> struct bt_crypto {
> int ref_count;
> int ecb_aes;
> @@ -265,7 +267,8 @@ static inline void swap_buf(const uint8_t *src, uint8_t *dst, uint16_t len)
>
> bool bt_crypto_sign_att(struct bt_crypto *crypto, const uint8_t key[16],
> const uint8_t *m, uint16_t m_len,
> - uint32_t sign_cnt, uint8_t signature[12])
> + uint32_t sign_cnt,
> + uint8_t signature[ATT_SIGN_LEN])
> {
> int fd;
> int len;
> @@ -319,10 +322,25 @@ bool bt_crypto_sign_att(struct bt_crypto *crypto, const uint8_t key[16],
> * 12 octets
> */
> swap_buf(out, tmp, 16);
> - memcpy(signature, tmp + 4, 12);
> + memcpy(signature, tmp + 4, ATT_SIGN_LEN);
>
> return true;
> }
> +
> +bool bt_crypto_verify_att_sign(struct bt_crypto *crypto, const uint8_t key[16],
> + const uint8_t *pdu, uint16_t pdu_len)
> +{
> + uint8_t generated_sign[ATT_SIGN_LEN];
> + const uint8_t *sign = pdu + pdu_len - ATT_SIGN_LEN;
> + uint32_t sign_cnt = get_le32(sign);
We should probablu check if pdu_len is actually bigger than
ATT_SIGN_LEN before trying anything otherwise this may cause negative
result with pdu_len - ATT_SIGN_LEN.
> + if (!bt_crypto_sign_att(crypto, key, pdu, pdu_len - ATT_SIGN_LEN,
> + sign_cnt, generated_sign))
> + return false;
> +
> + return memcmp(generated_sign, sign, ATT_SIGN_LEN) == 0;
> +}
> +
> /*
> * Security function e
> *
> diff --git a/src/shared/crypto.h b/src/shared/crypto.h
> index c58d2e104..d17daa835 100644
> --- a/src/shared/crypto.h
> +++ b/src/shared/crypto.h
> @@ -62,5 +62,7 @@ bool bt_crypto_h6(struct bt_crypto *crypto, const uint8_t w[16],
> bool bt_crypto_sign_att(struct bt_crypto *crypto, const uint8_t key[16],
> const uint8_t *m, uint16_t m_len,
> uint32_t sign_cnt, uint8_t signature[12]);
> +bool bt_crypto_verify_att_sign(struct bt_crypto *crypto, const uint8_t key[16],
> + const uint8_t *pdu, uint16_t pdu_len);
> bool bt_crypto_gatt_hash(struct bt_crypto *crypto, struct iovec *iov,
> size_t iov_len, uint8_t res[16]);
> --
> 2.26.0.rc2.310.g2932bb562d-goog
>
--
Luiz Augusto von Dentz
