[PATCH 0/6] Bluetooth: LE Security fixes/improvements

[PATCH 0/6] Bluetooth: LE Security fixes/improvements

[johan.hedberg]

Hi,

Here's a set of patches I've ended up creating today due to various
issues found with LE security handling. Most of the issues fixed have
been in the three for a bit longer time, but the issue fixed by 3/6 was
introduced (by me) by the change to use passkey notification instead of
confirmation.

I've left the LTK re-encryption removal to be the last patch since it
can be debated whether we want it or not. I've on occasion had quite
many LTK re-encryption failures when testing against my iPhone, but
mostly it works just fine. Removing the re-encryption drops the failures
to 0. Today I kept getting local failures (Start Encrypt cmd_status)
constantly with local Intel Bluetooth hardware and a Bluetooth mouse.
Removing the re-encryption made this work again.

Johan

----------------------------------------------------------------
Johan Hedberg (6):
      Bluetooth: Fix potential NULL pointer dereference in SMP
      Bluetooth: Add missing cmd_status handler for LE_Start_Encryption
      Bluetooth: Fix SMP confirmation callback handling
      Bluetooth: Add SMP flag to track which side is the initiator
      Bluetooth: Don't try to confirm locally initiated SMP pairing
      Bluetooth: Remove LTK re-encryption procedure

 net/bluetooth/hci_event.c | 34 +++++++++++++++++++++
 net/bluetooth/smp.c       | 71
++++++++++++++++---------------------------
 net/bluetooth/smp.h       |  7 ++---
 3 files changed, 63 insertions(+), 49 deletions(-)

[PATCH 1/6] Bluetooth: Fix potential NULL pointer dereference in SMP

[johan.hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

If a sudden disconnection happens the l2cap_conn pointer may already
have been cleaned up by the time hci_conn_security gets called,
resulting in the following oops if we don't have a proper NULL check:

BUG: unable to handle kernel NULL pointer dereference at 000000c8
IP: [<c132e2ed>] smp_conn_security+0x26/0x151
*pde = 00000000
Oops: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC
CPU: 1 PID: 673 Comm: memcheck-x86-li Not tainted 3.14.0-rc2+ #437
Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
task: f0ef0520 ti: f0d6a000 task.ti: f0d6a000
EIP: 0060:[<c132e2ed>] EFLAGS: 00010246 CPU: 1
EIP is at smp_conn_security+0x26/0x151
EAX: f0ec1770 EBX: f0ec1770 ECX: 00000002 EDX: 00000002
ESI: 00000002 EDI: 00000000 EBP: f0d6bdc0 ESP: f0d6bda0
 DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
CR0: 80050033 CR2: 000000c8 CR3: 30f0f000 CR4: 00000690
Stack:
 f4f55000 00000002 f0d6bdcc c1097a2b c1319f40 f0ec1770 00000002 f0d6bdd0
 f0d6bde8 c1312a82 f0d6bdfc c1312a82 c1319f84 00000008 f4d81c20 f0e5fd86
 f0ec1770 f0d6bdfc f0d6be28 c131be3b c131bdc1 f0d25270 c131be3b 00000008
Call Trace:
 [<c1097a2b>] ? __kmalloc+0x118/0x128
 [<c1319f40>] ? mgmt_pending_add+0x49/0x9b
 [<c1312a82>] hci_conn_security+0x4a/0x1dd
 [<c1312a82>] ? hci_conn_security+0x4a/0x1dd
 [<c1319f84>] ? mgmt_pending_add+0x8d/0x9b
 [<c131be3b>] pair_device+0x1e1/0x206
 [<c131bdc1>] ? pair_device+0x167/0x206
 [<c131be3b>] ? pair_device+0x1e1/0x206
 [<c131ed44>] mgmt_control+0x275/0x2d6

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/smp.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index 8d618e4654a5..b8c31467a7ac 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -884,11 +884,17 @@ bool smp_sufficient_security(struct hci_conn *hcon, u8 sec_level)
 int smp_conn_security(struct hci_conn *hcon, __u8 sec_level)
 {
 	struct l2cap_conn *conn = hcon->l2cap_data;
-	struct smp_chan *smp = conn->smp_chan;
+	struct smp_chan *smp;
 	__u8 authreq;
 
 	BT_DBG("conn %p hcon %p level 0x%2.2x", conn, hcon, sec_level);
 
+	/* This may be NULL if there's an unexpected disconnection */
+	if (!conn)
+		return 1;
+
+	smp = conn->smp_chan;
+
 	if (!test_bit(HCI_LE_ENABLED, &hcon->hdev->dev_flags))
 		return 1;
 
-- 
1.8.5.3

[PATCH 2/6] Bluetooth: Add missing cmd_status handler for LE_Start_Encryption

[johan.hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

It is possible that the HCI_LE_Start_Encryption command fails in an
early stage and triggers a command status event with the failure code.
In such a case we need to properly notify the hci_conn object and
cleanly bring the connection down. This patch adds the missing command
status handler for this HCI command.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/hci_event.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index addc44f28c87..4ac8ee07bec3 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1743,6 +1743,36 @@ unlock:
 	hci_dev_unlock(hdev);
 }
 
+static void hci_cs_le_start_enc(struct hci_dev *hdev, u8 status)
+{
+	struct hci_cp_le_start_enc *cp;
+	struct hci_conn *conn;
+
+	BT_DBG("%s status 0x%2.2x", hdev->name, status);
+
+	if (!status)
+		return;
+
+	hci_dev_lock(hdev);
+
+	cp = hci_sent_cmd_data(hdev, HCI_OP_LE_START_ENC);
+	if (!cp)
+		goto unlock;
+
+	conn = hci_conn_hash_lookup_handle(hdev, __le16_to_cpu(cp->handle));
+	if (!conn)
+		goto unlock;
+
+	if (conn->state != BT_CONNECTED)
+		goto unlock;
+
+	hci_disconnect(conn, HCI_ERROR_AUTH_FAILURE);
+	hci_conn_drop(conn);
+
+unlock:
+	hci_dev_unlock(hdev);
+}
+
 static void hci_inquiry_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 {
 	__u8 status = *((__u8 *) skb->data);
@@ -2654,6 +2684,10 @@ static void hci_cmd_status_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		hci_cs_le_create_conn(hdev, ev->status);
 		break;
 
+	case HCI_OP_LE_START_ENC:
+		hci_cs_le_start_enc(hdev, ev->status);
+		break;
+
 	default:
 		BT_DBG("%s opcode 0x%4.4x", hdev->name, opcode);
 		break;
-- 
1.8.5.3

[PATCH 3/6] Bluetooth: Fix SMP confirmation callback handling

[johan.hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

In the case that a local pairing confirmation (JUST_CFM) has been
selected as the method we need to use the user confirm request mgmt
event for it with the confirm_hint set to 1 (to indicate confirmation
without any specific passkey value). Without this (if passkey_notify was
used) the pairing would never proceed. This patch adds the necessary
call to mgmt_user_confirm_request in this scenario.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/smp.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index b8c31467a7ac..97e95c849fff 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -422,6 +422,10 @@ static int tk_request(struct l2cap_conn *conn, u8 remote_oob, u8 auth,
 	if (method == REQ_PASSKEY)
 		ret = mgmt_user_passkey_request(hcon->hdev, &hcon->dst,
 						hcon->type, hcon->dst_type);
+	else if (method == JUST_CFM)
+		ret = mgmt_user_confirm_request(hcon->hdev, &hcon->dst,
+						hcon->type, hcon->dst_type,
+						passkey, 1);
 	else
 		ret = mgmt_user_passkey_notify(hcon->hdev, &hcon->dst,
 						hcon->type, hcon->dst_type,
-- 
1.8.5.3

[PATCH 4/6] Bluetooth: Add SMP flag to track which side is the initiator

[johan.hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

For remotely initiated just-works pairings we want to show the user a
confirmation dialog for the pairing. However, we can only know which
side was the initiator by tracking which side sends the first Security
Request or Pairing Request PDU. This patch adds a new SMP flag to
indicate whether our side was the initiator for the pairing.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/smp.c | 6 ++++++
 net/bluetooth/smp.h | 1 +
 2 files changed, 7 insertions(+)

diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index 97e95c849fff..7cb8440811e3 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -716,6 +716,8 @@ static u8 smp_cmd_pairing_req(struct l2cap_conn *conn, struct sk_buff *skb)
 	if (ret)
 		return SMP_UNSPECIFIED;
 
+	clear_bit(SMP_FLAG_INITIATOR, &smp->smp_flags);
+
 	return 0;
 }
 
@@ -859,6 +861,8 @@ static u8 smp_cmd_security_req(struct l2cap_conn *conn, struct sk_buff *skb)
 	if (test_and_set_bit(HCI_CONN_LE_SMP_PEND, &hcon->flags))
 		return 0;
 
+	clear_bit(SMP_FLAG_INITIATOR, &smp->smp_flags);
+
 	smp = smp_chan_create(conn);
 
 	skb_pull(skb, sizeof(*rp));
@@ -939,6 +943,8 @@ int smp_conn_security(struct hci_conn *hcon, __u8 sec_level)
 	}
 
 done:
+	set_bit(SMP_FLAG_INITIATOR, &smp->smp_flags);
+
 	hcon->pending_sec_level = sec_level;
 
 	return 0;
diff --git a/net/bluetooth/smp.h b/net/bluetooth/smp.h
index b6913471815a..0d536b8b3f9a 100644
--- a/net/bluetooth/smp.h
+++ b/net/bluetooth/smp.h
@@ -120,6 +120,7 @@ struct smp_cmd_security_req {
 #define SMP_FLAG_MITM_AUTH	3
 #define SMP_FLAG_LTK_ENCRYPT	4
 #define SMP_FLAG_COMPLETE	5
+#define SMP_FLAG_INITIATOR	6
 
 #define SMP_REENCRYPT_TIMEOUT	msecs_to_jiffies(500)
 
-- 
1.8.5.3

[PATCH v2 4/6] Bluetooth: Add SMP flag to track which side is the initiator

[johan.hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

For remotely initiated just-works pairings we want to show the user a
confirmation dialog for the pairing. However, we can only know which
side was the initiator by tracking which side sends the first Security
Request or Pairing Request PDU. This patch adds a new SMP flag to
indicate whether our side was the initiator for the pairing.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
v2: Fix potential NULL dereference in smp_cmd_security_req()

 net/bluetooth/smp.c | 6 ++++++
 net/bluetooth/smp.h | 1 +
 2 files changed, 7 insertions(+)

diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index 97e95c849fff..1b28f5fd798f 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -716,6 +716,8 @@ static u8 smp_cmd_pairing_req(struct l2cap_conn *conn, struct sk_buff *skb)
 	if (ret)
 		return SMP_UNSPECIFIED;
 
+	clear_bit(SMP_FLAG_INITIATOR, &smp->smp_flags);
+
 	return 0;
 }
 
@@ -871,6 +873,8 @@ static u8 smp_cmd_security_req(struct l2cap_conn *conn, struct sk_buff *skb)
 
 	smp_send_cmd(conn, SMP_CMD_PAIRING_REQ, sizeof(cp), &cp);
 
+	clear_bit(SMP_FLAG_INITIATOR, &smp->smp_flags);
+
 	return 0;
 }
 
@@ -939,6 +943,8 @@ int smp_conn_security(struct hci_conn *hcon, __u8 sec_level)
 	}
 
 done:
+	set_bit(SMP_FLAG_INITIATOR, &smp->smp_flags);
+
 	hcon->pending_sec_level = sec_level;
 
 	return 0;
diff --git a/net/bluetooth/smp.h b/net/bluetooth/smp.h
index b6913471815a..0d536b8b3f9a 100644
--- a/net/bluetooth/smp.h
+++ b/net/bluetooth/smp.h
@@ -120,6 +120,7 @@ struct smp_cmd_security_req {
 #define SMP_FLAG_MITM_AUTH	3
 #define SMP_FLAG_LTK_ENCRYPT	4
 #define SMP_FLAG_COMPLETE	5
+#define SMP_FLAG_INITIATOR	6
 
 #define SMP_REENCRYPT_TIMEOUT	msecs_to_jiffies(500)
 
-- 
1.8.5.3

[PATCH 5/6] Bluetooth: Don't try to confirm locally initiated SMP pairing

[johan.hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

In the case that the just-works model would be triggered we only want to
confirm remotely initiated pairings (i.e. those triggered by a Security
Request or Pairing Request). This patch adds the necessary check to the
tk_request function to fall back to the JUST_WORKS method in the case of
a locally initiated pairing.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/smp.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index 7cb8440811e3..2ab1b3eb2bb7 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -387,6 +387,11 @@ static int tk_request(struct l2cap_conn *conn, u8 remote_oob, u8 auth,
 	if (!(auth & SMP_AUTH_BONDING) && method == JUST_CFM)
 		method = JUST_WORKS;
 
+	/* Don't confirm locally initiated pairing attempts */
+	if (method == JUST_CFM && test_bit(SMP_FLAG_INITIATOR,
+					   &smp->smp_flags))
+		method = JUST_WORKS;
+
 	/* If Just Works, Continue with Zero TK */
 	if (method == JUST_WORKS) {
 		set_bit(SMP_FLAG_TK_VALID, &smp->smp_flags);
-- 
1.8.5.3

[PATCH 6/6] Bluetooth: Remove LTK re-encryption procedure

[johan.hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

Due to several devices being unable to handle this procedure reliably
(resulting in forced disconnections before pairing completes) it's
better to remove it altogether.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/smp.c | 48 +++++-------------------------------------------
 net/bluetooth/smp.h |  8 ++------
 2 files changed, 7 insertions(+), 49 deletions(-)

diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index 2ab1b3eb2bb7..98488be6dafa 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -556,20 +556,6 @@ error:
 	smp_failure(conn, reason);
 }
 
-static void smp_reencrypt(struct work_struct *work)
-{
-	struct smp_chan *smp = container_of(work, struct smp_chan,
-					    reencrypt.work);
-	struct l2cap_conn *conn = smp->conn;
-	struct hci_conn *hcon = conn->hcon;
-	struct smp_ltk *ltk = smp->ltk;
-
-	BT_DBG("");
-
-	hci_le_start_enc(hcon, ltk->ediv, ltk->rand, ltk->val);
-	hcon->enc_key_size = ltk->enc_size;
-}
-
 static struct smp_chan *smp_chan_create(struct l2cap_conn *conn)
 {
 	struct smp_chan *smp;
@@ -580,7 +566,6 @@ static struct smp_chan *smp_chan_create(struct l2cap_conn *conn)
 
 	INIT_WORK(&smp->confirm, confirm_work);
 	INIT_WORK(&smp->random, random_work);
-	INIT_DELAYED_WORK(&smp->reencrypt, smp_reencrypt);
 
 	smp->conn = conn;
 	conn->smp_chan = smp;
@@ -598,8 +583,6 @@ void smp_chan_destroy(struct l2cap_conn *conn)
 
 	BUG_ON(!smp);
 
-	cancel_delayed_work_sync(&smp->reencrypt);
-
 	complete = test_bit(SMP_FLAG_COMPLETE, &smp->smp_flags);
 	mgmt_smp_complete(conn->hcon, complete);
 
@@ -1276,7 +1259,6 @@ int smp_distribute_keys(struct l2cap_conn *conn)
 	struct smp_chan *smp = conn->smp_chan;
 	struct hci_conn *hcon = conn->hcon;
 	struct hci_dev *hdev = hcon->hdev;
-	bool ltk_encrypt;
 	__u8 *keydist;
 
 	BT_DBG("conn %p", conn);
@@ -1376,32 +1358,12 @@ int smp_distribute_keys(struct l2cap_conn *conn)
 	if ((smp->remote_key_dist & 0x07))
 		return 0;
 
-	/* Check if we should try to re-encrypt the link with the LTK.
-	 * SMP_FLAG_LTK_ENCRYPT flag is used to track whether we've
-	 * already tried this (in which case we shouldn't try again).
-	 *
-	 * The request will trigger an encryption key refresh event
-	 * which will cause a call to auth_cfm and eventually lead to
-	 * l2cap_core.c calling this smp_distribute_keys function again
-	 * and thereby completing the process.
-	 */
-	if (smp->ltk)
-		ltk_encrypt = !test_and_set_bit(SMP_FLAG_LTK_ENCRYPT,
-						&smp->smp_flags);
-	else
-		ltk_encrypt = false;
+	clear_bit(HCI_CONN_LE_SMP_PEND, &hcon->flags);
+	cancel_delayed_work_sync(&conn->security_timer);
+	set_bit(SMP_FLAG_COMPLETE, &smp->smp_flags);
+	smp_notify_keys(conn);
 
-	/* Re-encrypt the link with LTK if possible */
-	if (ltk_encrypt && hcon->out) {
-		queue_delayed_work(hdev->req_workqueue, &smp->reencrypt,
-				   SMP_REENCRYPT_TIMEOUT);
-	} else {
-		clear_bit(HCI_CONN_LE_SMP_PEND, &hcon->flags);
-		cancel_delayed_work_sync(&conn->security_timer);
-		set_bit(SMP_FLAG_COMPLETE, &smp->smp_flags);
-		smp_notify_keys(conn);
-		smp_chan_destroy(conn);
-	}
+	smp_chan_destroy(conn);
 
 	return 0;
 }
diff --git a/net/bluetooth/smp.h b/net/bluetooth/smp.h
index 0d536b8b3f9a..1277147a9150 100644
--- a/net/bluetooth/smp.h
+++ b/net/bluetooth/smp.h
@@ -118,11 +118,8 @@ struct smp_cmd_security_req {
 #define SMP_FLAG_TK_VALID	1
 #define SMP_FLAG_CFM_PENDING	2
 #define SMP_FLAG_MITM_AUTH	3
-#define SMP_FLAG_LTK_ENCRYPT	4
-#define SMP_FLAG_COMPLETE	5
-#define SMP_FLAG_INITIATOR	6
-
-#define SMP_REENCRYPT_TIMEOUT	msecs_to_jiffies(500)
+#define SMP_FLAG_COMPLETE	4
+#define SMP_FLAG_INITIATOR	5
 
 struct smp_chan {
 	struct l2cap_conn *conn;
@@ -145,7 +142,6 @@ struct smp_chan {
 	unsigned long	smp_flags;
 	struct work_struct confirm;
 	struct work_struct random;
-	struct delayed_work reencrypt;
 };
 
 /* SMP Commands */
-- 
1.8.5.3

Re: [PATCH 0/6] Bluetooth: LE Security fixes/improvements

[Marcel Holtmann]

Hi Johan,

> Here's a set of patches I've ended up creating today due to various
> issues found with LE security handling. Most of the issues fixed have
> been in the three for a bit longer time, but the issue fixed by 3/6 was
> introduced (by me) by the change to use passkey notification instead of
> confirmation.
> 
> I've left the LTK re-encryption removal to be the last patch since it
> can be debated whether we want it or not. I've on occasion had quite
> many LTK re-encryption failures when testing against my iPhone, but
> mostly it works just fine. Removing the re-encryption drops the failures
> to 0. Today I kept getting local failures (Start Encrypt cmd_status)
> constantly with local Intel Bluetooth hardware and a Bluetooth mouse.
> Removing the re-encryption made this work again.
> 
> Johan
> 
> ----------------------------------------------------------------
> Johan Hedberg (6):
>      Bluetooth: Fix potential NULL pointer dereference in SMP
>      Bluetooth: Add missing cmd_status handler for LE_Start_Encryption
>      Bluetooth: Fix SMP confirmation callback handling
>      Bluetooth: Add SMP flag to track which side is the initiator
>      Bluetooth: Don't try to confirm locally initiated SMP pairing
>      Bluetooth: Remove LTK re-encryption procedure
> 
> net/bluetooth/hci_event.c | 34 +++++++++++++++++++++
> net/bluetooth/smp.c       | 71
> ++++++++++++++++---------------------------
> net/bluetooth/smp.h       |  7 ++---
> 3 files changed, 63 insertions(+), 49 deletions(-)

all 6 patches have been applied to bluetooth-next tree.

Regards

Marcel