From: Mikel Astiz <[email protected]>
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
From: Timo Mueller <[email protected]>
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 <[email protected]>
Signed-off-by: Mikel Astiz <[email protected]>
---
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
From: Mikel Astiz <[email protected]>
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 <[email protected]>
Signed-off-by: Mikel Astiz <[email protected]>
---
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
Hi Johan,
On Thu, Jun 13, 2013 at 10:32 AM, Johan Hedberg <[email protected]> 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 <[email protected]>
>> Signed-off-by: Mikel Astiz <[email protected]>
>> ---
>> 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
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 <[email protected]>
> Signed-off-by: Mikel Astiz <[email protected]>
> ---
> 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
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 <[email protected]>
> Signed-off-by: Mikel Astiz <[email protected]>
> ---
> 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
Hi,
On Thu, May 30, 2013 at 11:26 AM, Mikel Astiz <[email protected]> wrote:
> From: Mikel Astiz <[email protected]>
>
> 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.