This patch for signing outgoing, csrk and cryto stored in bt_att.
the verifying incoming ATT PDU implementation introduce the
shared/gatt-server which combined with shared/gatt-db.c, so need
more disccussion.
Signed-off-by: Gu Chaojie <[email protected]>
---
src/shared/att-types.h | 3 +++
src/shared/att.c | 32 ++++++++++++++++++++++++++++++++
src/shared/gatt-client.c | 12 +++++++-----
3 files changed, 42 insertions(+), 5 deletions(-)
diff --git a/src/shared/att-types.h b/src/shared/att-types.h
index b85c969..b03b8f2 100644
--- a/src/shared/att-types.h
+++ b/src/shared/att-types.h
@@ -25,6 +25,9 @@
#define BT_ATT_DEFAULT_LE_MTU 23
+/* Len of signature in write signed packet */
+#define BT_ATT_SIGNATURE_LEN 12
+
/* ATT protocol opcodes */
#define BT_ATT_OP_ERROR_RSP 0x01
#define BT_ATT_OP_MTU_REQ 0x02
diff --git a/src/shared/att.c b/src/shared/att.c
index a5cab45..76fc360 100644
--- a/src/shared/att.c
+++ b/src/shared/att.c
@@ -36,6 +36,7 @@
#include "lib/uuid.h"
#include "src/shared/att.h"
#include "src/shared/att-types.h"
+#include "src/shared/crypto.h"
#define ATT_MIN_PDU_LEN 1 /* At least 1 byte for the opcode. */
#define ATT_OP_CMD_MASK 0x40
@@ -77,6 +78,16 @@ struct bt_att {
bt_att_debug_func_t debug_callback;
bt_att_destroy_func_t debug_destroy;
void *debug_data;
+
+ struct bt_crypto *crypto;
+
+ bool valid_remote_csrk;
+ uint8_t remote_csrk[16];
+ uint32_t remote_sign_cnt;
+
+ bool valid_local_csrk;
+ uint8_t local_csrk[16];
+ uint32_t local_sign_cnt;
};
enum att_op_type {
@@ -277,6 +288,12 @@ static bool encode_pdu(struct att_send_op *op, const void *pdu,
uint16_t length, uint16_t mtu)
{
uint16_t pdu_len = 1;
+ struct bt_att *att = NULL;
+
+ if (op->opcode == BT_ATT_OP_SIGNED_WRITE_CMD) {
+ pdu_len += BT_ATT_SIGNATURE_LEN;
+ att = op->user_data;
+ }
if (length && pdu)
pdu_len += length;
@@ -293,6 +310,16 @@ static bool encode_pdu(struct att_send_op *op, const void *pdu,
if (pdu_len > 1)
memcpy(op->pdu + 1, pdu, length);
+ if (att) {
+ if (!bt_crypto_sign_att(att->crypto,
+ att->local_csrk,
+ op->pdu,
+ 1 + length,
+ att->local_sign_cnt,
+ &((uint8_t *) op->pdu)[1 + length]))
+ return false;
+ }
+
return true;
}
@@ -707,6 +734,10 @@ struct bt_att *bt_att_new(int fd)
if (!att->io)
goto fail;
+ att->crypto = bt_crypto_new();
+ if (!att->crypto)
+ goto fail;
+
att->req_queue = queue_new();
if (!att->req_queue)
goto fail;
@@ -741,6 +772,7 @@ fail:
queue_destroy(att->write_queue, NULL);
queue_destroy(att->notify_list, NULL);
queue_destroy(att->disconn_list, NULL);
+ bt_crypto_unref(att->crypto);
io_destroy(att->io);
free(att->buf);
free(att);
diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
index 1a157ec..2c94ad3 100644
--- a/src/shared/gatt-client.c
+++ b/src/shared/gatt-client.c
@@ -973,19 +973,21 @@ bool bt_gatt_client_read_long_value(struct bt_gatt_client *client,
bool bt_gatt_client_write_without_response(struct bt_gatt_client *client,
uint16_t value_handle,
bool signed_write,
- uint8_t *value, uint16_t length) {
+ uint8_t *value, uint16_t length)
+{
uint8_t pdu[2 + length];
if (!client)
return 0;
- /* TODO: Support this once bt_att_send supports signed writes. */
- if (signed_write)
- return 0;
-
put_le16(value_handle, pdu);
memcpy(pdu + 2, value, length);
+ if (signed_write)
+ return bt_att_send(client->att, BT_ATT_OP_SIGNED_WRITE_CMD,
+ pdu, sizeof(pdu), NULL,
+ client->att, NULL);
+
return bt_att_send(client->att, BT_ATT_OP_WRITE_CMD, pdu, sizeof(pdu),
NULL, NULL, NULL);
}
--
1.7.10.4
Hi Chaojie,
>> > + pdu_len += BT_ATT_SIGNATURE_LEN;
>> > + att = op->user_data;
>>
>> I don't think that this code here works. op->user_data refers to user data provided in
>> bt_att_send and not the bt_att structure. Instead, I would just add a "struct bt_att
>> *att" field to struct att_send_op and remove the local variable "att".
>
> In my implementation , op->user_data in the signed write command code path refer to
> the bt_att structure because struct bt_att provided in bt_att_send parameter user_data.
> So it could be work.
What would happen to the user passed user_data then? :)
> But your proposal make more sense if extend the att_send_op struct to store struct bt_att *att
> Because user_data should be provided for callback , current implementation style is ugly indeed.
>
Yep, sounds better.
> This point is my major purpose for this patch discussion. We can see in android/gatt.c,
> Csrk and sign counter get from struct device in new_csrk_callback by mgmt_register
> event from kernel. And we can get att->csrk from struct device. However, in
> btgatt-client tool, there is no struct device and no event notification handler.
> So if we have to implemetnt bt_att_set_csrk, how can we get csrk ?
We want a generic way to obtain the CSRK and the sign count.
Eventually bluetoothd will do exactly that where src/device.c (which
will own a struct bt_gatt_client instance) will give us the CSRK and
initialize the sign count. For testing purposes you're welcome to add
this functionality to tools/btgatt-client by directly hooking into
shared/mgmt.
> And for the lifecycle of sign_counter, it should be updated to store in files after signed write.
> Btgatt-client need to create special file to do that?
This is a good question. I think as far as btgatt-client is concerned,
I wouldn't worry about persisting the sign count. btgatt-client is
meant to be a simple tool and not maintain state the way the daemon
does. So, keep it simple there. Maybe somehow reset the count to 0
every time btgatt-client restarts?
Cheers,
Arman
SGksIGFybWFuLA0KDQo+ID4gVGhpcyBwYXRjaCBmb3Igc2lnbmluZyBvdXRnb2luZywgY3NyayBh
bmQgY3J5dG8gc3RvcmVkIGluIGJ0X2F0dC4NCj4gPiB0aGUgdmVyaWZ5aW5nIGluY29taW5nIEFU
VCBQRFUgaW1wbGVtZW50YXRpb24gaW50cm9kdWNlIHRoZQ0KPiA+IHNoYXJlZC9nYXR0LXNlcnZl
ciB3aGljaCBjb21iaW5lZCB3aXRoIHNoYXJlZC9nYXR0LWRiLmMsIHNvIG5lZWQgbW9yZQ0KPiA+
IGRpc2NjdXNzaW9uLg0KPiA+DQo+ID4gU2lnbmVkLW9mZi1ieTogR3UgQ2hhb2ppZSA8Y2hhby5q
aWUuZ3VAaW50ZWwuY29tPg0KPiANCj4gSSBkb24ndCB0aGluayBpbmNsdWRlICJTaWduZWQtb2Zm
LWJ5IiBpbiBjb21taXQgbWVzc2FnZXMgaW4gQmx1ZVouDQo+IA0KDQogSSB3aWxsIGZvbGxvdyBi
bHVleiBydWxlcy4NCg0KPiA+ICsvKiBMZW4gb2Ygc2lnbmF0dXJlIGluIHdyaXRlIHNpZ25lZCBw
YWNrZXQgKi8NCj4gPiArI2RlZmluZSBCVF9BVFRfU0lHTkFUVVJFX0xFTiAgICAgICAgICAgMTIN
Cj4gDQo+IENhbiB5b3Ugc3BlbGwgb3V0ICJMZW5ndGggb2Ygc2lnbmF0dXJlIiBpbiB0aGUgY29t
bWVudCBpbnN0ZWFkIG9mIGp1c3QgIkxlbiI/DQoNCk5vIHByb2JsZW0uDQoNCj4gDQo+ID4gKyAg
ICAgICBib29sIHZhbGlkX3JlbW90ZV9jc3JrOw0KPiA+ICsgICAgICAgdWludDhfdCByZW1vdGVf
Y3Nya1sxNl07DQo+ID4gKyAgICAgICB1aW50MzJfdCByZW1vdGVfc2lnbl9jbnQ7DQo+IENhbiB3
ZSBhZGQgdGhlc2UgbGF0ZXIgaWYgdGhleSBhcmVuJ3QgYmVpbmcgdXNlZCB5ZXQ/DQo+IA0KSSBh
Z3JlZSB3aXRoIHRoYXQuDQoNCj4gPiArICAgICAgIHN0cnVjdCBidF9hdHQgKmF0dCA9IE5VTEw7
DQo+ID4gKw0KPiA+ICsgICAgICAgaWYgKG9wLT5vcGNvZGUgPT0gQlRfQVRUX09QX1NJR05FRF9X
UklURV9DTUQpIHsNCj4gDQo+IEFjdHVhbGx5LCBpbnN0ZWFkIG9mIGNoZWNraW5nIGV4cGxpY2l0
bHkgZm9yIEJUX0FUVF9PUF9TSUdORURfV1JJVEVfQ01ELCBpdA0KPiBtaWdodCBiZSBiZXR0ZXIg
dG8gY2hlY2sgaWYgdGhlIG9wY29kZSBoYXMgdGhlICJzaWduZWQiIGJpdCBzZXQgaW4gZ2VuZXJh
bC4gKFlvdSBjYW4NCj4gdXNlIEFUVF9PUF9TSUdORURfTUFTSywgd2hpY2ggaXMgZGVmaW5lZCBp
biB0aGlzIGZpbGUpLg0KPiANCj4gDQo+ID4gKyAgICAgICAgICAgICAgIHBkdV9sZW4gKz0gQlRf
QVRUX1NJR05BVFVSRV9MRU47DQo+ID4gKyAgICAgICAgICAgICAgIGF0dCA9IG9wLT51c2VyX2Rh
dGE7DQo+IA0KPiBJIGRvbid0IHRoaW5rIHRoYXQgdGhpcyBjb2RlIGhlcmUgd29ya3MuIG9wLT51
c2VyX2RhdGEgcmVmZXJzIHRvIHVzZXIgZGF0YSBwcm92aWRlZCBpbg0KPiBidF9hdHRfc2VuZCBh
bmQgbm90IHRoZSBidF9hdHQgc3RydWN0dXJlLiBJbnN0ZWFkLCBJIHdvdWxkIGp1c3QgYWRkIGEg
InN0cnVjdCBidF9hdHQNCj4gKmF0dCIgZmllbGQgdG8gc3RydWN0IGF0dF9zZW5kX29wIGFuZCBy
ZW1vdmUgdGhlIGxvY2FsIHZhcmlhYmxlICJhdHQiLg0KDQpJbiBteSBpbXBsZW1lbnRhdGlvbiAs
IG9wLT51c2VyX2RhdGEgaW4gdGhlIHNpZ25lZCB3cml0ZSBjb21tYW5kIGNvZGUgcGF0aCByZWZl
ciB0bw0KdGhlIGJ0X2F0dCBzdHJ1Y3R1cmUgYmVjYXVzZSBzdHJ1Y3QgYnRfYXR0IHByb3ZpZGVk
IGluIGJ0X2F0dF9zZW5kIHBhcmFtZXRlciB1c2VyX2RhdGEuDQpTbyBpdCBjb3VsZCBiZSB3b3Jr
LiANCkJ1dCB5b3VyIHByb3Bvc2FsIG1ha2UgbW9yZSBzZW5zZSBpZiBleHRlbmQgdGhlIGF0dF9z
ZW5kX29wIHN0cnVjdCB0byBzdG9yZSBzdHJ1Y3QgYnRfYXR0ICphdHQgDQpCZWNhdXNlIHVzZXJf
ZGF0YSBzaG91bGQgYmUgcHJvdmlkZWQgZm9yIGNhbGxiYWNrICwgY3VycmVudCBpbXBsZW1lbnRh
dGlvbiBzdHlsZSBpcyB1Z2x5IGluZGVlZC4gDQoNCj4gPiArICAgICAgIH0NCj4gPg0KPiA+ICAg
ICAgICAgaWYgKGxlbmd0aCAmJiBwZHUpDQo+ID4gICAgICAgICAgICAgICAgIHBkdV9sZW4gKz0g
bGVuZ3RoOw0KPiA+IEBAIC0yOTMsNiArMzEwLDE2IEBAIHN0YXRpYyBib29sIGVuY29kZV9wZHUo
c3RydWN0IGF0dF9zZW5kX29wICpvcCwgY29uc3QNCj4gdm9pZCAqcGR1LA0KPiA+ICAgICAgICAg
aWYgKHBkdV9sZW4gPiAxKQ0KPiA+ICAgICAgICAgICAgICAgICBtZW1jcHkob3AtPnBkdSArIDEs
IHBkdSwgbGVuZ3RoKTsNCj4gPg0KPiA+ICsgICAgICAgaWYgKGF0dCkgew0KPiA+ICsgICAgICAg
ICAgICAgICBpZiAoIWJ0X2NyeXB0b19zaWduX2F0dChhdHQtPmNyeXB0bywNCj4gPiArICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgYXR0LT5sb2NhbF9jc3JrLA0KPiA+ICsg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBvcC0+cGR1LA0KPiA+ICsgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAxICsgbGVuZ3RoLA0KPiA+ICsgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBhdHQtPmxvY2FsX3NpZ25fY250LA0K
PiA+ICsgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAmKCh1aW50OF90ICop
IG9wLT5wZHUpWzEgKw0KPiBsZW5ndGhdKSkNCj4gPiArICAgICAgICAgICAgICAgcmV0dXJuIGZh
bHNlOw0KPiA+ICsgICAgICAgfQ0KPiANCj4gWW91IHNob3VsZG4ndCBoYXZlIHRvIGNoZWNrIGlm
ICJhdHQiIGlzIE5VTEwuICJhdHQiIHNob3VsZCBhbHdheXMgYmUgcHJlc2VudCBpZiBzdHJ1Y3QN
Cj4gYXR0X3NlbmRfb3AgaGFzIGEgZmllbGQgZm9yIGl0Lg0KDQpUaGlzIHByb2JsZW0gd291bGQg
YmUgZml4ZWQgYnkgZXh0ZW5kaW5nIHN0cnVjdCBhdHRfc2VuZF9vcC4NCg0KPiBJIHdvdWxkIGFs
c28gcmVtb3ZlIHRoZSBuZXN0ZWQgaWYgc3RhdGVtZW50IGFuZCBqdXN0IHJldHVybiB0aGUgcmV0
dXJuIHZhbHVlIG9mDQo+IGJ0X2NyeXB0b19zaWduX2F0dCBpbmxpbmUuDQo+IA0KPiBPdGhlciB0
aGFuIHRoYXQsIEkgZG9uJ3Qgc2VlIGhvdyB0aGUgQ1NSSyBpcyBiZWluZyBwcm92aWRlZCB0byB0
aGUgYnRfYXR0IHN0cnVjdHVyZS4NCj4gU2hvdWxkbid0IHRoZXJlIGJlIGEgYnRfYXR0X3NldF9j
c3JrIG9mIHNvcnRzPw0KPiANClRoaXMgcG9pbnQgaXMgbXkgbWFqb3IgcHVycG9zZSBmb3IgdGhp
cyBwYXRjaCBkaXNjdXNzaW9uLiBXZSBjYW4gc2VlIGluIGFuZHJvaWQvZ2F0dC5jLA0KQ3NyayBh
bmQgc2lnbiBjb3VudGVyIGdldCBmcm9tIHN0cnVjdCBkZXZpY2UgaW4gbmV3X2NzcmtfY2FsbGJh
Y2sgYnkgbWdtdF9yZWdpc3Rlcg0KZXZlbnQgZnJvbSBrZXJuZWwuIEFuZCB3ZSBjYW4gZ2V0IGF0
dC0+Y3NyayBmcm9tIHN0cnVjdCBkZXZpY2UuIEhvd2V2ZXIsIGluIA0KYnRnYXR0LWNsaWVudCB0
b29sLCB0aGVyZSBpcyBubyBzdHJ1Y3QgZGV2aWNlIGFuZCBubyBldmVudCBub3RpZmljYXRpb24g
aGFuZGxlci4NClNvIGlmIHdlIGhhdmUgdG8gaW1wbGVtZXRudCBidF9hdHRfc2V0X2NzcmssIGhv
dyBjYW4gd2UgZ2V0IGNzcmsgPw0KDQpBbmQgZm9yIHRoZSBsaWZlY3ljbGUgb2Ygc2lnbl9jb3Vu
dGVyLCBpdCBzaG91bGQgYmUgdXBkYXRlZCB0byBzdG9yZSBpbiBmaWxlcyBhZnRlciBzaWduZWQg
d3JpdGUuIA0KQnRnYXR0LWNsaWVudCBuZWVkIHRvIGNyZWF0ZSBzcGVjaWFsIGZpbGUgdG8gZG8g
dGhhdD8gDQoNCkJlc3QgUmVnYXJkcw0KQ2hhb2ppZSBHdQ0K
Hi Chaojie,
> This patch for signing outgoing, csrk and cryto stored in bt_att.
> the verifying incoming ATT PDU implementation introduce the
> shared/gatt-server which combined with shared/gatt-db.c, so need
> more disccussion.
>
> Signed-off-by: Gu Chaojie <[email protected]>
I don't think include "Signed-off-by" in commit messages in BlueZ.
> +/* Len of signature in write signed packet */
> +#define BT_ATT_SIGNATURE_LEN 12
Can you spell out "Length of signature" in the comment instead of just "Len"?
> + bool valid_remote_csrk;
> + uint8_t remote_csrk[16];
> + uint32_t remote_sign_cnt;
Can we add these later if they aren't being used yet?
> + struct bt_att *att = NULL;
> +
> + if (op->opcode == BT_ATT_OP_SIGNED_WRITE_CMD) {
Actually, instead of checking explicitly for
BT_ATT_OP_SIGNED_WRITE_CMD, it might be better to check if the opcode
has the "signed" bit set in general. (You can use ATT_OP_SIGNED_MASK,
which is defined in this file).
> + pdu_len += BT_ATT_SIGNATURE_LEN;
> + att = op->user_data;
I don't think that this code here works. op->user_data refers to user
data provided in bt_att_send and not the bt_att structure. Instead, I
would just add a "struct bt_att *att" field to struct att_send_op and
remove the local variable "att".
> + }
>
> if (length && pdu)
> pdu_len += length;
> @@ -293,6 +310,16 @@ static bool encode_pdu(struct att_send_op *op, const void *pdu,
> if (pdu_len > 1)
> memcpy(op->pdu + 1, pdu, length);
>
> + if (att) {
> + if (!bt_crypto_sign_att(att->crypto,
> + att->local_csrk,
> + op->pdu,
> + 1 + length,
> + att->local_sign_cnt,
> + &((uint8_t *) op->pdu)[1 + length]))
> + return false;
> + }
You shouldn't have to check if "att" is NULL. "att" should always be
present if struct att_send_op has a field for it.
I would also remove the nested if statement and just return the return
value of bt_crypto_sign_att inline.
Other than that, I don't see how the CSRK is being provided to the
bt_att structure. Shouldn't there be a bt_att_set_csrk of sorts?
Cheers,
Arman