| Message ID | 20200330183400.Bluez.v2.1.Id504860f0b01d123e0b29ed953dbdf02d738fed7@changeid (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Luiz Von Dentz |
| Headers | show |
| Series | [Bluez,v2] shared/att: Check the signature of att packets | expand |
Hi Archie, On Mon, Mar 30, 2020 at 3:36 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 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 >
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; } 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); + + 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]);
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 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(-)