| Message ID | 20200407165521.Bluez.v4.4.I6813a39e5d8499d24471d7b575c7ef6c493a046c@changeid (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Check the signature of att packets | expand |
Hi Archie, On Tue, Apr 7, 2020 at 1:56 AM Archie Pusaka <apusaka@google.com> wrote: > > From: Archie Pusaka <apusaka@chromium.org> > > The BT spec doesn't make it explicit of what should happen when > receiving a bad signed att request packet. > > According to BT core spec Vol 3, Part C, Sec 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. In my interpretation 'ignore the received Data PDU' may not supersed the followin statement: > According to BT core spec Vol 3, Part F, Sec 3.3 > If a server receives a request that it does not support, then the > server shall respond with the ATT_ERROR_RSP PDU with the error code > Request Not Supported. So what I think we should be doing is to ignore the received data, meaning it should not even be processed as a signed data since we don't understand the command, _and_ respond with ATT_ERROR_RSP, otherwise we risk having the 30 seconds timeout and disconnecting since the request is no responded. > This patch does this two things: > (1) Removing the signed bit to the existing tests so they are not > in a conflicting state within the bluetooth spec, while still > keeping the original intent of the test. > (2) Add another test that purposely fall within this grey area > with some comments. > --- > > Changes in v4: > - Fixing test-gatt.c > > Changes in v3: None > Changes in v2: None > > unit/test-gatt.c | 32 +++++++++++++++++++++++++++----- > 1 file changed, 27 insertions(+), 5 deletions(-) > > diff --git a/unit/test-gatt.c b/unit/test-gatt.c > index 36dd2847c..139a6fc72 100644 > --- a/unit/test-gatt.c > +++ b/unit/test-gatt.c > @@ -4473,16 +4473,38 @@ int main(int argc, char *argv[]) > raw_pdu(0x18, 0x01), > raw_pdu(0x01, 0x18, 0x25, 0x00, 0x06)); > > - define_test_server("/robustness/unkown-request", > + define_test_server("/robustness/unknown-request", > test_server, service_db_1, NULL, > raw_pdu(0x03, 0x00, 0x02), > - raw_pdu(0xbf, 0x00), > - raw_pdu(0x01, 0xbf, 0x00, 0x00, 0x06)); > + raw_pdu(0x3f, 0x00), > + raw_pdu(0x01, 0x3f, 0x00, 0x00, 0x06)); Afaik we used 0xbf because that was what PTS was using. > + define_test_server("/robustness/unknown-command", > + test_server, service_db_1, NULL, > + raw_pdu(0x03, 0x00, 0x02), > + raw_pdu(0x7f, 0x00), > + raw_pdu()); > > - define_test_server("/robustness/unkown-command", > + /* > + * According to BT core spec Vol 3, Part C, Sec 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. > + * > + * However, according to BT core spec Vol 3, Part F, Sec 3.3 > + * If a server receives a request that it does not support, then the > + * server shall respond with the ATT_ERROR_RSP PDU with the error code > + * Request Not Supported. > + * > + * Since there is no explicit instruction on what should be done in > + * case the server receives a bad signed unsupported request, here > + * we just ignore the received PDU. > + */ > + define_test_server("/robustness/signed-unknown-request", > test_server, service_db_1, NULL, > raw_pdu(0x03, 0x00, 0x02), > - raw_pdu(0xff, 0x00), > + raw_pdu(0xbf, 0x00), > raw_pdu()); > > return tester_run(); > -- > 2.26.0.292.g33ef6b2f38-goog >
diff --git a/unit/test-gatt.c b/unit/test-gatt.c index 36dd2847c..139a6fc72 100644 --- a/unit/test-gatt.c +++ b/unit/test-gatt.c @@ -4473,16 +4473,38 @@ int main(int argc, char *argv[]) raw_pdu(0x18, 0x01), raw_pdu(0x01, 0x18, 0x25, 0x00, 0x06)); - define_test_server("/robustness/unkown-request", + define_test_server("/robustness/unknown-request", test_server, service_db_1, NULL, raw_pdu(0x03, 0x00, 0x02), - raw_pdu(0xbf, 0x00), - raw_pdu(0x01, 0xbf, 0x00, 0x00, 0x06)); + raw_pdu(0x3f, 0x00), + raw_pdu(0x01, 0x3f, 0x00, 0x00, 0x06)); + + define_test_server("/robustness/unknown-command", + test_server, service_db_1, NULL, + raw_pdu(0x03, 0x00, 0x02), + raw_pdu(0x7f, 0x00), + raw_pdu()); - define_test_server("/robustness/unkown-command", + /* + * According to BT core spec Vol 3, Part C, Sec 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. + * + * However, according to BT core spec Vol 3, Part F, Sec 3.3 + * If a server receives a request that it does not support, then the + * server shall respond with the ATT_ERROR_RSP PDU with the error code + * Request Not Supported. + * + * Since there is no explicit instruction on what should be done in + * case the server receives a bad signed unsupported request, here + * we just ignore the received PDU. + */ + define_test_server("/robustness/signed-unknown-request", test_server, service_db_1, NULL, raw_pdu(0x03, 0x00, 0x02), - raw_pdu(0xff, 0x00), + raw_pdu(0xbf, 0x00), raw_pdu()); return tester_run();
From: Archie Pusaka <apusaka@chromium.org> The BT spec doesn't make it explicit of what should happen when receiving a bad signed att request packet. According to BT core spec Vol 3, Part C, Sec 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. According to BT core spec Vol 3, Part F, Sec 3.3 If a server receives a request that it does not support, then the server shall respond with the ATT_ERROR_RSP PDU with the error code Request Not Supported. This patch does this two things: (1) Removing the signed bit to the existing tests so they are not in a conflicting state within the bluetooth spec, while still keeping the original intent of the test. (2) Add another test that purposely fall within this grey area with some comments. --- Changes in v4: - Fixing test-gatt.c Changes in v3: None Changes in v2: None unit/test-gatt.c | 32 +++++++++++++++++++++++++++----- 1 file changed, 27 insertions(+), 5 deletions(-)