[RFC v2 0/2] SSP MITM protection for General Bonding

[RFC v2 0/2] SSP MITM protection for General Bonding

[Mikel Astiz]

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

I'm reworking Timo's patch originally submitted as "[RFC] Bluetooth: Fix missing MITM protection when being responding LM".

The proposal is to use MITM protection for General Bonding exactly as for Dedicated Bonding (patch 2/2). I can't think of any reason why the current implementation has different policies, leading to not using MITM protection for General Bonding even if the I/O capabilities make it possible.

The proposal is therefore to make both bonding types equal.

Another alternative would be to toggle this behavior through a mgmt API setting.

Mikel Astiz (1):
  Bluetooth: Use defines instead of integer literals

Timo Mueller (1):
  Bluetooth: Use MITM protection when responding LM

 net/bluetooth/hci_event.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

-- 
1.8.1.4

[RFC v2 1/2] Bluetooth: Use defines instead of integer literals

[Mikel Astiz]

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

Make the code in hci_get_auth_req() more readable by using the
defined macros instead of inlining magic numbers.

Signed-off-by: Timo Mueller <timo.mueller@bmw-carit.de>
Signed-off-by: Mikel Astiz <mikel.astiz@bmw-carit.de>
---
 net/bluetooth/hci_event.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 0437200..777a040 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -31,6 +31,7 @@
 #include <net/bluetooth/mgmt.h>
 #include <net/bluetooth/a2mp.h>
 #include <net/bluetooth/amp.h>
+#include <net/bluetooth/smp.h>
 
 /* Handle HCI Event packets */
 
@@ -3024,17 +3025,18 @@ unlock:
 static u8 hci_get_auth_req(struct hci_conn *conn)
 {
 	/* If remote requests dedicated bonding follow that lead */
-	if (conn->remote_auth == 0x02 || conn->remote_auth == 0x03) {
+	if ((conn->remote_auth & ~0x01) == HCI_AT_DEDICATED_BONDING) {
 		/* If both remote and local IO capabilities allow MITM
 		 * protection then require it, otherwise don't */
-		if (conn->remote_cap == 0x03 || conn->io_capability == 0x03)
+		if (conn->remote_cap == SMP_IO_NO_INPUT_OUTPUT ||
+		    conn->io_capability == SMP_IO_NO_INPUT_OUTPUT)
 			return 0x02;
 		else
 			return 0x03;
 	}
 
 	/* If remote requests no-bonding follow that lead */
-	if (conn->remote_auth == 0x00 || conn->remote_auth == 0x01)
+	if ((conn->remote_auth & ~0x01) == HCI_AT_NO_BONDING)
 		return conn->remote_auth | (conn->auth_type & 0x01);
 
 	return conn->auth_type;
-- 
1.8.1.4

Re: [RFC v2 1/2] Bluetooth: Use defines instead of integer literals

[Johan Hedberg]

Hi Mikel,

On Thu, May 30, 2013, Mikel Astiz wrote:
> Make the code in hci_get_auth_req() more readable by using the
> defined macros instead of inlining magic numbers.
> 
> Signed-off-by: Timo Mueller <timo.mueller@bmw-carit.de>
> Signed-off-by: Mikel Astiz <mikel.astiz@bmw-carit.de>
> ---
>  net/bluetooth/hci_event.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 0437200..777a040 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -31,6 +31,7 @@
>  #include <net/bluetooth/mgmt.h>
>  #include <net/bluetooth/a2mp.h>
>  #include <net/bluetooth/amp.h>
> +#include <net/bluetooth/smp.h>
>  
>  /* Handle HCI Event packets */
>  
> @@ -3024,17 +3025,18 @@ unlock:
>  static u8 hci_get_auth_req(struct hci_conn *conn)
>  {
>  	/* If remote requests dedicated bonding follow that lead */
> -	if (conn->remote_auth == 0x02 || conn->remote_auth == 0x03) {
> +	if ((conn->remote_auth & ~0x01) == HCI_AT_DEDICATED_BONDING) {
>  		/* If both remote and local IO capabilities allow MITM
>  		 * protection then require it, otherwise don't */
> -		if (conn->remote_cap == 0x03 || conn->io_capability == 0x03)
> +		if (conn->remote_cap == SMP_IO_NO_INPUT_OUTPUT ||
> +		    conn->io_capability == SMP_IO_NO_INPUT_OUTPUT)

Why are you using SMP_* defines here and not HCI_*? If the HCI ones are
missing in some header file please feel free to add them.

Johan

[RFC v2 2/2] Bluetooth: Use MITM protection when responding LM

[Mikel Astiz]

From: Timo Mueller <timo.mueller@bmw-carit.de>

A MITM protected SSP associaton model can be used for pairing if both
local and remote IO capabilities are set to something other than
NoInputNoOutput, regardless of the bonding type (dedicated or general).

With these IO capabilities a MITM protected SSP association model has
been used by the Kernel if we are initiating the pairing process
(initiating LM).

When responding to a pairing request (remote device is the initiating
LM) the pairing should also be proteced against MITM attacks, as
proposed in this patch.

Signed-off-by: Timo Mueller <timo.mueller@bmw-carit.de>
Signed-off-by: Mikel Astiz <mikel.astiz@bmw-carit.de>
---
 net/bluetooth/hci_event.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 777a040..ca59623 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -3024,22 +3024,19 @@ unlock:
 
 static u8 hci_get_auth_req(struct hci_conn *conn)
 {
-	/* If remote requests dedicated bonding follow that lead */
-	if ((conn->remote_auth & ~0x01) == HCI_AT_DEDICATED_BONDING) {
-		/* If both remote and local IO capabilities allow MITM
-		 * protection then require it, otherwise don't */
-		if (conn->remote_cap == SMP_IO_NO_INPUT_OUTPUT ||
-		    conn->io_capability == SMP_IO_NO_INPUT_OUTPUT)
-			return 0x02;
-		else
-			return 0x03;
-	}
-
 	/* If remote requests no-bonding follow that lead */
 	if ((conn->remote_auth & ~0x01) == HCI_AT_NO_BONDING)
 		return conn->remote_auth | (conn->auth_type & 0x01);
 
-	return conn->auth_type;
+	/* If both remote and local IO capabilities allow MITM protection
+	 * then require it
+	 */
+	if (conn->remote_cap != SMP_IO_NO_INPUT_OUTPUT &&
+	    conn->io_capability != SMP_IO_NO_INPUT_OUTPUT)
+		return conn->remote_auth | 0x01;
+
+	/* No MITM protection possible due to lacking capabilities */
+	return conn->remote_auth & ~0x01;
 }
 
 static void hci_io_capa_request_evt(struct hci_dev *hdev, struct sk_buff *skb)
-- 
1.8.1.4

Re: [RFC v2 2/2] Bluetooth: Use MITM protection when responding LM

[Johan Hedberg]

Hi,

On Thu, May 30, 2013, Mikel Astiz wrote:
> A MITM protected SSP associaton model can be used for pairing if both
> local and remote IO capabilities are set to something other than
> NoInputNoOutput, regardless of the bonding type (dedicated or general).
> 
> With these IO capabilities a MITM protected SSP association model has
> been used by the Kernel if we are initiating the pairing process
> (initiating LM).
> 
> When responding to a pairing request (remote device is the initiating
> LM) the pairing should also be proteced against MITM attacks, as
> proposed in this patch.
> 
> Signed-off-by: Timo Mueller <timo.mueller@bmw-carit.de>
> Signed-off-by: Mikel Astiz <mikel.astiz@bmw-carit.de>
> ---
>  net/bluetooth/hci_event.c | 21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 777a040..ca59623 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -3024,22 +3024,19 @@ unlock:
>  
>  static u8 hci_get_auth_req(struct hci_conn *conn)
>  {
> -	/* If remote requests dedicated bonding follow that lead */
> -	if ((conn->remote_auth & ~0x01) == HCI_AT_DEDICATED_BONDING) {
> -		/* If both remote and local IO capabilities allow MITM
> -		 * protection then require it, otherwise don't */
> -		if (conn->remote_cap == SMP_IO_NO_INPUT_OUTPUT ||
> -		    conn->io_capability == SMP_IO_NO_INPUT_OUTPUT)
> -			return 0x02;
> -		else
> -			return 0x03;
> -	}
> -
>  	/* If remote requests no-bonding follow that lead */
>  	if ((conn->remote_auth & ~0x01) == HCI_AT_NO_BONDING)
>  		return conn->remote_auth | (conn->auth_type & 0x01);
>  
> -	return conn->auth_type;
> +	/* If both remote and local IO capabilities allow MITM protection
> +	 * then require it
> +	 */
> +	if (conn->remote_cap != SMP_IO_NO_INPUT_OUTPUT &&
> +	    conn->io_capability != SMP_IO_NO_INPUT_OUTPUT)
> +		return conn->remote_auth | 0x01;
> +
> +	/* No MITM protection possible due to lacking capabilities */
> +	return conn->remote_auth & ~0x01;
>  }
>  
>  static void hci_io_capa_request_evt(struct hci_dev *hdev, struct sk_buff *skb)

Did you test this with acting as pairing initiator too? What worries me
a bit is the partial removal of reliance on conn->auth_type. It'd also
be important to verify that this doesn't cause issues with GAP test
cases using the BITE.

Johan

Re: [RFC v2 2/2] Bluetooth: Use MITM protection when responding LM

[Mikel Astiz]

Hi Johan,

On Thu, Jun 13, 2013 at 10:32 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi,
>
> On Thu, May 30, 2013, Mikel Astiz wrote:
>> A MITM protected SSP associaton model can be used for pairing if both
>> local and remote IO capabilities are set to something other than
>> NoInputNoOutput, regardless of the bonding type (dedicated or general).
>>
>> With these IO capabilities a MITM protected SSP association model has
>> been used by the Kernel if we are initiating the pairing process
>> (initiating LM).
>>
>> When responding to a pairing request (remote device is the initiating
>> LM) the pairing should also be proteced against MITM attacks, as
>> proposed in this patch.
>>
>> Signed-off-by: Timo Mueller <timo.mueller@bmw-carit.de>
>> Signed-off-by: Mikel Astiz <mikel.astiz@bmw-carit.de>
>> ---
>>  net/bluetooth/hci_event.c | 21 +++++++++------------
>>  1 file changed, 9 insertions(+), 12 deletions(-)
>>
>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>> index 777a040..ca59623 100644
>> --- a/net/bluetooth/hci_event.c
>> +++ b/net/bluetooth/hci_event.c
>> @@ -3024,22 +3024,19 @@ unlock:
>>
>>  static u8 hci_get_auth_req(struct hci_conn *conn)
>>  {
>> -     /* If remote requests dedicated bonding follow that lead */
>> -     if ((conn->remote_auth & ~0x01) == HCI_AT_DEDICATED_BONDING) {
>> -             /* If both remote and local IO capabilities allow MITM
>> -              * protection then require it, otherwise don't */
>> -             if (conn->remote_cap == SMP_IO_NO_INPUT_OUTPUT ||
>> -                 conn->io_capability == SMP_IO_NO_INPUT_OUTPUT)
>> -                     return 0x02;
>> -             else
>> -                     return 0x03;
>> -     }
>> -
>>       /* If remote requests no-bonding follow that lead */
>>       if ((conn->remote_auth & ~0x01) == HCI_AT_NO_BONDING)
>>               return conn->remote_auth | (conn->auth_type & 0x01);
>>
>> -     return conn->auth_type;
>> +     /* If both remote and local IO capabilities allow MITM protection
>> +      * then require it
>> +      */
>> +     if (conn->remote_cap != SMP_IO_NO_INPUT_OUTPUT &&
>> +         conn->io_capability != SMP_IO_NO_INPUT_OUTPUT)
>> +             return conn->remote_auth | 0x01;
>> +
>> +     /* No MITM protection possible due to lacking capabilities */
>> +     return conn->remote_auth & ~0x01;
>>  }
>>
>>  static void hci_io_capa_request_evt(struct hci_dev *hdev, struct sk_buff *skb)
>
> Did you test this with acting as pairing initiator too? What worries me
> a bit is the partial removal of reliance on conn->auth_type. It'd also
> be important to verify that this doesn't cause issues with GAP test
> cases using the BITE.

We can double check all the tests we can think of, but unfortunately
we have no access to the BITE.

Regarding the reliance on conn->auth_type, I believe it boils down to
whether the same policy (i.e. use MITM protection when capabilities
allow it) should also be applied to outgoing general bondings. This
patch would affect both incoming and outgoing pairing procedures.

IMO it doesn't make much sense to introduce such a policy for incoming
ones only, based on the intuition that two BlueZ ends would always use
MITM protection as a consequence of the responding end enforcing it.

This seems to suggest a management socket command to enforce MITM
protection and otherwise leave the current behavior. Any thoughts on
this?

As a reminder, the motivation behind this change is that IVI use-cases
require disallowing just-works pairing. MITM protection should always
be used and the pairing will fail (rejected in userland) if the IO
capabilities don't allow it.

Cheers,
Mikel

Re: [RFC v2 0/2] SSP MITM protection for General Bonding

[Mikel Astiz]

Hi,

On Thu, May 30, 2013 at 11:26 AM, Mikel Astiz <mikel.astiz.oss@gmail.com> wrote:
> From: Mikel Astiz <mikel.astiz@bmw-carit.de>
>
> I'm reworking Timo's patch originally submitted as "[RFC] Bluetooth: Fix missing MITM protection when being responding LM".
>
> The proposal is to use MITM protection for General Bonding exactly as for Dedicated Bonding (patch 2/2). I can't think of any reason why the current implementation has different policies, leading to not using MITM protection for General Bonding even if the I/O capabilities make it possible.
>
> The proposal is therefore to make both bonding types equal.
>
> Another alternative would be to toggle this behavior through a mgmt API setting.
>
> Mikel Astiz (1):
>   Bluetooth: Use defines instead of integer literals
>
> Timo Mueller (1):
>   Bluetooth: Use MITM protection when responding LM
>
>  net/bluetooth/hci_event.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
>
> --
> 1.8.1.4
>

Ping.