2014-05-28 14:44:28

by Lukasz Rymanowski

[permalink] [raw]
Subject: [PATCH 0/9] android: Fixes to sign write

This patch set brings couple of bug fixes founded during tests against PTS.
Biggest change is that I had to move signing procedure down to att.c as
not only payload need to be signed but also opcode and handle.
Therefore it makes sense to generate signature in place where pdu is encoded

With those patches PTS TC_GAW_CL_BV_02_C and TC_GAW_SR_BV_02_C are passing


Lukasz Rymanowski (9):
android/gatt: Verify signature counter from remote
android/gatt: Fix handling signed write command from remote
android/gatt: Fix signed write command encoding
shared/crypto: Fix concatenate of sign counter into the msg
shared/crypto: Fix byte order of sign counter added to signature
shared/crypto: Change swap128 to swap_buf
shared/crypto: Fix byte order of message
unit/crypto: Update unit test vectors after changes in singing process
android/pts: Update GATT PTS test results

Makefile.am | 3 ++-
Makefile.tools | 4 +++-
android/gatt.c | 25 ++++++++++++-------------
android/pts-gatt.txt | 21 +++++++++++++++++++--
attrib/att.c | 11 +++++++----
attrib/att.h | 6 +++++-
attrib/gatt.c | 11 ++++++++---
attrib/gatt.h | 4 +++-
src/shared/crypto.c | 26 +++++++++++++++-----------
unit/test-crypto.c | 6 +++---
10 files changed, 77 insertions(+), 40 deletions(-)

--
1.8.4



2014-05-28 14:44:42

by Lukasz Rymanowski

[permalink] [raw]
Subject: [PATCH 8/9] unit/crypto: Update unit test vectors after changes in singing process

---
unit/test-crypto.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/unit/test-crypto.c b/unit/test-crypto.c
index 9bc072b..c5a6954 100644
--- a/unit/test-crypto.c
+++ b/unit/test-crypto.c
@@ -62,7 +62,7 @@ static const uint8_t msg_2[] = {
};

static const uint8_t t_msg_2[] = {
- 0x00, 0x00, 0x00, 0x00, 0x79, 0xc1, 0x60, 0x5b, 0x71, 0x32, 0x68, 0x59
+ 0x00, 0x00, 0x00, 0x00, 0x27, 0x39, 0x74, 0xf4, 0x39, 0x2a, 0x23, 0x2a
};

static const struct test_data test_data_2 = {
@@ -79,7 +79,7 @@ static const uint8_t msg_3[] = {
};

static const uint8_t t_msg_3[12] = {
- 0x00, 0x00, 0x00, 0x00, 0x3e, 0xc3, 0x46, 0x95, 0x2c, 0xdf, 0x88, 0x32
+ 0x00, 0x00, 0x00, 0x00, 0xb7, 0xca, 0x94, 0xab, 0x87, 0xc7, 0x82, 0x18
};

static const struct test_data test_data_3 = {
@@ -98,7 +98,7 @@ static const uint8_t msg_4[] = {
};

static const uint8_t t_msg_4[12] = {
- 0x00, 0x00, 0x00, 0x00, 0x43, 0x0c, 0xaa, 0x71, 0x19, 0x73, 0xbb, 0x59
+ 0x00, 0x00, 0x00, 0x00, 0x44, 0xe1, 0xe6, 0xce, 0x1d, 0xf5, 0x13, 0x68
};

static const struct test_data test_data_4 = {
--
1.8.4


2014-05-28 14:44:43

by Lukasz Rymanowski

[permalink] [raw]
Subject: [PATCH 9/9] android/pts: Update GATT PTS test results

Update tests related to write signed
---
android/pts-gatt.txt | 21 +++++++++++++++++++--
1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/android/pts-gatt.txt b/android/pts-gatt.txt
index 60968e2..cd4660d 100644
--- a/android/pts-gatt.txt
+++ b/android/pts-gatt.txt
@@ -340,7 +340,15 @@ TC_GAW_CL_BV_01_C PASS haltest:
handle from logs
gattc write_characteristic
gattc disconnect
-TC_GAW_CL_BV_02_C INC
+TC_GAW_CL_BV_02_C PASS haltest:
+ bluetooth create_bond
+ gattc disconnect
+ gattc connect
+ gattc search_service
+ gattc get_characteristics
+ gattc write_characteristics: <type> 4
+ gattc disconnect
+
TC_GAW_CL_BV_03_C PASS haltest:
gattc connect
gattc search_service
@@ -580,7 +588,16 @@ TC_GAW_CL_BI_36_C PASS haltest:
gattc write_descriptor 2 <long_value>
gattc disconnect
TC_GAW_SR_BV_01_C INC
-TC_GAW_SR_BV_02_C INC
+TC_GAW_SR_BV_02_C PASS haltest:
+ gatts add service
+ gatts add_characteristics:
+ <properties> 66 <permisions> 147
+ gatts start_service
+ gattc listen
+ gatts send_response: (twice)
+ NOTE: gatts_request_write_cb shall be called
+ (verify it)
+
TC_GAW_SR_BI_01_C INC
TC_GAW_SR_BV_03_C INC
TC_GAW_SR_BI_02_C INC
--
1.8.4


2014-05-28 14:44:41

by Lukasz Rymanowski

[permalink] [raw]
Subject: [PATCH 8/9] unit/crypto: Update after changes in singing process

---
unit/test-crypto.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/unit/test-crypto.c b/unit/test-crypto.c
index 9bc072b..c5a6954 100644
--- a/unit/test-crypto.c
+++ b/unit/test-crypto.c
@@ -62,7 +62,7 @@ static const uint8_t msg_2[] = {
};

static const uint8_t t_msg_2[] = {
- 0x00, 0x00, 0x00, 0x00, 0x79, 0xc1, 0x60, 0x5b, 0x71, 0x32, 0x68, 0x59
+ 0x00, 0x00, 0x00, 0x00, 0x27, 0x39, 0x74, 0xf4, 0x39, 0x2a, 0x23, 0x2a
};

static const struct test_data test_data_2 = {
@@ -79,7 +79,7 @@ static const uint8_t msg_3[] = {
};

static const uint8_t t_msg_3[12] = {
- 0x00, 0x00, 0x00, 0x00, 0x3e, 0xc3, 0x46, 0x95, 0x2c, 0xdf, 0x88, 0x32
+ 0x00, 0x00, 0x00, 0x00, 0xb7, 0xca, 0x94, 0xab, 0x87, 0xc7, 0x82, 0x18
};

static const struct test_data test_data_3 = {
@@ -98,7 +98,7 @@ static const uint8_t msg_4[] = {
};

static const uint8_t t_msg_4[12] = {
- 0x00, 0x00, 0x00, 0x00, 0x43, 0x0c, 0xaa, 0x71, 0x19, 0x73, 0xbb, 0x59
+ 0x00, 0x00, 0x00, 0x00, 0x44, 0xe1, 0xe6, 0xce, 0x1d, 0xf5, 0x13, 0x68
};

static const struct test_data test_data_4 = {
--
1.8.4


2014-05-28 14:44:40

by Lukasz Rymanowski

[permalink] [raw]
Subject: [PATCH 7/9] shared/crypto: Swap message before signing it

---
src/shared/crypto.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/shared/crypto.c b/src/shared/crypto.c
index ad89174..c400081 100644
--- a/src/shared/crypto.c
+++ b/src/shared/crypto.c
@@ -265,6 +265,7 @@ bool bt_crypto_sign_att(struct bt_crypto *crypto, const uint8_t key[16],
uint8_t tmp[16], out[16];
uint16_t msg_len = m_len + sizeof(uint32_t);
uint8_t msg[msg_len];
+ uint8_t msg_s[msg_len];

if (!crypto)
return false;
@@ -283,7 +284,10 @@ bool bt_crypto_sign_att(struct bt_crypto *crypto, const uint8_t key[16],
if (fd < 0)
return false;

- len = send(fd, msg, msg_len, 0);
+ /* Swap msg before signing */
+ swap_buf(msg, msg_s, msg_len);
+
+ len = send(fd, msg_s, msg_len, 0);
if (len < 0) {
close(fd);
return false;
--
1.8.4


2014-05-28 14:44:39

by Lukasz Rymanowski

[permalink] [raw]
Subject: [PATCH 7/9] shared/crypto: Fix byte order of message

Message should be swapped before we send it to the kernel for signing
---
src/shared/crypto.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/shared/crypto.c b/src/shared/crypto.c
index ad89174..c400081 100644
--- a/src/shared/crypto.c
+++ b/src/shared/crypto.c
@@ -265,6 +265,7 @@ bool bt_crypto_sign_att(struct bt_crypto *crypto, const uint8_t key[16],
uint8_t tmp[16], out[16];
uint16_t msg_len = m_len + sizeof(uint32_t);
uint8_t msg[msg_len];
+ uint8_t msg_s[msg_len];

if (!crypto)
return false;
@@ -283,7 +284,10 @@ bool bt_crypto_sign_att(struct bt_crypto *crypto, const uint8_t key[16],
if (fd < 0)
return false;

- len = send(fd, msg, msg_len, 0);
+ /* Swap msg before signing */
+ swap_buf(msg, msg_s, msg_len);
+
+ len = send(fd, msg_s, msg_len, 0);
if (len < 0) {
close(fd);
return false;
--
1.8.4


2014-05-28 14:44:37

by Lukasz Rymanowski

[permalink] [raw]
Subject: [PATCH 5/9] shared/crypto: Fix order of sign counter added to signature

---
src/shared/crypto.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/shared/crypto.c b/src/shared/crypto.c
index b50baa5..d4c4cc7 100644
--- a/src/shared/crypto.c
+++ b/src/shared/crypto.c
@@ -301,7 +301,7 @@ bool bt_crypto_sign_att(struct bt_crypto *crypto, const uint8_t key[16],
* As to BT spec. 4.1 Vol[3], Part C, chapter 10.4.1 sign counter should
* be placed in the signature
*/
- put_le32(sign_cnt, out + 8);
+ put_be32(sign_cnt, out + 8);

/*
* The most significant octet of hash corresponds to out[0] - swap it.
--
1.8.4


2014-05-28 14:44:38

by Lukasz Rymanowski

[permalink] [raw]
Subject: [PATCH 6/9] shared/crypto: Change swap128 to swap_buf

We will need to swap bigger buffers so lets make swap128 more generic for
that purpose
---
src/shared/crypto.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/src/shared/crypto.c b/src/shared/crypto.c
index d4c4cc7..ad89174 100644
--- a/src/shared/crypto.c
+++ b/src/shared/crypto.c
@@ -248,12 +248,12 @@ static bool alg_encrypt(int fd, const void *inbuf, size_t inlen,
return true;
}

-static inline void swap128(const uint8_t src[16], uint8_t dst[16])
+static inline void swap_buf(const uint8_t *src, uint8_t *dst, uint16_t len)
{
int i;

- for (i = 0; i < 16; i++)
- dst[15 - i] = src[i];
+ for (i = 0; i < len; i++)
+ dst[len - 1 - i] = src[i];
}

bool bt_crypto_sign_att(struct bt_crypto *crypto, const uint8_t key[16],
@@ -276,7 +276,7 @@ bool bt_crypto_sign_att(struct bt_crypto *crypto, const uint8_t key[16],
put_le32(sign_cnt, msg + m_len);

/* The most significant octet of key corresponds to key[0] */
- swap128(key, tmp);
+ swap_buf(key, tmp, 16);

memcpy(signature, tmp + 4, 12);
fd = alg_new(crypto->cmac_aes, tmp, 16);
@@ -308,7 +308,7 @@ bool bt_crypto_sign_att(struct bt_crypto *crypto, const uint8_t key[16],
* Then truncate in most significant bit first order to a length of
* 12 octets
*/
- swap128(out, tmp);
+ swap_buf(out, tmp, 16);
memcpy(signature, tmp + 4, 12);

return true;
@@ -336,7 +336,7 @@ bool bt_crypto_e(struct bt_crypto *crypto, const uint8_t key[16],
return false;

/* The most significant octet of key corresponds to key[0] */
- swap128(key, tmp);
+ swap_buf(key, tmp, 16);

fd = alg_new(crypto->ecb_aes, tmp, 16);
if (fd < 0)
@@ -344,7 +344,7 @@ bool bt_crypto_e(struct bt_crypto *crypto, const uint8_t key[16],


/* Most significant octet of plaintextData corresponds to in[0] */
- swap128(plaintext, in);
+ swap_buf(plaintext, in, 16);

if (!alg_encrypt(fd, in, 16, out, 16)) {
close(fd);
@@ -352,7 +352,7 @@ bool bt_crypto_e(struct bt_crypto *crypto, const uint8_t key[16],
}

/* Most significant octet of encryptedData corresponds to out[0] */
- swap128(out, encrypted);
+ swap_buf(out, encrypted, 16);

close(fd);

--
1.8.4


2014-05-28 14:44:36

by Lukasz Rymanowski

[permalink] [raw]
Subject: [PATCH 5/9] shared/crypto: Fix byte order of sign counter added to signature

---
src/shared/crypto.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/shared/crypto.c b/src/shared/crypto.c
index b50baa5..d4c4cc7 100644
--- a/src/shared/crypto.c
+++ b/src/shared/crypto.c
@@ -301,7 +301,7 @@ bool bt_crypto_sign_att(struct bt_crypto *crypto, const uint8_t key[16],
* As to BT spec. 4.1 Vol[3], Part C, chapter 10.4.1 sign counter should
* be placed in the signature
*/
- put_le32(sign_cnt, out + 8);
+ put_be32(sign_cnt, out + 8);

/*
* The most significant octet of hash corresponds to out[0] - swap it.
--
1.8.4


2014-05-28 14:44:35

by Lukasz Rymanowski

[permalink] [raw]
Subject: [PATCH 4/9] shared/crypto: Fix concatenate of sign counter to the msg

This patch fixes incorrectly concatenated sign counter to the message
---
src/shared/crypto.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/shared/crypto.c b/src/shared/crypto.c
index e46f07f..b50baa5 100644
--- a/src/shared/crypto.c
+++ b/src/shared/crypto.c
@@ -273,7 +273,7 @@ bool bt_crypto_sign_att(struct bt_crypto *crypto, const uint8_t key[16],
memcpy(msg, m, m_len);

/* Add sign_counter to the message */
- put_le32(sign_cnt, msg + msg_len);
+ put_le32(sign_cnt, msg + m_len);

/* The most significant octet of key corresponds to key[0] */
swap128(key, tmp);
--
1.8.4


2014-05-28 14:44:32

by Lukasz Rymanowski

[permalink] [raw]
Subject: [PATCH 3/9] android/gatt: Fix sending sign write

As per spec (BT spec 4.1 Vol[3], Part F, chapter 3.4.5.4) we need to
take opcode, handle and parameters to generate signature.
In order to support it signing is moved to att.c, place where pdu is
assembly.
---
Makefile.am | 3 ++-
Makefile.tools | 4 +++-
android/gatt.c | 13 ++-----------
attrib/att.c | 11 +++++++----
attrib/att.h | 6 +++++-
attrib/gatt.c | 11 ++++++++---
attrib/gatt.h | 4 +++-
7 files changed, 30 insertions(+), 22 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 4e77019..dc88816 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -98,7 +98,8 @@ gdbus_libgdbus_internal_la_SOURCES = gdbus/gdbus.h \
attrib_sources = attrib/att.h attrib/att-database.h attrib/att.c \
attrib/gatt.h attrib/gatt.c \
attrib/gattrib.h attrib/gattrib.c \
- attrib/gatt-service.h attrib/gatt-service.c
+ attrib/gatt-service.h attrib/gatt-service.c \
+ src/shared/crypto.h src/shared/crypto.c

btio_sources = btio/btio.h btio/btio.c

diff --git a/Makefile.tools b/Makefile.tools
index 412a998..c24bdf7 100644
--- a/Makefile.tools
+++ b/Makefile.tools
@@ -356,7 +356,9 @@ attrib_gatttool_SOURCES = attrib/gatttool.c attrib/att.c attrib/gatt.c \
attrib/gattrib.c btio/btio.c \
attrib/gatttool.h attrib/interactive.c \
attrib/utils.c src/log.c client/display.c \
- client/display.h
+ client/display.h \
+ src/shared/crypto.h src/shared/crypto.c
+
attrib_gatttool_LDADD = lib/libbluetooth-internal.la @GLIB_LIBS@ -lreadline

tools_obex_client_tool_SOURCES = $(gobex_sources) $(btio_sources) \
diff --git a/android/gatt.c b/android/gatt.c
index 6638fe6..21cba75 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -44,7 +44,6 @@
#include "src/shared/util.h"
#include "src/shared/queue.h"
#include "src/shared/gatt-db.h"
-#include "src/shared/crypto.h"
#include "attrib/gattrib.h"
#include "attrib/att.h"
#include "attrib/gatt.h"
@@ -2642,7 +2641,6 @@ static void write_char_cb(guint8 status, const guint8 *pdu, guint16 len,
static bool signed_write_cmd(struct gatt_device *dev, uint16_t handle,
const uint8_t *value, uint16_t vlen)
{
- uint8_t s[ATT_SIGNATURE_LEN];
uint8_t csrk[16];
uint32_t sign_cnt;

@@ -2653,15 +2651,8 @@ static bool signed_write_cmd(struct gatt_device *dev, uint16_t handle,
return false;
}

- memset(s, 0, ATT_SIGNATURE_LEN);
-
- if (!bt_crypto_sign_att(crypto, csrk, value, vlen, sign_cnt, s)) {
- error("gatt: Could not sign att data");
- return false;
- }
-
- if (!gatt_signed_write_cmd(dev->attrib, handle, value, vlen, s, NULL,
- NULL)) {
+ if (!gatt_signed_write_cmd(dev->attrib, handle, value, vlen, crypto,
+ csrk, sign_cnt, NULL, NULL)) {
error("gatt: Could write signed cmd");
return false;
}
diff --git a/attrib/att.c b/attrib/att.c
index e7d5682..2680458 100644
--- a/attrib/att.c
+++ b/attrib/att.c
@@ -561,9 +561,10 @@ uint16_t dec_write_cmd(const uint8_t *pdu, size_t len, uint16_t *handle,
return len;
}

-uint16_t enc_signed_write_cmd(uint16_t handle,
- const uint8_t *value, size_t vlen,
- const uint8_t signature[12],
+uint16_t enc_signed_write_cmd(uint16_t handle, const uint8_t *value,
+ size_t vlen, struct bt_crypto *crypto,
+ const uint8_t csrk[16],
+ uint32_t sign_cnt,
uint8_t *pdu, size_t len)
{
const uint16_t hdr_len = sizeof(pdu[0]) + sizeof(handle);
@@ -581,7 +582,9 @@ uint16_t enc_signed_write_cmd(uint16_t handle,
if (vlen > 0)
memcpy(&pdu[hdr_len], value, vlen);

- memcpy(&pdu[hdr_len + vlen], signature, ATT_SIGNATURE_LEN);
+ if (!bt_crypto_sign_att(crypto, csrk, pdu, hdr_len + vlen, sign_cnt,
+ &pdu[hdr_len + vlen]))
+ return 0;

return min_len + vlen;
}
diff --git a/attrib/att.h b/attrib/att.h
index c92cd5d..2311aaf 100644
--- a/attrib/att.h
+++ b/attrib/att.h
@@ -22,6 +22,8 @@
*
*/

+#include "src/shared/crypto.h"
+
/* Len of signature in write signed packet */
#define ATT_SIGNATURE_LEN 12

@@ -134,7 +136,9 @@ uint16_t dec_write_cmd(const uint8_t *pdu, size_t len, uint16_t *handle,
uint8_t *value, size_t *vlen);
uint16_t enc_signed_write_cmd(uint16_t handle,
const uint8_t *value, size_t vlen,
- const uint8_t signature[12],
+ struct bt_crypto *crypto,
+ const uint8_t csrk[16],
+ uint32_t sign_cnt,
uint8_t *pdu, size_t len);
uint16_t dec_signed_write_cmd(const uint8_t *pdu, size_t len,
uint16_t *handle,
diff --git a/attrib/gatt.c b/attrib/gatt.c
index ce08003..27fb0b3 100644
--- a/attrib/gatt.c
+++ b/attrib/gatt.c
@@ -1067,7 +1067,9 @@ guint gatt_write_cmd(GAttrib *attrib, uint16_t handle, const uint8_t *value,

guint gatt_signed_write_cmd(GAttrib *attrib, uint16_t handle,
const uint8_t *value, int vlen,
- const uint8_t signature[12],
+ struct bt_crypto *crypto,
+ const uint8_t csrk[16],
+ uint32_t sign_cnt,
GDestroyNotify notify,
gpointer user_data)
{
@@ -1076,8 +1078,11 @@ guint gatt_signed_write_cmd(GAttrib *attrib, uint16_t handle,
guint16 plen;

buf = g_attrib_get_buffer(attrib, &buflen);
- plen = enc_signed_write_cmd(handle, value, vlen, signature, buf,
- buflen);
+ plen = enc_signed_write_cmd(handle, value, vlen, crypto, csrk, sign_cnt,
+ buf, buflen);
+ if (plen == 0)
+ return 0;
+
return g_attrib_send(attrib, 0, buf, plen, NULL, user_data, notify);
}

diff --git a/attrib/gatt.h b/attrib/gatt.h
index 2d869e3..f6db10f 100644
--- a/attrib/gatt.h
+++ b/attrib/gatt.h
@@ -107,7 +107,9 @@ guint gatt_write_cmd(GAttrib *attrib, uint16_t handle, const uint8_t *value,

guint gatt_signed_write_cmd(GAttrib *attrib, uint16_t handle,
const uint8_t *value, int vlen,
- const uint8_t signature[12],
+ struct bt_crypto *crypto,
+ const uint8_t csrk[16],
+ uint32_t sign_cnt,
GDestroyNotify notify,
gpointer user_data);
guint gatt_read_char_by_uuid(GAttrib *attrib, uint16_t start, uint16_t end,
--
1.8.4


2014-05-28 14:44:34

by Lukasz Rymanowski

[permalink] [raw]
Subject: [PATCH 4/9] shared/crypto: Fix concatenate of sign counter into the msg

This patch fixes incorrectly concatenated sign counter to the message
---
src/shared/crypto.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/shared/crypto.c b/src/shared/crypto.c
index e46f07f..b50baa5 100644
--- a/src/shared/crypto.c
+++ b/src/shared/crypto.c
@@ -273,7 +273,7 @@ bool bt_crypto_sign_att(struct bt_crypto *crypto, const uint8_t key[16],
memcpy(msg, m, m_len);

/* Add sign_counter to the message */
- put_le32(sign_cnt, msg + msg_len);
+ put_le32(sign_cnt, msg + m_len);

/* The most significant octet of key corresponds to key[0] */
swap128(key, tmp);
--
1.8.4


2014-05-28 14:44:33

by Lukasz Rymanowski

[permalink] [raw]
Subject: [PATCH 3/9] android/gatt: Fix signed write command encoding

As per spec (BT spec 4.1 Vol[3], Part F, chapter 3.4.5.4) we need to
take opcode, handle and parameters to generate signature.
In order to support it signing is moved to att.c, place where pdu is
encoded
---
Makefile.am | 3 ++-
Makefile.tools | 4 +++-
android/gatt.c | 13 ++-----------
attrib/att.c | 11 +++++++----
attrib/att.h | 6 +++++-
attrib/gatt.c | 11 ++++++++---
attrib/gatt.h | 4 +++-
7 files changed, 30 insertions(+), 22 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 4e77019..dc88816 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -98,7 +98,8 @@ gdbus_libgdbus_internal_la_SOURCES = gdbus/gdbus.h \
attrib_sources = attrib/att.h attrib/att-database.h attrib/att.c \
attrib/gatt.h attrib/gatt.c \
attrib/gattrib.h attrib/gattrib.c \
- attrib/gatt-service.h attrib/gatt-service.c
+ attrib/gatt-service.h attrib/gatt-service.c \
+ src/shared/crypto.h src/shared/crypto.c

btio_sources = btio/btio.h btio/btio.c

diff --git a/Makefile.tools b/Makefile.tools
index 412a998..c24bdf7 100644
--- a/Makefile.tools
+++ b/Makefile.tools
@@ -356,7 +356,9 @@ attrib_gatttool_SOURCES = attrib/gatttool.c attrib/att.c attrib/gatt.c \
attrib/gattrib.c btio/btio.c \
attrib/gatttool.h attrib/interactive.c \
attrib/utils.c src/log.c client/display.c \
- client/display.h
+ client/display.h \
+ src/shared/crypto.h src/shared/crypto.c
+
attrib_gatttool_LDADD = lib/libbluetooth-internal.la @GLIB_LIBS@ -lreadline

tools_obex_client_tool_SOURCES = $(gobex_sources) $(btio_sources) \
diff --git a/android/gatt.c b/android/gatt.c
index 6638fe6..21cba75 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -44,7 +44,6 @@
#include "src/shared/util.h"
#include "src/shared/queue.h"
#include "src/shared/gatt-db.h"
-#include "src/shared/crypto.h"
#include "attrib/gattrib.h"
#include "attrib/att.h"
#include "attrib/gatt.h"
@@ -2642,7 +2641,6 @@ static void write_char_cb(guint8 status, const guint8 *pdu, guint16 len,
static bool signed_write_cmd(struct gatt_device *dev, uint16_t handle,
const uint8_t *value, uint16_t vlen)
{
- uint8_t s[ATT_SIGNATURE_LEN];
uint8_t csrk[16];
uint32_t sign_cnt;

@@ -2653,15 +2651,8 @@ static bool signed_write_cmd(struct gatt_device *dev, uint16_t handle,
return false;
}

- memset(s, 0, ATT_SIGNATURE_LEN);
-
- if (!bt_crypto_sign_att(crypto, csrk, value, vlen, sign_cnt, s)) {
- error("gatt: Could not sign att data");
- return false;
- }
-
- if (!gatt_signed_write_cmd(dev->attrib, handle, value, vlen, s, NULL,
- NULL)) {
+ if (!gatt_signed_write_cmd(dev->attrib, handle, value, vlen, crypto,
+ csrk, sign_cnt, NULL, NULL)) {
error("gatt: Could write signed cmd");
return false;
}
diff --git a/attrib/att.c b/attrib/att.c
index e7d5682..2680458 100644
--- a/attrib/att.c
+++ b/attrib/att.c
@@ -561,9 +561,10 @@ uint16_t dec_write_cmd(const uint8_t *pdu, size_t len, uint16_t *handle,
return len;
}

-uint16_t enc_signed_write_cmd(uint16_t handle,
- const uint8_t *value, size_t vlen,
- const uint8_t signature[12],
+uint16_t enc_signed_write_cmd(uint16_t handle, const uint8_t *value,
+ size_t vlen, struct bt_crypto *crypto,
+ const uint8_t csrk[16],
+ uint32_t sign_cnt,
uint8_t *pdu, size_t len)
{
const uint16_t hdr_len = sizeof(pdu[0]) + sizeof(handle);
@@ -581,7 +582,9 @@ uint16_t enc_signed_write_cmd(uint16_t handle,
if (vlen > 0)
memcpy(&pdu[hdr_len], value, vlen);

- memcpy(&pdu[hdr_len + vlen], signature, ATT_SIGNATURE_LEN);
+ if (!bt_crypto_sign_att(crypto, csrk, pdu, hdr_len + vlen, sign_cnt,
+ &pdu[hdr_len + vlen]))
+ return 0;

return min_len + vlen;
}
diff --git a/attrib/att.h b/attrib/att.h
index c92cd5d..2311aaf 100644
--- a/attrib/att.h
+++ b/attrib/att.h
@@ -22,6 +22,8 @@
*
*/

+#include "src/shared/crypto.h"
+
/* Len of signature in write signed packet */
#define ATT_SIGNATURE_LEN 12

@@ -134,7 +136,9 @@ uint16_t dec_write_cmd(const uint8_t *pdu, size_t len, uint16_t *handle,
uint8_t *value, size_t *vlen);
uint16_t enc_signed_write_cmd(uint16_t handle,
const uint8_t *value, size_t vlen,
- const uint8_t signature[12],
+ struct bt_crypto *crypto,
+ const uint8_t csrk[16],
+ uint32_t sign_cnt,
uint8_t *pdu, size_t len);
uint16_t dec_signed_write_cmd(const uint8_t *pdu, size_t len,
uint16_t *handle,
diff --git a/attrib/gatt.c b/attrib/gatt.c
index ce08003..27fb0b3 100644
--- a/attrib/gatt.c
+++ b/attrib/gatt.c
@@ -1067,7 +1067,9 @@ guint gatt_write_cmd(GAttrib *attrib, uint16_t handle, const uint8_t *value,

guint gatt_signed_write_cmd(GAttrib *attrib, uint16_t handle,
const uint8_t *value, int vlen,
- const uint8_t signature[12],
+ struct bt_crypto *crypto,
+ const uint8_t csrk[16],
+ uint32_t sign_cnt,
GDestroyNotify notify,
gpointer user_data)
{
@@ -1076,8 +1078,11 @@ guint gatt_signed_write_cmd(GAttrib *attrib, uint16_t handle,
guint16 plen;

buf = g_attrib_get_buffer(attrib, &buflen);
- plen = enc_signed_write_cmd(handle, value, vlen, signature, buf,
- buflen);
+ plen = enc_signed_write_cmd(handle, value, vlen, crypto, csrk, sign_cnt,
+ buf, buflen);
+ if (plen == 0)
+ return 0;
+
return g_attrib_send(attrib, 0, buf, plen, NULL, user_data, notify);
}

diff --git a/attrib/gatt.h b/attrib/gatt.h
index 2d869e3..f6db10f 100644
--- a/attrib/gatt.h
+++ b/attrib/gatt.h
@@ -107,7 +107,9 @@ guint gatt_write_cmd(GAttrib *attrib, uint16_t handle, const uint8_t *value,

guint gatt_signed_write_cmd(GAttrib *attrib, uint16_t handle,
const uint8_t *value, int vlen,
- const uint8_t signature[12],
+ struct bt_crypto *crypto,
+ const uint8_t csrk[16],
+ uint32_t sign_cnt,
GDestroyNotify notify,
gpointer user_data);
guint gatt_read_char_by_uuid(GAttrib *attrib, uint16_t start, uint16_t end,
--
1.8.4


2014-05-28 14:44:30

by Lukasz Rymanowski

[permalink] [raw]
Subject: [PATCH 2/9] android/gatt: Fix handling signed write command from remote

As per spec (BT spec 4.1 Vol[3], Part F, chapter 3.4.5.4) message which
should be taken to generate signature is opcode, handle and parameters
---
android/gatt.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index 2e37a02..6638fe6 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -5007,8 +5007,9 @@ static void write_signed_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
}

/* Generate signature and verify it */
- if (!bt_crypto_sign_att(crypto, csrk, value, vlen, sign_cnt,
- t)) {
+ if (!bt_crypto_sign_att(crypto, csrk, cmd,
+ cmd_len - ATT_SIGNATURE_LEN,
+ sign_cnt, t)) {
error("gatt: Error when generating att signature");
return;
}
--
1.8.4


2014-05-28 14:44:31

by Lukasz Rymanowski

[permalink] [raw]
Subject: [PATCH 2/9] android/gatt: Fix handling sign write from remote

As per spec (BT spec 4.1 Vol[3], Part F, chapter 3.4.5.4) message which
should be taken to generate signature is opcode, handle and parameters
---
android/gatt.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index 2e37a02..6638fe6 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -5007,8 +5007,9 @@ static void write_signed_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
}

/* Generate signature and verify it */
- if (!bt_crypto_sign_att(crypto, csrk, value, vlen, sign_cnt,
- t)) {
+ if (!bt_crypto_sign_att(crypto, csrk, cmd,
+ cmd_len - ATT_SIGNATURE_LEN,
+ sign_cnt, t)) {
error("gatt: Error when generating att signature");
return;
}
--
1.8.4


2014-05-28 14:44:29

by Lukasz Rymanowski

[permalink] [raw]
Subject: [PATCH 1/9] android/gatt: Verify signature counter from remote

We will receive signature counter as a part of signature in att packet.
We shall verify if signature counter has value as expected otherwise
drop the message.
---
android/gatt.c | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/android/gatt.c b/android/gatt.c
index 89aca16..2e37a02 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -4998,6 +4998,13 @@ static void write_signed_cmd_request(const uint8_t *cmd, uint16_t cmd_len,

if (len) {
uint8_t t[ATT_SIGNATURE_LEN];
+ uint32_t r_sign_cnt = get_le32(s);
+
+ if (r_sign_cnt != sign_cnt) {
+ error("gatt: sign_cnt does not match (%d!=%d)",
+ sign_cnt, r_sign_cnt);
+ return;
+ }

/* Generate signature and verify it */
if (!bt_crypto_sign_att(crypto, csrk, value, vlen, sign_cnt,
--
1.8.4