Linux-Bluetooth Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 1/4] Bluetooth: Disconnect if E0 is used for Level 4
@ 2020-05-19 20:25 Luiz Augusto von Dentz
  2020-05-19 20:25 ` [PATCH 2/4] Bluetooth: Fix assuming EIR flags can result in SSP authentication Luiz Augusto von Dentz
                   ` (3 more replies)
  0 siblings, 4 replies; 17+ messages in thread
From: Luiz Augusto von Dentz @ 2020-05-19 20:25 UTC (permalink / raw)
  To: linux-bluetooth

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

E0 is not allowed with Level 4:

BLUETOOTH CORE SPECIFICATION Version 5.2 | Vol 3, Part C page 1319:

  '128-bit equivalent strength for link and encryption keys
   required using FIPS approved algorithms (E0 not allowed,
   SAFER+ not allowed, and P-192 not allowed; encryption key
   not shortened'

SC enabled:

> HCI Event: Read Remote Extended Features (0x23) plen 13
        Status: Success (0x00)
        Handle: 256
        Page: 1/2
        Features: 0x0b 0x00 0x00 0x00 0x00 0x00 0x00 0x00
          Secure Simple Pairing (Host Support)
          LE Supported (Host)
          Secure Connections (Host Support)
> HCI Event: Encryption Change (0x08) plen 4
        Status: Success (0x00)
        Handle: 256
        Encryption: Enabled with AES-CCM (0x02)

SC disabled:

> HCI Event: Read Remote Extended Features (0x23) plen 13
        Status: Success (0x00)
        Handle: 256
        Page: 1/2
        Features: 0x03 0x00 0x00 0x00 0x00 0x00 0x00 0x00
          Secure Simple Pairing (Host Support)
          LE Supported (Host)
> HCI Event: Encryption Change (0x08) plen 4
        Status: Success (0x00)
        Handle: 256
        Encryption: Enabled with E0 (0x01)
[May 8 20:23] Bluetooth: hci0: Invalid security: expect AES but E0 was used
< HCI Command: Disconnect (0x01|0x0006) plen 3
        Handle: 256
        Reason: Authentication Failure (0x05)

Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
 net/bluetooth/hci_conn.c  | 17 +++++++++++++++++
 net/bluetooth/hci_event.c |  6 ++----
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 07c34c55fc50..0c1cae83c8dc 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -1325,6 +1325,23 @@ int hci_conn_check_link_mode(struct hci_conn *conn)
 			return 0;
 	}
 
+	 /* AES encryption is required for Level 4:
+	  *
+	  * BLUETOOTH CORE SPECIFICATION Version 5.2 | Vol 3, Part C
+	  * page 1319:
+	  *
+	  * 128-bit equivalent strength for link and encryption keys
+	  * required using FIPS approved algorithms (E0 not allowed,
+	  * SAFER+ not allowed, and P-192 not allowed; encryption key
+	  * not shortened)
+	  */
+	if (conn->sec_level == BT_SECURITY_FIPS &&
+	    !test_bit(HCI_CONN_AES_CCM, &conn->flags)) {
+		bt_dev_err(conn->hdev, "Invalid security: expect AES but E0 "
+			   "was used");
+		return 0;
+	}
+
 	if (hci_conn_ssp_enabled(conn) &&
 	    !test_bit(HCI_CONN_ENCRYPT, &conn->flags))
 		return 0;
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 006c24e04b44..dc1cc3c4348c 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -3078,10 +3078,8 @@ static void hci_encrypt_change_evt(struct hci_dev *hdev, struct sk_buff *skb)
 	 * that are not encrypted with AES-CCM using a P-256 authenticated
 	 * combination key.
 	 */
-	if (hci_dev_test_flag(hdev, HCI_SC_ONLY) &&
-	    (!test_bit(HCI_CONN_AES_CCM, &conn->flags) ||
-	     conn->key_type != HCI_LK_AUTH_COMBINATION_P256)) {
-		hci_connect_cfm(conn, HCI_ERROR_AUTH_FAILURE);
+	if (!hci_conn_check_link_mode(conn)) {
+		hci_disconnect(conn, HCI_ERROR_AUTH_FAILURE);
 		hci_conn_drop(conn);
 		goto unlock;
 	}
-- 
2.25.3


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 2/4] Bluetooth: Fix assuming EIR flags can result in SSP authentication
  2020-05-19 20:25 [PATCH 1/4] Bluetooth: Disconnect if E0 is used for Level 4 Luiz Augusto von Dentz
@ 2020-05-19 20:25 ` Luiz Augusto von Dentz
  2020-05-20 14:34   ` Marcel Holtmann
  2020-05-19 20:25 ` [PATCH 3/4] Bluetooth: Fix bogus check for re-auth no supported with non-ssp Luiz Augusto von Dentz
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 17+ messages in thread
From: Luiz Augusto von Dentz @ 2020-05-19 20:25 UTC (permalink / raw)
  To: linux-bluetooth

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

EIR flags should just hint if SSP may be supported but we shall verify
this with use of the actual features as the SSP bits may be disabled in
the lower layers which would result in legacy authentication to be
used.

Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
 net/bluetooth/hci_conn.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 0c1cae83c8dc..6b2288d97ab7 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -225,8 +225,6 @@ static void hci_acl_create_connection(struct hci_conn *conn)
 		}
 
 		memcpy(conn->dev_class, ie->data.dev_class, 3);
-		if (ie->data.ssp_mode > 0)
-			set_bit(HCI_CONN_SSP_ENABLED, &conn->flags);
 	}
 
 	cp.pkt_type = cpu_to_le16(conn->pkt_type);
-- 
2.25.3


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 3/4] Bluetooth: Fix bogus check for re-auth no supported with non-ssp
  2020-05-19 20:25 [PATCH 1/4] Bluetooth: Disconnect if E0 is used for Level 4 Luiz Augusto von Dentz
  2020-05-19 20:25 ` [PATCH 2/4] Bluetooth: Fix assuming EIR flags can result in SSP authentication Luiz Augusto von Dentz
@ 2020-05-19 20:25 ` Luiz Augusto von Dentz
  2020-05-20 14:25   ` Marcel Holtmann
  2020-05-19 20:25 ` [PATCH 4/4] Bluetooth: Consolidate encryption handling in hci_encrypt_cfm Luiz Augusto von Dentz
  2020-05-20 14:23 ` [PATCH 1/4] Bluetooth: Disconnect if E0 is used for Level 4 Marcel Holtmann
  3 siblings, 1 reply; 17+ messages in thread
From: Luiz Augusto von Dentz @ 2020-05-19 20:25 UTC (permalink / raw)
  To: linux-bluetooth

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This reverts 19f8def031bfa50c579149b200bfeeb919727b27
"Bluetooth: Fix auth_complete_evt for legacy units" which seems to be
working around a bug on a broken controller rather then any limitation
imposed by the Bluetooth spec, in fact if there ws not possible to
re-auth the command shall fail not succeed.

Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
 net/bluetooth/hci_event.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index dc1cc3c4348c..8c9051ffa665 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2869,14 +2869,8 @@ static void hci_auth_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 
 	if (!ev->status) {
 		clear_bit(HCI_CONN_AUTH_FAILURE, &conn->flags);
-
-		if (!hci_conn_ssp_enabled(conn) &&
-		    test_bit(HCI_CONN_REAUTH_PEND, &conn->flags)) {
-			bt_dev_info(hdev, "re-auth of legacy device is not possible.");
-		} else {
-			set_bit(HCI_CONN_AUTH, &conn->flags);
-			conn->sec_level = conn->pending_sec_level;
-		}
+		set_bit(HCI_CONN_AUTH, &conn->flags);
+		conn->sec_level = conn->pending_sec_level;
 	} else {
 		if (ev->status == HCI_ERROR_PIN_OR_KEY_MISSING)
 			set_bit(HCI_CONN_AUTH_FAILURE, &conn->flags);
-- 
2.25.3


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 4/4] Bluetooth: Consolidate encryption handling in hci_encrypt_cfm
  2020-05-19 20:25 [PATCH 1/4] Bluetooth: Disconnect if E0 is used for Level 4 Luiz Augusto von Dentz
  2020-05-19 20:25 ` [PATCH 2/4] Bluetooth: Fix assuming EIR flags can result in SSP authentication Luiz Augusto von Dentz
  2020-05-19 20:25 ` [PATCH 3/4] Bluetooth: Fix bogus check for re-auth no supported with non-ssp Luiz Augusto von Dentz
@ 2020-05-19 20:25 ` Luiz Augusto von Dentz
  2020-05-20 14:31   ` Marcel Holtmann
  2020-05-20 14:23 ` [PATCH 1/4] Bluetooth: Disconnect if E0 is used for Level 4 Marcel Holtmann
  3 siblings, 1 reply; 17+ messages in thread
From: Luiz Augusto von Dentz @ 2020-05-19 20:25 UTC (permalink / raw)
  To: linux-bluetooth

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This makes hci_encrypt_cfm calls hci_connect_cfm in case the connection
state is BT_CONFIG so callers don't have to check the state.

Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
 include/net/bluetooth/hci_core.h | 20 ++++++++++++++++++--
 net/bluetooth/hci_event.c        | 28 +++-------------------------
 2 files changed, 21 insertions(+), 27 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 239ab72f16c6..2fe8a5ca9a81 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1380,10 +1380,26 @@ static inline void hci_auth_cfm(struct hci_conn *conn, __u8 status)
 		conn->security_cfm_cb(conn, status);
 }
 
-static inline void hci_encrypt_cfm(struct hci_conn *conn, __u8 status,
-								__u8 encrypt)
+static inline void hci_encrypt_cfm(struct hci_conn *conn, __u8 status)
 {
 	struct hci_cb *cb;
+	__u8 encrypt;
+
+	if (conn->state == BT_CONFIG) {
+		if (status)
+			conn->state = BT_CONNECTED;
+
+		hci_connect_cfm(conn, status);
+		hci_conn_drop(conn);
+		return;
+	}
+
+	if (!test_bit(HCI_CONN_ENCRYPT, &conn->flags))
+		encrypt = 0x00;
+	else if (test_bit(HCI_CONN_AES_CCM, &conn->flags))
+		encrypt = 0x02;
+	else
+		encrypt = 0x01;
 
 	if (conn->sec_level == BT_SECURITY_SDP)
 		conn->sec_level = BT_SECURITY_LOW;
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 8c9051ffa665..34d09a084871 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2910,7 +2910,7 @@ static void hci_auth_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 				     &cp);
 		} else {
 			clear_bit(HCI_CONN_ENCRYPT_PEND, &conn->flags);
-			hci_encrypt_cfm(conn, ev->status, 0x00);
+			hci_encrypt_cfm(conn, ev->status);
 		}
 	}
 
@@ -2995,22 +2995,7 @@ static void read_enc_key_size_complete(struct hci_dev *hdev, u8 status,
 		conn->enc_key_size = rp->key_size;
 	}
 
-	if (conn->state == BT_CONFIG) {
-		conn->state = BT_CONNECTED;
-		hci_connect_cfm(conn, 0);
-		hci_conn_drop(conn);
-	} else {
-		u8 encrypt;
-
-		if (!test_bit(HCI_CONN_ENCRYPT, &conn->flags))
-			encrypt = 0x00;
-		else if (test_bit(HCI_CONN_AES_CCM, &conn->flags))
-			encrypt = 0x02;
-		else
-			encrypt = 0x01;
-
-		hci_encrypt_cfm(conn, 0, encrypt);
-	}
+	hci_encrypt_cfm(conn, 0);
 
 unlock:
 	hci_dev_unlock(hdev);
@@ -3126,14 +3111,7 @@ static void hci_encrypt_change_evt(struct hci_dev *hdev, struct sk_buff *skb)
 	}
 
 notify:
-	if (conn->state == BT_CONFIG) {
-		if (!ev->status)
-			conn->state = BT_CONNECTED;
-
-		hci_connect_cfm(conn, ev->status);
-		hci_conn_drop(conn);
-	} else
-		hci_encrypt_cfm(conn, ev->status, ev->encrypt);
+	hci_encrypt_cfm(conn, ev->status);
 
 unlock:
 	hci_dev_unlock(hdev);
-- 
2.25.3


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/4] Bluetooth: Disconnect if E0 is used for Level 4
  2020-05-19 20:25 [PATCH 1/4] Bluetooth: Disconnect if E0 is used for Level 4 Luiz Augusto von Dentz
                   ` (2 preceding siblings ...)
  2020-05-19 20:25 ` [PATCH 4/4] Bluetooth: Consolidate encryption handling in hci_encrypt_cfm Luiz Augusto von Dentz
@ 2020-05-20 14:23 ` Marcel Holtmann
  2020-05-20 16:00   ` Luiz Augusto von Dentz
  3 siblings, 1 reply; 17+ messages in thread
From: Marcel Holtmann @ 2020-05-20 14:23 UTC (permalink / raw)
  To: Luiz Augusto von Dentz; +Cc: linux-bluetooth

Hi Luiz,

> E0 is not allowed with Level 4:
> 
> BLUETOOTH CORE SPECIFICATION Version 5.2 | Vol 3, Part C page 1319:
> 
>  '128-bit equivalent strength for link and encryption keys
>   required using FIPS approved algorithms (E0 not allowed,
>   SAFER+ not allowed, and P-192 not allowed; encryption key
>   not shortened'
> 
> SC enabled:
> 
>> HCI Event: Read Remote Extended Features (0x23) plen 13
>        Status: Success (0x00)
>        Handle: 256
>        Page: 1/2
>        Features: 0x0b 0x00 0x00 0x00 0x00 0x00 0x00 0x00
>          Secure Simple Pairing (Host Support)
>          LE Supported (Host)
>          Secure Connections (Host Support)
>> HCI Event: Encryption Change (0x08) plen 4
>        Status: Success (0x00)
>        Handle: 256
>        Encryption: Enabled with AES-CCM (0x02)
> 
> SC disabled:
> 
>> HCI Event: Read Remote Extended Features (0x23) plen 13
>        Status: Success (0x00)
>        Handle: 256
>        Page: 1/2
>        Features: 0x03 0x00 0x00 0x00 0x00 0x00 0x00 0x00
>          Secure Simple Pairing (Host Support)
>          LE Supported (Host)
>> HCI Event: Encryption Change (0x08) plen 4
>        Status: Success (0x00)
>        Handle: 256
>        Encryption: Enabled with E0 (0x01)
> [May 8 20:23] Bluetooth: hci0: Invalid security: expect AES but E0 was used
> < HCI Command: Disconnect (0x01|0x0006) plen 3
>        Handle: 256
>        Reason: Authentication Failure (0x05)
> 
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
> net/bluetooth/hci_conn.c  | 17 +++++++++++++++++
> net/bluetooth/hci_event.c |  6 ++----
> 2 files changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index 07c34c55fc50..0c1cae83c8dc 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -1325,6 +1325,23 @@ int hci_conn_check_link_mode(struct hci_conn *conn)
> 			return 0;
> 	}
> 
> +	 /* AES encryption is required for Level 4:
> +	  *
> +	  * BLUETOOTH CORE SPECIFICATION Version 5.2 | Vol 3, Part C
> +	  * page 1319:
> +	  *
> +	  * 128-bit equivalent strength for link and encryption keys
> +	  * required using FIPS approved algorithms (E0 not allowed,
> +	  * SAFER+ not allowed, and P-192 not allowed; encryption key
> +	  * not shortened)
> +	  */
> +	if (conn->sec_level == BT_SECURITY_FIPS &&
> +	    !test_bit(HCI_CONN_AES_CCM, &conn->flags)) {
> +		bt_dev_err(conn->hdev, "Invalid security: expect AES but E0 "
> +			   "was used");

make this bt_dev_warn since it is something that can happen. We fail appropriately now. Also try to fit the error in a single line. “Invalid security: Missing AES-CCM usage”.

> +		return 0;
> +	}
> +
> 	if (hci_conn_ssp_enabled(conn) &&
> 	    !test_bit(HCI_CONN_ENCRYPT, &conn->flags))
> 		return 0;
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 006c24e04b44..dc1cc3c4348c 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -3078,10 +3078,8 @@ static void hci_encrypt_change_evt(struct hci_dev *hdev, struct sk_buff *skb)
> 	 * that are not encrypted with AES-CCM using a P-256 authenticated
> 	 * combination key.
> 	 */
> -	if (hci_dev_test_flag(hdev, HCI_SC_ONLY) &&
> -	    (!test_bit(HCI_CONN_AES_CCM, &conn->flags) ||
> -	     conn->key_type != HCI_LK_AUTH_COMBINATION_P256)) {
> -		hci_connect_cfm(conn, HCI_ERROR_AUTH_FAILURE);
> +	if (!hci_conn_check_link_mode(conn)) {
> +		hci_disconnect(conn, HCI_ERROR_AUTH_FAILURE);

Why is this a disconnect now? If this is better, then this needs to be explained above, but I have the feeling we really want to respond to the connect request in a clean fashion.

> 		hci_conn_drop(conn);
> 		goto unlock;

Regards

Marcel


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/4] Bluetooth: Fix bogus check for re-auth no supported with non-ssp
  2020-05-19 20:25 ` [PATCH 3/4] Bluetooth: Fix bogus check for re-auth no supported with non-ssp Luiz Augusto von Dentz
@ 2020-05-20 14:25   ` Marcel Holtmann
  2020-05-20 16:12     ` Luiz Augusto von Dentz
  0 siblings, 1 reply; 17+ messages in thread
From: Marcel Holtmann @ 2020-05-20 14:25 UTC (permalink / raw)
  To: Luiz Augusto von Dentz; +Cc: linux-bluetooth

Hi Luiz,

> This reverts 19f8def031bfa50c579149b200bfeeb919727b27
> "Bluetooth: Fix auth_complete_evt for legacy units" which seems to be
> working around a bug on a broken controller rather then any limitation
> imposed by the Bluetooth spec, in fact if there ws not possible to
> re-auth the command shall fail not succeed.
> 
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
> net/bluetooth/hci_event.c | 10 ++--------
> 1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index dc1cc3c4348c..8c9051ffa665 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -2869,14 +2869,8 @@ static void hci_auth_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
> 
> 	if (!ev->status) {
> 		clear_bit(HCI_CONN_AUTH_FAILURE, &conn->flags);
> -
> -		if (!hci_conn_ssp_enabled(conn) &&
> -		    test_bit(HCI_CONN_REAUTH_PEND, &conn->flags)) {
> -			bt_dev_info(hdev, "re-auth of legacy device is not possible.");
> -		} else {
> -			set_bit(HCI_CONN_AUTH, &conn->flags);
> -			conn->sec_level = conn->pending_sec_level;
> -		}
> +		set_bit(HCI_CONN_AUTH, &conn->flags);
> +		conn->sec_level = conn->pending_sec_level;
> 	} else {
> 		if (ev->status == HCI_ERROR_PIN_OR_KEY_MISSING)
> 			set_bit(HCI_CONN_AUTH_FAILURE, &conn->flags);

wouldn’t we now also remove HCI_CONN_REAUTH_PEND flag?

Regards

Marcel


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 4/4] Bluetooth: Consolidate encryption handling in hci_encrypt_cfm
  2020-05-19 20:25 ` [PATCH 4/4] Bluetooth: Consolidate encryption handling in hci_encrypt_cfm Luiz Augusto von Dentz
@ 2020-05-20 14:31   ` Marcel Holtmann
  0 siblings, 0 replies; 17+ messages in thread
From: Marcel Holtmann @ 2020-05-20 14:31 UTC (permalink / raw)
  To: Luiz Augusto von Dentz; +Cc: linux-bluetooth

Hi Luiz,

> This makes hci_encrypt_cfm calls hci_connect_cfm in case the connection
> state is BT_CONFIG so callers don't have to check the state.
> 
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
> include/net/bluetooth/hci_core.h | 20 ++++++++++++++++++--
> net/bluetooth/hci_event.c        | 28 +++-------------------------
> 2 files changed, 21 insertions(+), 27 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/4] Bluetooth: Fix assuming EIR flags can result in SSP authentication
  2020-05-19 20:25 ` [PATCH 2/4] Bluetooth: Fix assuming EIR flags can result in SSP authentication Luiz Augusto von Dentz
@ 2020-05-20 14:34   ` Marcel Holtmann
  2020-05-26 13:53     ` Alain Michaud
  0 siblings, 1 reply; 17+ messages in thread
From: Marcel Holtmann @ 2020-05-20 14:34 UTC (permalink / raw)
  To: Luiz Augusto von Dentz; +Cc: linux-bluetooth

Hi Luiz,

> EIR flags should just hint if SSP may be supported but we shall verify
> this with use of the actual features as the SSP bits may be disabled in
> the lower layers which would result in legacy authentication to be
> used.
> 
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
> net/bluetooth/hci_conn.c | 2 --
> 1 file changed, 2 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/4] Bluetooth: Disconnect if E0 is used for Level 4
  2020-05-20 14:23 ` [PATCH 1/4] Bluetooth: Disconnect if E0 is used for Level 4 Marcel Holtmann
@ 2020-05-20 16:00   ` Luiz Augusto von Dentz
  0 siblings, 0 replies; 17+ messages in thread
From: Luiz Augusto von Dentz @ 2020-05-20 16:00 UTC (permalink / raw)
  To: Marcel Holtmann; +Cc: linux-bluetooth

Hi Marcel,

On Wed, May 20, 2020 at 7:23 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Luiz,
>
> > E0 is not allowed with Level 4:
> >
> > BLUETOOTH CORE SPECIFICATION Version 5.2 | Vol 3, Part C page 1319:
> >
> >  '128-bit equivalent strength for link and encryption keys
> >   required using FIPS approved algorithms (E0 not allowed,
> >   SAFER+ not allowed, and P-192 not allowed; encryption key
> >   not shortened'
> >
> > SC enabled:
> >
> >> HCI Event: Read Remote Extended Features (0x23) plen 13
> >        Status: Success (0x00)
> >        Handle: 256
> >        Page: 1/2
> >        Features: 0x0b 0x00 0x00 0x00 0x00 0x00 0x00 0x00
> >          Secure Simple Pairing (Host Support)
> >          LE Supported (Host)
> >          Secure Connections (Host Support)
> >> HCI Event: Encryption Change (0x08) plen 4
> >        Status: Success (0x00)
> >        Handle: 256
> >        Encryption: Enabled with AES-CCM (0x02)
> >
> > SC disabled:
> >
> >> HCI Event: Read Remote Extended Features (0x23) plen 13
> >        Status: Success (0x00)
> >        Handle: 256
> >        Page: 1/2
> >        Features: 0x03 0x00 0x00 0x00 0x00 0x00 0x00 0x00
> >          Secure Simple Pairing (Host Support)
> >          LE Supported (Host)
> >> HCI Event: Encryption Change (0x08) plen 4
> >        Status: Success (0x00)
> >        Handle: 256
> >        Encryption: Enabled with E0 (0x01)
> > [May 8 20:23] Bluetooth: hci0: Invalid security: expect AES but E0 was used
> > < HCI Command: Disconnect (0x01|0x0006) plen 3
> >        Handle: 256
> >        Reason: Authentication Failure (0x05)
> >
> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > ---
> > net/bluetooth/hci_conn.c  | 17 +++++++++++++++++
> > net/bluetooth/hci_event.c |  6 ++----
> > 2 files changed, 19 insertions(+), 4 deletions(-)
> >
> > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> > index 07c34c55fc50..0c1cae83c8dc 100644
> > --- a/net/bluetooth/hci_conn.c
> > +++ b/net/bluetooth/hci_conn.c
> > @@ -1325,6 +1325,23 @@ int hci_conn_check_link_mode(struct hci_conn *conn)
> >                       return 0;
> >       }
> >
> > +      /* AES encryption is required for Level 4:
> > +       *
> > +       * BLUETOOTH CORE SPECIFICATION Version 5.2 | Vol 3, Part C
> > +       * page 1319:
> > +       *
> > +       * 128-bit equivalent strength for link and encryption keys
> > +       * required using FIPS approved algorithms (E0 not allowed,
> > +       * SAFER+ not allowed, and P-192 not allowed; encryption key
> > +       * not shortened)
> > +       */
> > +     if (conn->sec_level == BT_SECURITY_FIPS &&
> > +         !test_bit(HCI_CONN_AES_CCM, &conn->flags)) {
> > +             bt_dev_err(conn->hdev, "Invalid security: expect AES but E0 "
> > +                        "was used");
>
> make this bt_dev_warn since it is something that can happen. We fail appropriately now. Also try to fit the error in a single line. “Invalid security: Missing AES-CCM usage”.

Sure will update this.

> > +             return 0;
> > +     }
> > +
> >       if (hci_conn_ssp_enabled(conn) &&
> >           !test_bit(HCI_CONN_ENCRYPT, &conn->flags))
> >               return 0;
> > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > index 006c24e04b44..dc1cc3c4348c 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -3078,10 +3078,8 @@ static void hci_encrypt_change_evt(struct hci_dev *hdev, struct sk_buff *skb)
> >        * that are not encrypted with AES-CCM using a P-256 authenticated
> >        * combination key.
> >        */
> > -     if (hci_dev_test_flag(hdev, HCI_SC_ONLY) &&
> > -         (!test_bit(HCI_CONN_AES_CCM, &conn->flags) ||
> > -          conn->key_type != HCI_LK_AUTH_COMBINATION_P256)) {
> > -             hci_connect_cfm(conn, HCI_ERROR_AUTH_FAILURE);
> > +     if (!hci_conn_check_link_mode(conn)) {
> > +             hci_disconnect(conn, HCI_ERROR_AUTH_FAILURE);
>
> Why is this a disconnect now? If this is better, then this needs to be explained above, but I have the feeling we really want to respond to the connect request in a clean fashion.

While testing this I discover that calling connect_cfm doesn't
actually cause a disconnect, at least not immediately, and by
disconnecting it actually provides the right error to the remote. Note
that there is a similar usage in the if statement right above this
code, but I believe that even in case we want to cleanup the earlier
what we should be calling is hci_encrypt_cfm.

> >               hci_conn_drop(conn);
> >               goto unlock;
>
> Regards
>
> Marcel
>


-- 
Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/4] Bluetooth: Fix bogus check for re-auth no supported with non-ssp
  2020-05-20 14:25   ` Marcel Holtmann
@ 2020-05-20 16:12     ` Luiz Augusto von Dentz
  0 siblings, 0 replies; 17+ messages in thread
From: Luiz Augusto von Dentz @ 2020-05-20 16:12 UTC (permalink / raw)
  To: Marcel Holtmann; +Cc: linux-bluetooth

Hi Marcel,

On Wed, May 20, 2020 at 7:25 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Luiz,
>
> > This reverts 19f8def031bfa50c579149b200bfeeb919727b27
> > "Bluetooth: Fix auth_complete_evt for legacy units" which seems to be
> > working around a bug on a broken controller rather then any limitation
> > imposed by the Bluetooth spec, in fact if there ws not possible to
> > re-auth the command shall fail not succeed.
> >
> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > ---
> > net/bluetooth/hci_event.c | 10 ++--------
> > 1 file changed, 2 insertions(+), 8 deletions(-)
> >
> > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > index dc1cc3c4348c..8c9051ffa665 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -2869,14 +2869,8 @@ static void hci_auth_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
> >
> >       if (!ev->status) {
> >               clear_bit(HCI_CONN_AUTH_FAILURE, &conn->flags);
> > -
> > -             if (!hci_conn_ssp_enabled(conn) &&
> > -                 test_bit(HCI_CONN_REAUTH_PEND, &conn->flags)) {
> > -                     bt_dev_info(hdev, "re-auth of legacy device is not possible.");
> > -             } else {
> > -                     set_bit(HCI_CONN_AUTH, &conn->flags);
> > -                     conn->sec_level = conn->pending_sec_level;
> > -             }
> > +             set_bit(HCI_CONN_AUTH, &conn->flags);
> > +             conn->sec_level = conn->pending_sec_level;
> >       } else {
> >               if (ev->status == HCI_ERROR_PIN_OR_KEY_MISSING)
> >                       set_bit(HCI_CONN_AUTH_FAILURE, &conn->flags);
>
> wouldn’t we now also remove HCI_CONN_REAUTH_PEND flag?

Yep, it doesn't seem there is any other user of HCI_CONN_REAUTH_PEND,
not sure if we wouldn't need this in the future though if we do want
to check if reauth was triggered.

> Regards
>
> Marcel
>


-- 
Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/4] Bluetooth: Fix assuming EIR flags can result in SSP authentication
  2020-05-20 14:34   ` Marcel Holtmann
@ 2020-05-26 13:53     ` Alain Michaud
  2020-05-26 14:17       ` Marcel Holtmann
  0 siblings, 1 reply; 17+ messages in thread
From: Alain Michaud @ 2020-05-26 13:53 UTC (permalink / raw)
  To: Marcel Holtmann; +Cc: Luiz Augusto von Dentz, BlueZ

Hi Marcel and Luiz,

Starting with the 2.1 specification, it is my interpretation that it
is not valid to support EIR but not SSP.  I understand that SSP may be
disabled from BlueZ's point of view, but this doesn't seem to be a
legitimate/qualifiable configuration.  Should we instead fail the
legacy pairing if EIR was received as an invalid condition?

Thanks,
Alain

On Wed, May 20, 2020 at 10:35 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Luiz,
>
> > EIR flags should just hint if SSP may be supported but we shall verify
> > this with use of the actual features as the SSP bits may be disabled in
> > the lower layers which would result in legacy authentication to be
> > used.
> >
> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > ---
> > net/bluetooth/hci_conn.c | 2 --
> > 1 file changed, 2 deletions(-)
>
> patch has been applied to bluetooth-next tree.
>
> Regards
>
> Marcel
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/4] Bluetooth: Fix assuming EIR flags can result in SSP authentication
  2020-05-26 13:53     ` Alain Michaud
@ 2020-05-26 14:17       ` Marcel Holtmann
  2020-05-26 14:30         ` Alain Michaud
  0 siblings, 1 reply; 17+ messages in thread
From: Marcel Holtmann @ 2020-05-26 14:17 UTC (permalink / raw)
  To: Alain Michaud; +Cc: Luiz Augusto von Dentz, BlueZ

Hi Alain,

> Starting with the 2.1 specification, it is my interpretation that it
> is not valid to support EIR but not SSP.  I understand that SSP may be
> disabled from BlueZ's point of view, but this doesn't seem to be a
> legitimate/qualifiable configuration.  Should we instead fail the
> legacy pairing if EIR was received as an invalid condition?

I know that using EIR requires to also use SSP. However this is just a precaution in case the other device is an attacked and tries to trick us.

You might get an inquiry result and not extended inquiry result, but you are still talking to a SSP device. This has to do with the fact that the reception of EIR is not guaranteed. In case of radio interference you might miss one and only get an ordinary inquiry result.

If we indeed received an EIR and then get legacy pairing request, we could try to reject the pairing. However keep in mind that our inquiry cache is time limited and we through outdated information away. This might cause some race condition. So I rather read the remote host features to ensure we know the actual host features of the remote device.

Regards

Marcel


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/4] Bluetooth: Fix assuming EIR flags can result in SSP authentication
  2020-05-26 14:17       ` Marcel Holtmann
@ 2020-05-26 14:30         ` Alain Michaud
  2020-05-28  8:22           ` Marcel Holtmann
  0 siblings, 1 reply; 17+ messages in thread
From: Alain Michaud @ 2020-05-26 14:30 UTC (permalink / raw)
  To: Marcel Holtmann; +Cc: Luiz Augusto von Dentz, BlueZ

Hi Luiz,

On Tue, May 26, 2020 at 10:17 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Alain,
>
> > Starting with the 2.1 specification, it is my interpretation that it
> > is not valid to support EIR but not SSP.  I understand that SSP may be
> > disabled from BlueZ's point of view, but this doesn't seem to be a
> > legitimate/qualifiable configuration.  Should we instead fail the
> > legacy pairing if EIR was received as an invalid condition?
>
> I know that using EIR requires to also use SSP. However this is just a precaution in case the other device is an attacked and tries to trick us.
>
> You might get an inquiry result and not extended inquiry result, but you are still talking to a SSP device. This has to do with the fact that the reception of EIR is not guaranteed. In case of radio interference you might miss one and only get an ordinary inquiry result.
>
> If we indeed received an EIR and then get legacy pairing request, we could try to reject the pairing. However keep in mind that our inquiry cache is time limited and we through outdated information away. This might cause some race condition. So I rather read the remote host features to ensure we know the actual host features of the remote device.

You are correct, the EIR response is not a guaranteed thing.  For this
reason, the host should try to resolve the name of the device before
initiating bonding where a Remote Host Supported Feature Notification
Event is generated to signal the remote side's support of SSP.  As you
allude to, a remote spoofing a legitimate SSP device may always just
jam and downgrade to not SSP, but if you have any signals that SSP is
supported by the device, it may be a good defensive posture.
Receiving an EIR response or a Remote Host Supported Feature Event
with the SSP bit set is a good indication that the device supports SSP
and you should expect SSP to take place.  Again, it is not a valid
configuration to have EIR enabled but not SSP per my interpretation of
the 2.1 specification.


>
> Regards
>
> Marcel
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/4] Bluetooth: Fix assuming EIR flags can result in SSP authentication
  2020-05-26 14:30         ` Alain Michaud
@ 2020-05-28  8:22           ` Marcel Holtmann
  2020-05-28 13:17             ` Alain Michaud
  0 siblings, 1 reply; 17+ messages in thread
From: Marcel Holtmann @ 2020-05-28  8:22 UTC (permalink / raw)
  To: Alain Michaud; +Cc: Luiz Augusto von Dentz, BlueZ

Hi Alain,

>>> Starting with the 2.1 specification, it is my interpretation that it
>>> is not valid to support EIR but not SSP.  I understand that SSP may be
>>> disabled from BlueZ's point of view, but this doesn't seem to be a
>>> legitimate/qualifiable configuration.  Should we instead fail the
>>> legacy pairing if EIR was received as an invalid condition?
>> 
>> I know that using EIR requires to also use SSP. However this is just a precaution in case the other device is an attacked and tries to trick us.
>> 
>> You might get an inquiry result and not extended inquiry result, but you are still talking to a SSP device. This has to do with the fact that the reception of EIR is not guaranteed. In case of radio interference you might miss one and only get an ordinary inquiry result.
>> 
>> If we indeed received an EIR and then get legacy pairing request, we could try to reject the pairing. However keep in mind that our inquiry cache is time limited and we through outdated information away. This might cause some race condition. So I rather read the remote host features to ensure we know the actual host features of the remote device.
> 
> You are correct, the EIR response is not a guaranteed thing.  For this
> reason, the host should try to resolve the name of the device before
> initiating bonding where a Remote Host Supported Feature Notification
> Event is generated to signal the remote side's support of SSP.  As you
> allude to, a remote spoofing a legitimate SSP device may always just
> jam and downgrade to not SSP, but if you have any signals that SSP is
> supported by the device, it may be a good defensive posture.

trying to resolve the name before connected is a waste of time. Resolving the name after connecting will not give you that event. You should just read the remote features.

> Receiving an EIR response or a Remote Host Supported Feature Event
> with the SSP bit set is a good indication that the device supports SSP
> and you should expect SSP to take place.  Again, it is not a valid
> configuration to have EIR enabled but not SSP per my interpretation of
> the 2.1 specification.

If you have an idea on how to tighten this and fail, please send a patch. It is just that our inquiry cache was never designed for that. It was just to speed up the connection process.

Regards

Marcel


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/4] Bluetooth: Fix assuming EIR flags can result in SSP authentication
  2020-05-28  8:22           ` Marcel Holtmann
@ 2020-05-28 13:17             ` Alain Michaud
  2020-05-28 16:53               ` Luiz Augusto von Dentz
  0 siblings, 1 reply; 17+ messages in thread
From: Alain Michaud @ 2020-05-28 13:17 UTC (permalink / raw)
  To: Marcel Holtmann; +Cc: Luiz Augusto von Dentz, BlueZ

On Thu, May 28, 2020 at 4:22 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Alain,
>
> >>> Starting with the 2.1 specification, it is my interpretation that it
> >>> is not valid to support EIR but not SSP.  I understand that SSP may be
> >>> disabled from BlueZ's point of view, but this doesn't seem to be a
> >>> legitimate/qualifiable configuration.  Should we instead fail the
> >>> legacy pairing if EIR was received as an invalid condition?
> >>
> >> I know that using EIR requires to also use SSP. However this is just a precaution in case the other device is an attacked and tries to trick us.
> >>
> >> You might get an inquiry result and not extended inquiry result, but you are still talking to a SSP device. This has to do with the fact that the reception of EIR is not guaranteed. In case of radio interference you might miss one and only get an ordinary inquiry result.
> >>
> >> If we indeed received an EIR and then get legacy pairing request, we could try to reject the pairing. However keep in mind that our inquiry cache is time limited and we through outdated information away. This might cause some race condition. So I rather read the remote host features to ensure we know the actual host features of the remote device.
> >
> > You are correct, the EIR response is not a guaranteed thing.  For this
> > reason, the host should try to resolve the name of the device before
> > initiating bonding where a Remote Host Supported Feature Notification
> > Event is generated to signal the remote side's support of SSP.  As you
> > allude to, a remote spoofing a legitimate SSP device may always just
> > jam and downgrade to not SSP, but if you have any signals that SSP is
> > supported by the device, it may be a good defensive posture.
>
> trying to resolve the name before connected is a waste of time. Resolving the name after connecting will not give you that event. You should just read the remote features.

I have a vague memory that there was an interoperability issue around
this that required the initiator to know ahead of time if SSP was
supported by the remote host before connecting which was the reason
why this was added in the first place.  However, I agree that this can
also be read after you are connected rather than just waiting for a
RNR page to complete just to page again.  The point here however is
about the signals that SSP should be supported and the conditions
where we let legacy pairing go through.  My assertion is that EIR
implies SSP, so legacy pairing shouldn't be allowed in that case.
It's not a definitive security measure, but IMO, every signals that we
can get will help close a door to downgrade attacks.

>
> > Receiving an EIR response or a Remote Host Supported Feature Event
> > with the SSP bit set is a good indication that the device supports SSP
> > and you should expect SSP to take place.  Again, it is not a valid
> > configuration to have EIR enabled but not SSP per my interpretation of
> > the 2.1 specification.
>
> If you have an idea on how to tighten this and fail, please send a patch. It is just that our inquiry cache was never designed for that. It was just to speed up the connection process.
Ack.  This definitely looks like an opportunity.  We can add it to the backlog.

>
> Regards
>
> Marcel
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/4] Bluetooth: Fix assuming EIR flags can result in SSP authentication
  2020-05-28 13:17             ` Alain Michaud
@ 2020-05-28 16:53               ` Luiz Augusto von Dentz
  2020-05-28 17:16                 ` Alain Michaud
  0 siblings, 1 reply; 17+ messages in thread
From: Luiz Augusto von Dentz @ 2020-05-28 16:53 UTC (permalink / raw)
  To: Alain Michaud; +Cc: Marcel Holtmann, BlueZ

Hi Alain,

On Thu, May 28, 2020 at 6:18 AM Alain Michaud <alainmichaud@google.com> wrote:
>
> On Thu, May 28, 2020 at 4:22 AM Marcel Holtmann <marcel@holtmann.org> wrote:
> >
> > Hi Alain,
> >
> > >>> Starting with the 2.1 specification, it is my interpretation that it
> > >>> is not valid to support EIR but not SSP.  I understand that SSP may be
> > >>> disabled from BlueZ's point of view, but this doesn't seem to be a
> > >>> legitimate/qualifiable configuration.  Should we instead fail the
> > >>> legacy pairing if EIR was received as an invalid condition?
> > >>
> > >> I know that using EIR requires to also use SSP. However this is just a precaution in case the other device is an attacked and tries to trick us.
> > >>
> > >> You might get an inquiry result and not extended inquiry result, but you are still talking to a SSP device. This has to do with the fact that the reception of EIR is not guaranteed. In case of radio interference you might miss one and only get an ordinary inquiry result.
> > >>
> > >> If we indeed received an EIR and then get legacy pairing request, we could try to reject the pairing. However keep in mind that our inquiry cache is time limited and we through outdated information away. This might cause some race condition. So I rather read the remote host features to ensure we know the actual host features of the remote device.
> > >
> > > You are correct, the EIR response is not a guaranteed thing.  For this
> > > reason, the host should try to resolve the name of the device before
> > > initiating bonding where a Remote Host Supported Feature Notification
> > > Event is generated to signal the remote side's support of SSP.  As you
> > > allude to, a remote spoofing a legitimate SSP device may always just
> > > jam and downgrade to not SSP, but if you have any signals that SSP is
> > > supported by the device, it may be a good defensive posture.
> >
> > trying to resolve the name before connected is a waste of time. Resolving the name after connecting will not give you that event. You should just read the remote features.
>
> I have a vague memory that there was an interoperability issue around
> this that required the initiator to know ahead of time if SSP was
> supported by the remote host before connecting which was the reason
> why this was added in the first place.  However, I agree that this can
> also be read after you are connected rather than just waiting for a
> RNR page to complete just to page again.  The point here however is
> about the signals that SSP should be supported and the conditions
> where we let legacy pairing go through.  My assertion is that EIR
> implies SSP, so legacy pairing shouldn't be allowed in that case.
> It's not a definitive security measure, but IMO, every signals that we
> can get will help close a door to downgrade attacks.

Legacy pairing is still indicated with MGMT_DEV_FOUND_LEGACY_PAIRING
so for what is worth this didn't change any logic regarding how legacy
pairing is detected, in fact hci_conn_ssp_enabled is used to determine
if encryption is required or not for low security it was never used to
detect if the paring method used was really SSP or legacy.

> >
> > > Receiving an EIR response or a Remote Host Supported Feature Event
> > > with the SSP bit set is a good indication that the device supports SSP
> > > and you should expect SSP to take place.  Again, it is not a valid
> > > configuration to have EIR enabled but not SSP per my interpretation of
> > > the 2.1 specification.
> >
> > If you have an idea on how to tighten this and fail, please send a patch. It is just that our inquiry cache was never designed for that. It was just to speed up the connection process.
> Ack.  This definitely looks like an opportunity.  We can add it to the backlog.

If you guys agree I can prepare a patch requiring SSP to be used, but
first we will need to agree on what action we would take under such
situation, shall we disconnect if we figure out the SSP bit is
disabled? Btw, if the remote device has dropped SSP for some reason (I
suppose the spec allows doing it) and we have a EIR on the cache
(which we keep for 30 seconds) that doesn't necessarily mean the
device is broken since the use of EIR may have been dropped in the
meantime.

> >
> > Regards
> >
> > Marcel
> >



-- 
Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/4] Bluetooth: Fix assuming EIR flags can result in SSP authentication
  2020-05-28 16:53               ` Luiz Augusto von Dentz
@ 2020-05-28 17:16                 ` Alain Michaud
  0 siblings, 0 replies; 17+ messages in thread
From: Alain Michaud @ 2020-05-28 17:16 UTC (permalink / raw)
  To: Luiz Augusto von Dentz; +Cc: Marcel Holtmann, BlueZ

Hi Luiz,

On Thu, May 28, 2020 at 12:54 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Alain,
>
> On Thu, May 28, 2020 at 6:18 AM Alain Michaud <alainmichaud@google.com> wrote:
> >
> > On Thu, May 28, 2020 at 4:22 AM Marcel Holtmann <marcel@holtmann.org> wrote:
> > >
> > > Hi Alain,
> > >
> > > >>> Starting with the 2.1 specification, it is my interpretation that it
> > > >>> is not valid to support EIR but not SSP.  I understand that SSP may be
> > > >>> disabled from BlueZ's point of view, but this doesn't seem to be a
> > > >>> legitimate/qualifiable configuration.  Should we instead fail the
> > > >>> legacy pairing if EIR was received as an invalid condition?
> > > >>
> > > >> I know that using EIR requires to also use SSP. However this is just a precaution in case the other device is an attacked and tries to trick us.
> > > >>
> > > >> You might get an inquiry result and not extended inquiry result, but you are still talking to a SSP device. This has to do with the fact that the reception of EIR is not guaranteed. In case of radio interference you might miss one and only get an ordinary inquiry result.
> > > >>
> > > >> If we indeed received an EIR and then get legacy pairing request, we could try to reject the pairing. However keep in mind that our inquiry cache is time limited and we through outdated information away. This might cause some race condition. So I rather read the remote host features to ensure we know the actual host features of the remote device.
> > > >
> > > > You are correct, the EIR response is not a guaranteed thing.  For this
> > > > reason, the host should try to resolve the name of the device before
> > > > initiating bonding where a Remote Host Supported Feature Notification
> > > > Event is generated to signal the remote side's support of SSP.  As you
> > > > allude to, a remote spoofing a legitimate SSP device may always just
> > > > jam and downgrade to not SSP, but if you have any signals that SSP is
> > > > supported by the device, it may be a good defensive posture.
> > >
> > > trying to resolve the name before connected is a waste of time. Resolving the name after connecting will not give you that event. You should just read the remote features.
> >
> > I have a vague memory that there was an interoperability issue around
> > this that required the initiator to know ahead of time if SSP was
> > supported by the remote host before connecting which was the reason
> > why this was added in the first place.  However, I agree that this can
> > also be read after you are connected rather than just waiting for a
> > RNR page to complete just to page again.  The point here however is
> > about the signals that SSP should be supported and the conditions
> > where we let legacy pairing go through.  My assertion is that EIR
> > implies SSP, so legacy pairing shouldn't be allowed in that case.
> > It's not a definitive security measure, but IMO, every signals that we
> > can get will help close a door to downgrade attacks.
>
> Legacy pairing is still indicated with MGMT_DEV_FOUND_LEGACY_PAIRING
> so for what is worth this didn't change any logic regarding how legacy
> pairing is detected, in fact hci_conn_ssp_enabled is used to determine
> if encryption is required or not for low security it was never used to
> detect if the paring method used was really SSP or legacy.
The general idea is to both "try" to protect against a downgrade
attack but also to encrypt all channels (other than SDP) if SSP is
supported by the remote.  If the later is already handled, we just
need to make sure we use all available signals that the remote side
supports SSP.  Receiving an EIR is a valid signal, the absence of EIR
doesn't imply the remote doesn't support SSP.
>
> > >
> > > > Receiving an EIR response or a Remote Host Supported Feature Event
> > > > with the SSP bit set is a good indication that the device supports SSP
> > > > and you should expect SSP to take place.  Again, it is not a valid
> > > > configuration to have EIR enabled but not SSP per my interpretation of
> > > > the 2.1 specification.
> > >
> > > If you have an idea on how to tighten this and fail, please send a patch. It is just that our inquiry cache was never designed for that. It was just to speed up the connection process.
> > Ack.  This definitely looks like an opportunity.  We can add it to the backlog.
>
> If you guys agree I can prepare a patch requiring SSP to be used, but
> first we will need to agree on what action we would take under such
> situation, shall we disconnect if we figure out the SSP bit is
> disabled? Btw, if the remote device has dropped SSP for some reason (I
> suppose the spec allows doing it) and we have a EIR on the cache
> (which we keep for 30 seconds) that doesn't necessarily mean the
> device is broken since the use of EIR may have been dropped in the
> meantime.
Right, but if we do have the EIR signal, then we should expect SSP to
be used.  It is legitimate, in my interpretation, to fail anything
less than SSP in that case.

>
> > >
> > > Regards
> > >
> > > Marcel
> > >
>
>
>
> --
> Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, back to index

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-19 20:25 [PATCH 1/4] Bluetooth: Disconnect if E0 is used for Level 4 Luiz Augusto von Dentz
2020-05-19 20:25 ` [PATCH 2/4] Bluetooth: Fix assuming EIR flags can result in SSP authentication Luiz Augusto von Dentz
2020-05-20 14:34   ` Marcel Holtmann
2020-05-26 13:53     ` Alain Michaud
2020-05-26 14:17       ` Marcel Holtmann
2020-05-26 14:30         ` Alain Michaud
2020-05-28  8:22           ` Marcel Holtmann
2020-05-28 13:17             ` Alain Michaud
2020-05-28 16:53               ` Luiz Augusto von Dentz
2020-05-28 17:16                 ` Alain Michaud
2020-05-19 20:25 ` [PATCH 3/4] Bluetooth: Fix bogus check for re-auth no supported with non-ssp Luiz Augusto von Dentz
2020-05-20 14:25   ` Marcel Holtmann
2020-05-20 16:12     ` Luiz Augusto von Dentz
2020-05-19 20:25 ` [PATCH 4/4] Bluetooth: Consolidate encryption handling in hci_encrypt_cfm Luiz Augusto von Dentz
2020-05-20 14:31   ` Marcel Holtmann
2020-05-20 14:23 ` [PATCH 1/4] Bluetooth: Disconnect if E0 is used for Level 4 Marcel Holtmann
2020-05-20 16:00   ` Luiz Augusto von Dentz

Linux-Bluetooth Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-bluetooth/0 linux-bluetooth/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-bluetooth linux-bluetooth/ https://lore.kernel.org/linux-bluetooth \
		linux-bluetooth@vger.kernel.org
	public-inbox-index linux-bluetooth

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-bluetooth


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git