[PATCH v3 1/2] Bluetooth: Fix auth_complete_evt for legacy units

[PATCH v3 1/2] Bluetooth: Fix auth_complete_evt for legacy units

[Waldemar Rymarkiewicz]

Legacy devices don't re-authenticate the link properly if a link key
already exists.  Thus, don't update sec_level for this case even if
hci_auth_complete_evt indicates success. Otherwise the sec_level will
not reflect a real security on the link.

Signed-off-by: Waldemar Rymarkiewicz <waldemar.rymarkiewicz@tieto.com>
---
 include/net/bluetooth/hci_core.h |    1 +
 net/bluetooth/hci_conn.c         |    2 ++
 net/bluetooth/hci_event.c        |   12 ++++++++++--
 3 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index af4b0ed..0ac820d 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -322,6 +322,7 @@ void hci_inquiry_cache_update(struct hci_dev *hdev, struct inquiry_data *data);
 /* ----- HCI Connections ----- */
 enum {
 	HCI_CONN_AUTH_PEND,
+	HCI_CONN_REAUTH_PEND,
 	HCI_CONN_ENCRYPT_PEND,
 	HCI_CONN_RSWITCH_PEND,
 	HCI_CONN_MODE_CHANGE_PEND,
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 3163330..e675402 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -548,6 +548,8 @@ static int hci_conn_auth(struct hci_conn *conn, __u8 sec_level, __u8 auth_type)
 		cp.handle = cpu_to_le16(conn->handle);
 		hci_send_cmd(conn->hdev, HCI_OP_AUTH_REQUESTED,
 							sizeof(cp), &cp);
+		if (conn->key_type != 0xff)
+			set_bit(HCI_CONN_REAUTH_PEND, &conn->pend);
 	}
 
 	return 0;
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index c456f9c..82061db 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1491,13 +1491,21 @@ 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) {
-			conn->link_mode |= HCI_LM_AUTH;
-			conn->sec_level = conn->pending_sec_level;
+			if (!(conn->ssp_mode > 0 && hdev->ssp_mode > 0) &&
+						test_bit(HCI_CONN_REAUTH_PEND,
+						&conn->pend)) {
+				BT_INFO("re-auth of legacy device is not"
+								"possible.");
+			} else {
+				conn->link_mode |= HCI_LM_AUTH;
+				conn->sec_level = conn->pending_sec_level;
+			}
 		} else {
 			mgmt_auth_failed(hdev->id, &conn->dst, ev->status);
 		}
 
 		clear_bit(HCI_CONN_AUTH_PEND, &conn->pend);
+		clear_bit(HCI_CONN_REAUTH_PEND, &conn->pend);
 
 		if (conn->state == BT_CONFIG) {
 			if (!ev->status && hdev->ssp_mode > 0 &&
-- 
1.7.4.1

[PATCH v3 2/2] Bluetooth: Refactor hci_auth_complete_evt function

[Waldemar Rymarkiewicz]

Replace if(conn) with if(!conn) checking to avoid too many nested statements

Signed-off-by: Waldemar Rymarkiewicz <waldemar.rymarkiewicz@tieto.com>
---
 net/bluetooth/hci_event.c |   83 ++++++++++++++++++++++-----------------------
 1 files changed, 41 insertions(+), 42 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 82061db..0776c1d 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1489,59 +1489,58 @@ static inline void hci_auth_complete_evt(struct hci_dev *hdev, struct sk_buff *s
 	hci_dev_lock(hdev);
 
 	conn = hci_conn_hash_lookup_handle(hdev, __le16_to_cpu(ev->handle));
-	if (conn) {
-		if (!ev->status) {
-			if (!(conn->ssp_mode > 0 && hdev->ssp_mode > 0) &&
-						test_bit(HCI_CONN_REAUTH_PEND,
-						&conn->pend)) {
-				BT_INFO("re-auth of legacy device is not"
-								"possible.");
-			} else {
-				conn->link_mode |= HCI_LM_AUTH;
-				conn->sec_level = conn->pending_sec_level;
-			}
+	if (!conn)
+		goto unlock;
+
+	if (!ev->status) {
+		if (!(conn->ssp_mode > 0 && hdev->ssp_mode > 0) &&
+				test_bit(HCI_CONN_REAUTH_PEND,	&conn->pend)) {
+			BT_INFO("re-auth of legacy device is not possible.");
 		} else {
-			mgmt_auth_failed(hdev->id, &conn->dst, ev->status);
+			conn->link_mode |= HCI_LM_AUTH;
+			conn->sec_level = conn->pending_sec_level;
 		}
+	} else {
+		mgmt_auth_failed(hdev->id, &conn->dst, ev->status);
+	}
 
-		clear_bit(HCI_CONN_AUTH_PEND, &conn->pend);
-		clear_bit(HCI_CONN_REAUTH_PEND, &conn->pend);
+	clear_bit(HCI_CONN_AUTH_PEND, &conn->pend);
+	clear_bit(HCI_CONN_REAUTH_PEND, &conn->pend);
 
-		if (conn->state == BT_CONFIG) {
-			if (!ev->status && hdev->ssp_mode > 0 &&
-							conn->ssp_mode > 0) {
-				struct hci_cp_set_conn_encrypt cp;
-				cp.handle  = ev->handle;
-				cp.encrypt = 0x01;
-				hci_send_cmd(hdev, HCI_OP_SET_CONN_ENCRYPT,
-							sizeof(cp), &cp);
-			} else {
-				conn->state = BT_CONNECTED;
-				hci_proto_connect_cfm(conn, ev->status);
-				hci_conn_put(conn);
-			}
+	if (conn->state == BT_CONFIG) {
+		if (!ev->status && hdev->ssp_mode > 0 && conn->ssp_mode > 0) {
+			struct hci_cp_set_conn_encrypt cp;
+			cp.handle  = ev->handle;
+			cp.encrypt = 0x01;
+			hci_send_cmd(hdev, HCI_OP_SET_CONN_ENCRYPT, sizeof(cp),
+									&cp);
 		} else {
-			hci_auth_cfm(conn, ev->status);
-
-			hci_conn_hold(conn);
-			conn->disc_timeout = HCI_DISCONN_TIMEOUT;
+			conn->state = BT_CONNECTED;
+			hci_proto_connect_cfm(conn, ev->status);
 			hci_conn_put(conn);
 		}
+	} else {
+		hci_auth_cfm(conn, ev->status);
 
-		if (test_bit(HCI_CONN_ENCRYPT_PEND, &conn->pend)) {
-			if (!ev->status) {
-				struct hci_cp_set_conn_encrypt cp;
-				cp.handle  = ev->handle;
-				cp.encrypt = 0x01;
-				hci_send_cmd(hdev, HCI_OP_SET_CONN_ENCRYPT,
-							sizeof(cp), &cp);
-			} else {
-				clear_bit(HCI_CONN_ENCRYPT_PEND, &conn->pend);
-				hci_encrypt_cfm(conn, ev->status, 0x00);
-			}
+		hci_conn_hold(conn);
+		conn->disc_timeout = HCI_DISCONN_TIMEOUT;
+		hci_conn_put(conn);
+	}
+
+	if (test_bit(HCI_CONN_ENCRYPT_PEND, &conn->pend)) {
+		if (!ev->status) {
+			struct hci_cp_set_conn_encrypt cp;
+			cp.handle  = ev->handle;
+			cp.encrypt = 0x01;
+			hci_send_cmd(hdev, HCI_OP_SET_CONN_ENCRYPT, sizeof(cp),
+									&cp);
+		} else {
+			clear_bit(HCI_CONN_ENCRYPT_PEND, &conn->pend);
+			hci_encrypt_cfm(conn, ev->status, 0x00);
 		}
 	}
 
+unlock:
 	hci_dev_unlock(hdev);
 }
 
-- 
1.7.4.1

Re: [PATCH v3 2/2] Bluetooth: Refactor hci_auth_complete_evt function

[Gustavo F. Padovan]

Hi Waldemar,

* Waldemar Rymarkiewicz <waldemar.rymarkiewicz@tieto.com> [2011-05-31 15:49:26 +0200]:

> Replace if(conn) with if(!conn) checking to avoid too many nested statements
> 
> Signed-off-by: Waldemar Rymarkiewicz <waldemar.rymarkiewicz@tieto.com>
> ---
>  net/bluetooth/hci_event.c |   83 ++++++++++++++++++++++-----------------------
>  1 files changed, 41 insertions(+), 42 deletions(-)

I applied both patches, thanks.

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

Re: [PATCH v3 1/2] Bluetooth: Fix auth_complete_evt for legacy units

[Peter Hurley]

Hi Waldemar,

On Tue, 2011-05-31 at 09:49 -0400, Waldemar Rymarkiewicz wrote: 
> Legacy devices don't re-authenticate the link properly if a link key
> already exists.  Thus, don't update sec_level for this case even if
> hci_auth_complete_evt indicates success. Otherwise the sec_level will
> not reflect a real security on the link.
> 
> Signed-off-by: Waldemar Rymarkiewicz <waldemar.rymarkiewicz@tieto.com>

Can you clarify what you mean by "Legacy devices don't re-authenticate
the link properly if a link key already exists"?

Other than a controller defect, I'm having trouble understanding how an
authentication could be reported as successful by the host controller
if the remote fails to provide an acceptable LMP_sres pdu. Or did the
link manager not send an LMP_au_rand or was a new AU_RAND not issued? 

The reason this came up is because I'm testing a patch set that performs
automatic re-authentication tied to l2cap channel sec_level promotion,
via sockopts, and I'm not seeing problems with this reverted.

> ---
>  include/net/bluetooth/hci_core.h |    1 +
>  net/bluetooth/hci_conn.c         |    2 ++
>  net/bluetooth/hci_event.c        |   12 ++++++++++--
>  3 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index af4b0ed..0ac820d 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -322,6 +322,7 @@ void hci_inquiry_cache_update(struct hci_dev *hdev, struct inquiry_data *data);
>  /* ----- HCI Connections ----- */
>  enum {
>  	HCI_CONN_AUTH_PEND,
> +	HCI_CONN_REAUTH_PEND,
>  	HCI_CONN_ENCRYPT_PEND,
>  	HCI_CONN_RSWITCH_PEND,
>  	HCI_CONN_MODE_CHANGE_PEND,
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index 3163330..e675402 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -548,6 +548,8 @@ static int hci_conn_auth(struct hci_conn *conn, __u8 sec_level, __u8 auth_type)
>  		cp.handle = cpu_to_le16(conn->handle);
>  		hci_send_cmd(conn->hdev, HCI_OP_AUTH_REQUESTED,
>  							sizeof(cp), &cp);
> +		if (conn->key_type != 0xff)
> +			set_bit(HCI_CONN_REAUTH_PEND, &conn->pend);

Also, can you help me understand why the AUTH_REQUESTED cmd is sent
anyway and a pend status used for indication (as opposed to testing
for the legacy device in hci_conn_auth and returning early).

>  	}
>  
>  	return 0;
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index c456f9c..82061db 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -1491,13 +1491,21 @@ 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) {
> -			conn->link_mode |= HCI_LM_AUTH;
> -			conn->sec_level = conn->pending_sec_level;
> +			if (!(conn->ssp_mode > 0 && hdev->ssp_mode > 0) &&
> +						test_bit(HCI_CONN_REAUTH_PEND,
> +						&conn->pend)) {
> +				BT_INFO("re-auth of legacy device is not"
> +								"possible.");
> +			} else {
> +				conn->link_mode |= HCI_LM_AUTH;
> +				conn->sec_level = conn->pending_sec_level;
> +			}
>  		} else {
>  			mgmt_auth_failed(hdev->id, &conn->dst, ev->status);
>  		}
>  
>  		clear_bit(HCI_CONN_AUTH_PEND, &conn->pend);
> +		clear_bit(HCI_CONN_REAUTH_PEND, &conn->pend);
>  
>  		if (conn->state == BT_CONFIG) {
>  			if (!ev->status && hdev->ssp_mode > 0 &&

Regards,
Peter Hurley

RE: [PATCH v3 1/2] Bluetooth: Fix auth_complete_evt for legacy units

[Waldemar.Rymarkiewicz]

Hi Peter,

>> Legacy devices don't re-authenticate the link properly if a link key
>> already exists.  Thus, don't update sec_level for this case even if
>> hci_auth_complete_evt indicates success. Otherwise the sec_level will
>> not reflect a real security on the link.

>Can you clarify what you mean by "Legacy devices don't re-authenticate
>the link properly if a link key already exists"?

I don't have a BT spec behind me now, but the point is that the legacy devices will not initiate new paring if the link key already exists.
In another words, if the remote device has already been authenticated (e.g. with 4 digit pin), but the service requires higher security (use of 16 digit pin) the whole auth procedure will not start i.e. link_key_request ... etc. Instead, the controller returns auth_req with success immediately.

>Other than a controller defect, I'm having trouble understanding how an
>authentication could be reported as successful by the host controller
>if the remote fails to provide an acceptable LMP_sres pdu. Or did the
>link manager not send an LMP_au_rand or was a new AU_RAND not issued?

The controllers tested by me behaved this way. I didn't look into LMP stuff.

>The reason this came up is because I'm testing a patch set that performs
>automatic re-authentication tied to l2cap channel sec_level promotion,
>via sockopts, and I'm not seeing problems with this reverted.

>Also, can you help me understand why the AUTH_REQUESTED cmd is sent
>anyway and a pend status used for indication (as opposed to testing
>for the legacy device in hci_conn_auth and returning early)

If you mean HCI_CONN_REAUTH_PEND that was used as indication in order to update sec_level correctly.

I guess there was a wider discussion on the list on this topic, check this out as well.

Thanks,
/Waldek

RE: [PATCH v3 1/2] Bluetooth: Fix auth_complete_evt for legacy units

[Peter Hurley]

Hi Waldemar,

On Fri, 2011-09-09 at 02:42 -0400, Waldemar.Rymarkiewicz@tieto.com
wrote:
> Hi Peter,
> 
> >> Legacy devices don't re-authenticate the link properly if a link key
> >> already exists.  Thus, don't update sec_level for this case even if
> >> hci_auth_complete_evt indicates success. Otherwise the sec_level will
> >> not reflect a real security on the link.
> 
> >Can you clarify what you mean by "Legacy devices don't re-authenticate
> >the link properly if a link key already exists"?
> 
> I don't have a BT spec behind me now, but the point is that the legacy devices will not initiate new paring if the link key already exists.
> In another words, if the remote device has already been authenticated (e.g. with 4 digit pin), but the service requires higher security (use of 16 digit pin) the whole auth procedure will not start i.e. link_key_request ... etc. Instead, the controller returns auth_req with success immediately.

This behavior is unique to the controllers you tested. The Core 4.0 spec
(Vol 2, Part E - HCI Functional Spec, Section 7.1.15 Authentication
Requested Command) has this to say regarding this controller behavior:

"On receipt of an Authentication Requested Command, a local BR/EDR
Controller that is not in Simple Pairing Mode may use an existing stored
link key and respond immediately to the Host with an Authentication
Complete event.

This behavior is not recommended for controllers as it offers the Host
no method for enhancing the security of an existing link (e.g., in the
case where a profile mandating a minimum passkey length is started over
a link that is already authenticated with shorter passkey than the new
service requires)."

Here's a sequence from a controller that does not have this undesirable
behavior:

> 2011-08-08 12:43:18.558584 < HCI Command: Accept Connection Request (0x01|0x0009) plen 7
>     bdaddr 00:0D:FD:1E:99:30 role 0x00
>     Role: Master
> 2011-08-08 12:43:18.561558 > HCI Event: Command Status (0x0f) plen 4
>     Accept Connection Request (0x01|0x0009) status 0x00 ncmd 1
> 2011-08-08 12:43:18.737647 > HCI Event: Role Change (0x12) plen 8
>     status 0x00 bdaddr 00:0D:FD:1E:99:30 role 0x00
>     Role: Master
> 2011-08-08 12:43:18.876717 > HCI Event: Link Key Request (0x17) plen 6
>     bdaddr 00:0D:FD:1E:99:30
> 2011-08-08 12:43:18.876824 < HCI Command: Link Key Request Reply (0x01|0x000b) plen 22
>     bdaddr 00:0D:FD:1E:99:30 key AF20110EE1D32E2C27821EC3719FE7FF
> 2011-08-08 12:43:18.956757 > HCI Event: Command Complete (0x0e) plen 10
>     Link Key Request Reply (0x01|0x000b) ncmd 1
>     status 0x00 bdaddr 00:0D:FD:1E:99:30
> 2011-08-08 12:43:19.143850 > HCI Event: Connect Complete (0x03) plen 11
>     status 0x00 handle 13 bdaddr 00:0D:FD:1E:99:30 type ACL encrypt 0x01
> 
> Legacy security-level 3 remote device that creates an encrypted
> connection.
> 
> ... < snip > ... Incoming RFCOMM connection
> 
> 2011-08-08 12:43:19.510035 > ACL data: handle 13 flags 0x02 dlen 12
>     L2CAP(s): Connect req: psm 3 scid 0x0041
> 2011-08-08 12:43:19.510051 < ACL data: handle 13 flags 0x00 dlen 16
>     L2CAP(s): Connect rsp: dcid 0x0040 scid 0x0041 result 0 status 0
>       Connection successful
> 
> ... < snip > ... Re-auth & re-encrypt (sec_level of RFCOMM dlc was medium)
> 
> 2011-08-08 12:43:19.677119 > ACL data: handle 13 flags 0x02 dlen 8
>     L2CAP(d): cid 0x0040 len 4 [psm 3]
>       RFCOMM(s): SABM: cr 1 dlci 26 pf 1 ilen 0 fcs 0xe7 
> 2011-08-08 12:43:19.677144 < HCI Command: Authentication Requested (0x01|0x0011) plen 2
>     handle 13
> 2011-08-08 12:43:19.679118 > HCI Event: Command Status (0x0f) plen 4
>     Authentication Requested (0x01|0x0011) status 0x00 ncmd 1
> 2011-08-08 12:43:19.754156 > HCI Event: Link Key Request (0x17) plen 6
>     bdaddr 00:0D:FD:1E:99:30
> 2011-08-08 12:43:19.754234 < HCI Command: Link Key Request Reply (0x01|0x000b) plen 22
>     bdaddr 00:0D:FD:1E:99:30 key AF20110EE1D32E2C27821EC3719FE7FF
> 2011-08-08 12:43:19.836196 > HCI Event: Command Complete (0x0e) plen 10
>     Link Key Request Reply (0x01|0x000b) ncmd 1
>     status 0x00 bdaddr 00:0D:FD:1E:99:30
> 2011-08-08 12:43:19.837197 > HCI Event: Auth Complete (0x06) plen 3
>     status 0x00 handle 13
> 2011-08-08 12:43:19.837207 < HCI Command: Set Connection Encryption (0x01|0x0013) plen 3
>     handle 13 encrypt 0x01
> 2011-08-08 12:43:19.839198 > HCI Event: Number of Completed Packets (0x13) plen 5
>     handle 13 packets 1
> 2011-08-08 12:43:19.841197 > HCI Event: Command Status (0x0f) plen 4
>     Set Connection Encryption (0x01|0x0013) status 0x00 ncmd 1
> 2011-08-08 12:43:19.843199 > HCI Event: Encrypt Change (0x08) plen 4
>     status 0x00 handle 13 encrypt 0x01

As you can see here, this controller correctly issues a Link Key Request
event as a result of receiving an Authentication Requested command (thus
giving the host stack the opportunity to re-pair at a higher security
level).

Unfortunately, your patch disables sec_level promotion of legacy devices
for *all* controllers.

Regards,
Peter Hurley