Received-SPF: pass (google.com: best guess record for domain of linux-bluetooth-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 27 Mar 2020 20:18:59 +0800
Message-Id: <20200327201817.Bluez.v1.1.If919a39697a6506be273f879d752bd506e63b45b@changeid>
Mime-Version: 1.0
Subject: [Bluez PATCH v1] shared/att: Check the signature of att packets
From:   Archie Pusaka <apusaka@google.com>
To:     linux-bluetooth <linux-bluetooth@vger.kernel.org>,
        Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc:     Archie Pusaka <apusaka@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

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
---

 src/shared/att.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/src/shared/att.c b/src/shared/att.c
index 948a5548b..0faac4d1d 100644
--- a/src/shared/att.c
+++ b/src/shared/att.c
@@ -886,6 +886,8 @@ static bool handle_signed(struct bt_att *att, uint8_t opcode, uint8_t *pdu,
 {
 	uint8_t *signature;
 	uint32_t sign_cnt;
+	uint8_t *copy_pdu = NULL;
+	uint8_t *generated_signature;
 	struct sign_info *sign;
 
 	/* Check if there is enough data for a signature */
@@ -903,15 +905,29 @@ 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))
+	/* Generate signature */
+	copy_pdu = malloc(pdu_len + 1);
+	if (!copy_pdu)
 		goto fail;
 
+	copy_pdu[0] = opcode;
+	memcpy(copy_pdu + 1, pdu, pdu_len - BT_ATT_SIGNATURE_LEN);
+	generated_signature = copy_pdu + pdu_len - BT_ATT_SIGNATURE_LEN + 1;
+
+	if (!bt_crypto_sign_att(att->crypto, sign->key, copy_pdu,
+				pdu_len - BT_ATT_SIGNATURE_LEN + 1, sign_cnt,
+				generated_signature))
+		goto fail;
+
+	/* Verify received signature */
+	if (memcmp(generated_signature, signature, BT_ATT_SIGNATURE_LEN))
+		goto fail;
+
+	free(copy_pdu);
 	return true;
 
 fail:
+	free(copy_pdu);
 	util_debug(att->debug_callback, att->debug_data,
 			"ATT failed to verify signature: 0x%02x", opcode);
 
@@ -925,7 +941,7 @@ static void handle_notify(struct bt_att_chan *chan, uint8_t opcode,
 	const struct queue_entry *entry;
 	bool found;
 
-	if ((opcode & ATT_OP_SIGNED_MASK) && !att->crypto) {
+	if ((opcode & ATT_OP_SIGNED_MASK) && att->crypto) {
 		if (!handle_signed(att, opcode, pdu, pdu_len))
 			return;
 		pdu_len -= BT_ATT_SIGNATURE_LEN;
-- 
2.25.1.696.g5e7596f4ac-goog