[PATCH 0/5] Fixes for authentication and security level issues

[PATCH 0/5] Fixes for authentication and security level issues

[johan.hedberg]

Hi,

Here's a set of patches that we created based on a recent test-run with
the BITE tester. They should be fine, but we're still re-running all GAP
and L2CAP tests during today to see that there aren't any regressions.

Johan

[PATCH 1/5] Revert "Bluetooth: Update sec_level/auth_type for already existing connections"

[johan.hedberg]

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

This reverts commit 045309820afe047920a50de25634dab46a1e851d. That
commit is wrong for two reasons:

- The conn->sec_level should be updated without performing
authentication first (as it's supposed to represent the level of
security that the existing connection has)

- A higher auth_type value doesn't mean "more secure" like the commit
seems to assume. E.g. dedicated bonding with MITM protection is 0x03
whereas general bonding without MITM protection is 0x04. hci_conn_auth
already takes care of updating conn->auth_type so hci_connect doesn't
need to do it.

Signed-off-by: Johan Hedberg <johan.hedberg@nokia.com>
---
 net/bluetooth/hci_conn.c |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 6b90a41..65a3fb5 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -382,11 +382,6 @@ struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst, __u8
 		acl->sec_level = sec_level;
 		acl->auth_type = auth_type;
 		hci_acl_connect(acl);
-	} else {
-		if (acl->sec_level < sec_level)
-			acl->sec_level = sec_level;
-		if (acl->auth_type < auth_type)
-			acl->auth_type = auth_type;
 	}
 
 	if (type == ACL_LINK)
-- 
1.7.2.3

Re: [PATCH 1/5] Revert "Bluetooth: Update sec_level/auth_type for already existing connections"

[Johan Hedberg]

Hi Gustavo,

On Wed, Jan 19, 2011, johan.hedberg@gmail.com wrote:
> - The conn->sec_level should be updated without performing

Stupid typo: s/should/shouldn't/

Do you want me to resend or will you fix it manually yourself (e.g. git
commit --amend)?

Johan

[PATCH 2/5] Bluetooth: Fix MITM protection requirement preservation

[johan.hedberg]

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

If an existing connection has a MITM protection requirement (the first
bit of the auth_type) then that requirement should not be cleared by new
sockets that reuse the ACL but don't have that requirement.

Signed-off-by: Johan Hedberg <johan.hedberg@nokia.com>
---
 net/bluetooth/hci_conn.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 65a3fb5..fe712a8 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -442,6 +442,9 @@ static int hci_conn_auth(struct hci_conn *conn, __u8 sec_level, __u8 auth_type)
 	else if (conn->link_mode & HCI_LM_AUTH)
 		return 1;
 
+	/* Make sure we preserve an existing MITM requirement*/
+	auth_type |= (conn->auth_type & 0x01);
+
 	conn->auth_type = auth_type;
 
 	if (!test_and_set_bit(HCI_CONN_AUTH_PEND, &conn->pend)) {
-- 
1.7.2.3

[PATCH 3/5] Bluetooth: Create a unified auth_type evaluation function

[johan.hedberg]

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

The logic for determining the needed auth_type for an L2CAP socket is
rather complicated and has so far been duplicated in
l2cap_check_security as well as l2cap_do_connect. Additionally the
l2cap_check_security code was completely missing the handling of
SOCK_RAW type sockets. This patch creates a unified function for the
evaluation and makes l2cap_do_connect and l2cap_check_security use that
function.

Signed-off-by: Johan Hedberg <johan.hedberg@nokia.com>
---
 net/bluetooth/l2cap.c |   77 ++++++++++++++++++-------------------------------
 1 files changed, 28 insertions(+), 49 deletions(-)

diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index 4fd88eb..ae227bf 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -305,33 +305,44 @@ static void l2cap_chan_del(struct sock *sk, int err)
 	}
 }
 
-/* Service level security */
-static inline int l2cap_check_security(struct sock *sk)
+static inline u8 l2cap_get_auth_type(struct sock *sk)
 {
-	struct l2cap_conn *conn = l2cap_pi(sk)->conn;
-	__u8 auth_type;
+	if (sk->sk_type == SOCK_RAW) {
+		switch (l2cap_pi(sk)->sec_level) {
+		case BT_SECURITY_HIGH:
+			return HCI_AT_DEDICATED_BONDING_MITM;
+		case BT_SECURITY_MEDIUM:
+			return HCI_AT_DEDICATED_BONDING;
+		default:
+			return HCI_AT_NO_BONDING;
+		}
+	} else if (l2cap_pi(sk)->psm == cpu_to_le16(0x0001)) {
+		if (l2cap_pi(sk)->sec_level == BT_SECURITY_LOW)
+			l2cap_pi(sk)->sec_level = BT_SECURITY_SDP;
 
-	if (l2cap_pi(sk)->psm == cpu_to_le16(0x0001)) {
 		if (l2cap_pi(sk)->sec_level == BT_SECURITY_HIGH)
-			auth_type = HCI_AT_NO_BONDING_MITM;
+			return HCI_AT_NO_BONDING_MITM;
 		else
-			auth_type = HCI_AT_NO_BONDING;
-
-		if (l2cap_pi(sk)->sec_level == BT_SECURITY_LOW)
-			l2cap_pi(sk)->sec_level = BT_SECURITY_SDP;
+			return HCI_AT_NO_BONDING;
 	} else {
 		switch (l2cap_pi(sk)->sec_level) {
 		case BT_SECURITY_HIGH:
-			auth_type = HCI_AT_GENERAL_BONDING_MITM;
-			break;
+			return HCI_AT_GENERAL_BONDING_MITM;
 		case BT_SECURITY_MEDIUM:
-			auth_type = HCI_AT_GENERAL_BONDING;
-			break;
+			return HCI_AT_GENERAL_BONDING;
 		default:
-			auth_type = HCI_AT_NO_BONDING;
-			break;
+			return HCI_AT_NO_BONDING;
 		}
 	}
+}
+
+/* Service level security */
+static inline int l2cap_check_security(struct sock *sk)
+{
+	struct l2cap_conn *conn = l2cap_pi(sk)->conn;
+	__u8 auth_type;
+
+	auth_type = l2cap_get_auth_type(sk);
 
 	return hci_conn_security(conn->hcon, l2cap_pi(sk)->sec_level,
 								auth_type);
@@ -1068,39 +1079,7 @@ static int l2cap_do_connect(struct sock *sk)
 
 	err = -ENOMEM;
 
-	if (sk->sk_type == SOCK_RAW) {
-		switch (l2cap_pi(sk)->sec_level) {
-		case BT_SECURITY_HIGH:
-			auth_type = HCI_AT_DEDICATED_BONDING_MITM;
-			break;
-		case BT_SECURITY_MEDIUM:
-			auth_type = HCI_AT_DEDICATED_BONDING;
-			break;
-		default:
-			auth_type = HCI_AT_NO_BONDING;
-			break;
-		}
-	} else if (l2cap_pi(sk)->psm == cpu_to_le16(0x0001)) {
-		if (l2cap_pi(sk)->sec_level == BT_SECURITY_HIGH)
-			auth_type = HCI_AT_NO_BONDING_MITM;
-		else
-			auth_type = HCI_AT_NO_BONDING;
-
-		if (l2cap_pi(sk)->sec_level == BT_SECURITY_LOW)
-			l2cap_pi(sk)->sec_level = BT_SECURITY_SDP;
-	} else {
-		switch (l2cap_pi(sk)->sec_level) {
-		case BT_SECURITY_HIGH:
-			auth_type = HCI_AT_GENERAL_BONDING_MITM;
-			break;
-		case BT_SECURITY_MEDIUM:
-			auth_type = HCI_AT_GENERAL_BONDING;
-			break;
-		default:
-			auth_type = HCI_AT_NO_BONDING;
-			break;
-		}
-	}
+	auth_type = l2cap_get_auth_type(sk);
 
 	hcon = hci_connect(hdev, ACL_LINK, dst,
 					l2cap_pi(sk)->sec_level, auth_type);
-- 
1.7.2.3

[PATCH 4/5] Bluetooth: Fix authentication request for L2CAP raw sockets

[johan.hedberg]

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

When there is an existing connection l2cap_check_security needs to be
called to ensure that the security level of the new socket is fulfilled.
Normally l2cap_do_start takes care of this, but that function doesn't
get called for SOCK_RAW type sockets. This patch adds the necessary
l2cap_check_security call to the appropriate branch in l2cap_do_connect.

Signed-off-by: Johan Hedberg <johan.hedberg@nokia.com>
---
 net/bluetooth/l2cap.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index ae227bf..7550abb 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -1106,7 +1106,8 @@ static int l2cap_do_connect(struct sock *sk)
 		if (sk->sk_type != SOCK_SEQPACKET &&
 				sk->sk_type != SOCK_STREAM) {
 			l2cap_sock_clear_timer(sk);
-			sk->sk_state = BT_CONNECTED;
+			if (l2cap_check_security(sk))
+				sk->sk_state = BT_CONNECTED;
 		} else
 			l2cap_do_start(sk);
 	}
-- 
1.7.2.3

Re: [PATCH 4/5] Bluetooth: Fix authentication request for L2CAP raw sockets

[Gustavo F. Padovan]

Hi Johan,

* johan.hedberg@gmail.com <johan.hedberg@gmail.com> [2011-01-19 12:06:51 +0530]:

> From: Johan Hedberg <johan.hedberg@nokia.com>
> 
> When there is an existing connection l2cap_check_security needs to be
> called to ensure that the security level of the new socket is fulfilled.
> Normally l2cap_do_start takes care of this, but that function doesn't
> get called for SOCK_RAW type sockets. This patch adds the necessary
> l2cap_check_security call to the appropriate branch in l2cap_do_connect.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@nokia.com>

I applied patches 1 to 4. Patch 5 doesn't apply to bluetooth-2.6. Please
rebase it. Thanks.

-- 
Gustavo F. Padovan
http://profusion.mobi

[PATCH 5/5] Bluetooth: Fix race condition with conn->sec_level

[johan.hedberg]

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

The conn->sec_level value is supposed to represent the current level of
security that the connection has. However, by assigning to it before
requesting authentication it will have the wrong value during the
authentication procedure. To fix this a pending_sec_level variable is
added which is used to track the desired security level while making
sure that sec_level always represents the current level of security.

Signed-off-by: Johan Hedberg <johan.hedberg@nokia.com>
---
 include/net/bluetooth/hci_core.h |    1 +
 net/bluetooth/hci_conn.c         |    8 ++++++--
 net/bluetooth/hci_event.c        |    9 +++++----
 3 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index a29feb0..d2cf884 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -184,6 +184,7 @@ struct hci_conn {
 	__u32		 link_mode;
 	__u8             auth_type;
 	__u8             sec_level;
+	__u8		 pending_sec_level;
 	__u8             power_save;
 	__u16            disc_timeout;
 	unsigned long	 pend;
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index fe712a8..99cd8d9 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -379,7 +379,8 @@ struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst, __u8
 	hci_conn_hold(acl);
 
 	if (acl->state == BT_OPEN || acl->state == BT_CLOSED) {
-		acl->sec_level = sec_level;
+		acl->sec_level = BT_SECURITY_LOW;
+		acl->pending_sec_level = sec_level;
 		acl->auth_type = auth_type;
 		hci_acl_connect(acl);
 	}
@@ -437,8 +438,11 @@ static int hci_conn_auth(struct hci_conn *conn, __u8 sec_level, __u8 auth_type)
 {
 	BT_DBG("conn %p", conn);
 
+	if (conn->pending_sec_level > sec_level)
+		sec_level = conn->pending_sec_level;
+
 	if (sec_level > conn->sec_level)
-		conn->sec_level = sec_level;
+		conn->pending_sec_level = sec_level;
 	else if (conn->link_mode & HCI_LM_AUTH)
 		return 1;
 
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 3810017..a290854 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -692,13 +692,13 @@ static int hci_outgoing_auth_needed(struct hci_dev *hdev,
 	if (conn->state != BT_CONFIG || !conn->out)
 		return 0;
 
-	if (conn->sec_level == BT_SECURITY_SDP)
+	if (conn->pending_sec_level == BT_SECURITY_SDP)
 		return 0;
 
 	/* Only request authentication for SSP connections or non-SSP
 	 * devices with sec_level HIGH */
 	if (!(hdev->ssp_mode > 0 && conn->ssp_mode > 0) &&
-					conn->sec_level != BT_SECURITY_HIGH)
+				conn->pending_sec_level != BT_SECURITY_HIGH)
 		return 0;
 
 	return 1;
@@ -1095,9 +1095,10 @@ static inline void hci_auth_complete_evt(struct hci_dev *hdev, struct sk_buff *s
 
 	conn = hci_conn_hash_lookup_handle(hdev, __le16_to_cpu(ev->handle));
 	if (conn) {
-		if (!ev->status)
+		if (!ev->status) {
 			conn->link_mode |= HCI_LM_AUTH;
-		else
+			conn->sec_level = conn->pending_sec_level;
+		} else
 			conn->sec_level = BT_SECURITY_LOW;
 
 		clear_bit(HCI_CONN_AUTH_PEND, &conn->pend);
-- 
1.7.2.3

Re: [PATCH 5/5] Bluetooth: Fix race condition with conn->sec_level

[Gustavo F. Padovan]

Hi Johan,

* johan.hedberg@gmail.com <johan.hedberg@gmail.com> [2011-01-19 12:06:52 +0530]:

> From: Johan Hedberg <johan.hedberg@nokia.com>
> 
> The conn->sec_level value is supposed to represent the current level of
> security that the connection has. However, by assigning to it before
> requesting authentication it will have the wrong value during the
> authentication procedure. To fix this a pending_sec_level variable is
> added which is used to track the desired security level while making
> sure that sec_level always represents the current level of security.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@nokia.com>
> ---
>  include/net/bluetooth/hci_core.h |    1 +
>  net/bluetooth/hci_conn.c         |    8 ++++++--
>  net/bluetooth/hci_event.c        |    9 +++++----
>  3 files changed, 12 insertions(+), 6 deletions(-)

I rebased my tree and now this patches applies. It is there now. Thanks.

-- 
Gustavo F. Padovan
http://profusion.mobi

Re: [PATCH 0/5] Fixes for authentication and security level issues

[Marcel Holtmann]

Hi Johan,

> Here's a set of patches that we created based on a recent test-run with
> the BITE tester. They should be fine, but we're still re-running all GAP
> and L2CAP tests during today to see that there aren't any regressions.

besides the typo, I can't spot anything wrong with these patches. So
depending on that these pass BITE testing.

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel