| Message ID | 20200406194749.Bluez.v3.3.I28a54f18ca82b58e44689a0c76663e735fefb6f1@changeid (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Luiz Von Dentz |
| Headers | show |
| Series | Check the signature of att packets | expand |
Hi Archie, On Mon, Apr 6, 2020 at 4:49 AM Archie Pusaka <apusaka@google.com> wrote: > > From: Archie Pusaka <apusaka@chromium.org> > > Tested to pass these BT certification test > SM/MAS/SIGN/BV-03-C > SM/MAS/SIGN/BI-01-C > > --- > > Changes in v3: > - 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. > > src/shared/att.c | 25 ++++++++++++------------- > 1 file changed, 12 insertions(+), 13 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; > } > > -- > 2.26.0.292.g33ef6b2f38-goog This actually make unit/test-gatt get stuck for some reason, so you will need to investigate and make it work with existing tests or fix the tests if there are actually not conforming to the spec.
Hi Luiz, The reason test-gatt.c is stuck is because there exists a test which sends pdu with opcode = 0xBF, an invalid opcode, to test the robustness of the system of an invalid request. By applying the patch, instead of replying to the pdu, we silently ignore it, which makes the test stuck. The opcode has the "signed" bit (0x80) on and "command" bit (0x40) off, which makes it in a difficult situation where it shouldn't be possible and as far as I know is not explicitly discussed in the spec. I shall fall back to this part from the BT spec, vol 3. Part C section 10.4.2: "A device receiving signed data shall authenticate it by performing the Signing Algorithm. 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." Therefore, I shall modify the failing test, and add another case which is just "invalid request" but without the signed bit. Thanks, Archie On Tue, 7 Apr 2020 at 05:16, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Archie, > > On Mon, Apr 6, 2020 at 4:49 AM Archie Pusaka <apusaka@google.com> wrote: > > > > From: Archie Pusaka <apusaka@chromium.org> > > > > Tested to pass these BT certification test > > SM/MAS/SIGN/BV-03-C > > SM/MAS/SIGN/BI-01-C > > > > --- > > > > Changes in v3: > > - 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. > > > > src/shared/att.c | 25 ++++++++++++------------- > > 1 file changed, 12 insertions(+), 13 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; > > } > > > > -- > > 2.26.0.292.g33ef6b2f38-goog > > This actually make unit/test-gatt get stuck for some reason, so you > will need to investigate and make it work with existing tests or fix > the tests if there are actually not conforming to the spec. > > -- > Luiz Augusto von Dentz
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; }
From: Archie Pusaka <apusaka@chromium.org> Tested to pass these BT certification test SM/MAS/SIGN/BV-03-C SM/MAS/SIGN/BI-01-C --- Changes in v3: - 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. src/shared/att.c | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-)