[PATCH 0/2] Add SMP MITM hooks into MGMTOPS

[PATCH 0/2] Add SMP MITM hooks into MGMTOPS

[Brian Gix]

This includes:
1. Addition of BR/EDR vs LE breakout in user_confirm_reply
	1.1 Adds SMP placeholder
2. Addition of user_passkey_reply, with both BR/EDR (SSP) and LE (SMP) handling
	2.1 Adds approriate SSP HCI opcodes
	2.2 Adds appropriate MGMT opcodes
	2.3 Adds SMP placeholder

-- 
Brian Gix
bgix@codeaurora.org
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum 

[PATCH 1/2] Bluetooth: Add SMP support to user_confirm_reply

[Brian Gix]

to enable User Confirmation during LE-SMP pairing.

Signed-off-by: Brian Gix <bgix@codeaurora.org>
---
 net/bluetooth/mgmt.c |   22 +++++++++++++++++++---
 1 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index a6720c6..6c35f8d 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1423,6 +1423,7 @@ static int user_confirm_reply(struct sock *sk, u16 index, unsigned char *data,
 	u16 mgmt_op, hci_op;
 	struct pending_cmd *cmd;
 	struct hci_dev *hdev;
+	struct hci_conn *conn;
 	int err;
 
 	BT_DBG("");
@@ -1446,20 +1447,35 @@ static int user_confirm_reply(struct sock *sk, u16 index, unsigned char *data,
 
 	if (!test_bit(HCI_UP, &hdev->flags)) {
 		err = cmd_status(sk, index, mgmt_op, ENETDOWN);
-		goto failed;
+		goto done;
+	}
+
+	conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &cp->bdaddr);
+	if (!conn) {
+		conn = hci_conn_hash_lookup_ba(hdev, LE_LINK, &cp->bdaddr);
+		if (!conn) {
+			err = cmd_status(sk, index, mgmt_op, ENOTCONN);
+			goto done;
+		}
+
+		/* Forward Confirm response to SMP */
+
+		err = cmd_status(sk, index, mgmt_op, 0);
+		goto done;
+
 	}
 
 	cmd = mgmt_pending_add(sk, mgmt_op, hdev, data, len);
 	if (!cmd) {
 		err = -ENOMEM;
-		goto failed;
+		goto done;
 	}
 
 	err = hci_send_cmd(hdev, hci_op, sizeof(cp->bdaddr), &cp->bdaddr);
 	if (err < 0)
 		mgmt_pending_remove(cmd);
 
-failed:
+done:
 	hci_dev_unlock_bh(hdev);
 	hci_dev_put(hdev);
 
-- 
1.7.7.2
-- 
Brian Gix
bgix@codeaurora.org
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum 

[PATCH 2/2] Bluetooth: Add Passkey for SSP & SMP pairing

[Brian Gix]

Some Man-In-The-Middle (MITM) protection schemes require
User Passkey Entry.

Signed-off-by: Brian Gix <bgix@codeaurora.org>
---
 include/net/bluetooth/hci.h  |    8 ++++
 include/net/bluetooth/mgmt.h |   12 ++++++
 net/bluetooth/mgmt.c         |   77 +++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 96 insertions(+), 1 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 139ce2a..ac107b5 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -453,6 +453,14 @@ struct hci_rp_user_confirm_reply {
 
 #define HCI_OP_USER_CONFIRM_NEG_REPLY	0x042d
 
+#define HCI_OP_USER_PASSKEY_REPLY		0x042e
+struct hci_cp_user_passkey_reply {
+	bdaddr_t bdaddr;
+	__u32	passkey;
+} __packed;
+
+#define HCI_OP_USER_PASSKEY_NEG_REPLY	0x042f
+
 #define HCI_OP_REMOTE_OOB_DATA_REPLY	0x0430
 struct hci_cp_remote_oob_data_reply {
 	bdaddr_t bdaddr;
diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index 3e320c9..aa56bd5 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -228,6 +228,18 @@ struct mgmt_cp_set_fast_connectable {
 	__u8 enable;
 } __packed;
 
+#define MGMT_OP_USER_PASSKEY_REPLY	0x0020
+struct mgmt_cp_user_passkey_reply {
+	bdaddr_t bdaddr;
+	__le32 passkey;
+} __packed;
+
+#define MGMT_OP_USER_PASSKEY_NEG_REPLY	0x0021
+struct mgmt_cp_user_passkey_neg_reply {
+	bdaddr_t bdaddr;
+} __packed;
+
+
 #define MGMT_EV_CMD_COMPLETE		0x0001
 struct mgmt_ev_cmd_complete {
 	__le16 opcode;
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 6c35f8d..c6e1ad4 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1462,7 +1462,6 @@ static int user_confirm_reply(struct sock *sk, u16 index, unsigned char *data,
 
 		err = cmd_status(sk, index, mgmt_op, 0);
 		goto done;
-
 	}
 
 	cmd = mgmt_pending_add(sk, mgmt_op, hdev, data, len);
@@ -1482,6 +1481,76 @@ done:
 	return err;
 }
 
+static int user_passkey_reply(struct sock *sk, u16 index, unsigned char *data,
+							u16 len, int success)
+{
+	struct mgmt_cp_user_passkey_reply *cp = (void *) data;
+	u16 mgmt_op, hci_op;
+	u32 passkey = 0;
+	struct pending_cmd *cmd;
+	struct hci_dev *hdev;
+	struct hci_conn *conn;
+	int expected_len, err = 0;
+
+	BT_DBG("");
+
+	if (success) {
+		mgmt_op = MGMT_OP_USER_PASSKEY_REPLY;
+		hci_op = HCI_OP_USER_PASSKEY_REPLY;
+		expected_len = sizeof(*cp);
+		passkey = le32_to_cpu(cp->passkey);
+	} else {
+		mgmt_op = MGMT_OP_USER_PASSKEY_NEG_REPLY;
+		hci_op = HCI_OP_USER_PASSKEY_NEG_REPLY;
+		expected_len = sizeof(struct mgmt_cp_user_passkey_neg_reply);
+	}
+
+	if (len != expected_len)
+		return cmd_status(sk, index, mgmt_op, EINVAL);
+
+	hdev = hci_dev_get(index);
+	if (!hdev)
+		return cmd_status(sk, index, mgmt_op, ENODEV);
+
+	hci_dev_lock_bh(hdev);
+
+	if (!test_bit(HCI_UP, &hdev->flags)) {
+		err = cmd_status(sk, index, mgmt_op, ENETDOWN);
+		goto done;
+	}
+
+	conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &cp->bdaddr);
+	if (!conn) {
+		conn = hci_conn_hash_lookup_ba(hdev, LE_LINK, &cp->bdaddr);
+		if (!conn) {
+			err = cmd_status(sk, index, mgmt_op, ENOTCONN);
+			goto done;
+		}
+
+		/* Forward Passkey response to SMP */
+
+		err = cmd_status(sk, index, mgmt_op, 0);
+		goto done;
+	}
+
+	cmd = mgmt_pending_add(sk, mgmt_op, hdev, data, len);
+	if (!cmd) {
+		err = -ENOMEM;
+		goto done;
+	}
+
+	err = hci_send_cmd(hdev, hci_op, len, cp);
+
+	if (err < 0)
+		mgmt_pending_remove(cmd);
+
+done:
+	hci_dev_unlock_bh(hdev);
+	hci_dev_put(hdev);
+
+	return err;
+}
+
 static int set_local_name(struct sock *sk, u16 index, unsigned char *data,
 								u16 len)
 {
@@ -1923,6 +1992,12 @@ int mgmt_control(struct sock *sk, struct msghdr *msg, size_t msglen)
 	case MGMT_OP_USER_CONFIRM_NEG_REPLY:
 		err = user_confirm_reply(sk, index, buf + sizeof(*hdr), len, 0);
 		break;
+	case MGMT_OP_USER_PASSKEY_REPLY:
+		err = user_passkey_reply(sk, index, buf + sizeof(*hdr), len, 1);
+		break;
+	case MGMT_OP_USER_PASSKEY_NEG_REPLY:
+		err = user_passkey_reply(sk, index, buf + sizeof(*hdr), len, 0);
+		break;
 	case MGMT_OP_SET_LOCAL_NAME:
 		err = set_local_name(sk, index, buf + sizeof(*hdr), len);
 		break;
-- 
1.7.7.2
-- 
Brian Gix
bgix@codeaurora.org
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum 

Re: [PATCH 1/2] Bluetooth: Add SMP support to user_confirm_reply

[Marcel Holtmann]

Hi Brian,

> to enable User Confirmation during LE-SMP pairing.
> 
> Signed-off-by: Brian Gix <bgix@codeaurora.org>
> ---
>  net/bluetooth/mgmt.c |   22 +++++++++++++++++++---
>  1 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index a6720c6..6c35f8d 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -1423,6 +1423,7 @@ static int user_confirm_reply(struct sock *sk, u16 index, unsigned char *data,
>  	u16 mgmt_op, hci_op;
>  	struct pending_cmd *cmd;
>  	struct hci_dev *hdev;
> +	struct hci_conn *conn;
>  	int err;
>  
>  	BT_DBG("");
> @@ -1446,20 +1447,35 @@ static int user_confirm_reply(struct sock *sk, u16 index, unsigned char *data,
>  
>  	if (!test_bit(HCI_UP, &hdev->flags)) {
>  		err = cmd_status(sk, index, mgmt_op, ENETDOWN);
> -		goto failed;
> +		goto done;
> +	}
> +
> +	conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &cp->bdaddr);
> +	if (!conn) {
> +		conn = hci_conn_hash_lookup_ba(hdev, LE_LINK, &cp->bdaddr);
> +		if (!conn) {
> +			err = cmd_status(sk, index, mgmt_op, ENOTCONN);
> +			goto done;
> +		}
> +
> +		/* Forward Confirm response to SMP */
> +
> +		err = cmd_status(sk, index, mgmt_op, 0);
> +		goto done;
> +
>  	}

I am a bit lost why we are trying to find an ACL_LINK here first.

Regards

Marcel

Re: [PATCH 2/2] Bluetooth: Add Passkey for SSP & SMP pairing

[Marcel Holtmann]

Hi Brian,

> Some Man-In-The-Middle (MITM) protection schemes require
> User Passkey Entry.
> 
> Signed-off-by: Brian Gix <bgix@codeaurora.org>
> ---
>  include/net/bluetooth/hci.h  |    8 ++++
>  include/net/bluetooth/mgmt.h |   12 ++++++
>  net/bluetooth/mgmt.c         |   77 +++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 96 insertions(+), 1 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index 139ce2a..ac107b5 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -453,6 +453,14 @@ struct hci_rp_user_confirm_reply {
>  
>  #define HCI_OP_USER_CONFIRM_NEG_REPLY	0x042d
>  
> +#define HCI_OP_USER_PASSKEY_REPLY		0x042e
> +struct hci_cp_user_passkey_reply {
> +	bdaddr_t bdaddr;
> +	__u32	passkey;
> +} __packed;
> +
> +#define HCI_OP_USER_PASSKEY_NEG_REPLY	0x042f
> +
>  #define HCI_OP_REMOTE_OOB_DATA_REPLY	0x0430
>  struct hci_cp_remote_oob_data_reply {
>  	bdaddr_t bdaddr;
> diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
> index 3e320c9..aa56bd5 100644
> --- a/include/net/bluetooth/mgmt.h
> +++ b/include/net/bluetooth/mgmt.h
> @@ -228,6 +228,18 @@ struct mgmt_cp_set_fast_connectable {
>  	__u8 enable;
>  } __packed;
>  
> +#define MGMT_OP_USER_PASSKEY_REPLY	0x0020
> +struct mgmt_cp_user_passkey_reply {
> +	bdaddr_t bdaddr;
> +	__le32 passkey;
> +} __packed;
> +
> +#define MGMT_OP_USER_PASSKEY_NEG_REPLY	0x0021
> +struct mgmt_cp_user_passkey_neg_reply {
> +	bdaddr_t bdaddr;
> +} __packed;
> +
> +

No need for two empty lines here.

>  #define MGMT_EV_CMD_COMPLETE		0x0001
>  struct mgmt_ev_cmd_complete {
>  	__le16 opcode;
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 6c35f8d..c6e1ad4 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -1462,7 +1462,6 @@ static int user_confirm_reply(struct sock *sk, u16 index, unsigned char *data,
>  
>  		err = cmd_status(sk, index, mgmt_op, 0);
>  		goto done;
> -
>  	}

And since you just introduced that all by yourself in your previous
patch, please fix it there and not here.
 
>  	cmd = mgmt_pending_add(sk, mgmt_op, hdev, data, len);
> @@ -1482,6 +1481,76 @@ done:
>  	return err;
>  }
>  
> +static int user_passkey_reply(struct sock *sk, u16 index, unsigned char *data,
> +							u16 len, int success)
> +{
> +	struct mgmt_cp_user_passkey_reply *cp = (void *) data;
> +	u16 mgmt_op, hci_op;
> +	u32 passkey = 0;
> +	struct pending_cmd *cmd;
> +	struct hci_dev *hdev;
> +	struct hci_conn *conn;
> +	int expected_len, err = 0;
> +
> +	BT_DBG("");
> +
> +	if (success) {
> +		mgmt_op = MGMT_OP_USER_PASSKEY_REPLY;
> +		hci_op = HCI_OP_USER_PASSKEY_REPLY;
> +		expected_len = sizeof(*cp);
> +		passkey = le32_to_cpu(cp->passkey);
> +	} else {
> +		mgmt_op = MGMT_OP_USER_PASSKEY_NEG_REPLY;
> +		hci_op = HCI_OP_USER_PASSKEY_NEG_REPLY;
> +		expected_len = sizeof(struct mgmt_cp_user_passkey_neg_reply);
> +	}
> +
> +	if (len != expected_len)
> +		return cmd_status(sk, index, mgmt_op, EINVAL);
> +
> +	hdev = hci_dev_get(index);
> +	if (!hdev)
> +		return cmd_status(sk, index, mgmt_op, ENODEV);
> +
> +	hci_dev_lock_bh(hdev);
> +
> +	if (!test_bit(HCI_UP, &hdev->flags)) {
> +		err = cmd_status(sk, index, mgmt_op, ENETDOWN);
> +		goto done;
> +	}
> +
> +	conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &cp->bdaddr);
> +	if (!conn) {
> +		conn = hci_conn_hash_lookup_ba(hdev, LE_LINK, &cp->bdaddr);
> +		if (!conn) {
> +			err = cmd_status(sk, index, mgmt_op, ENOTCONN);
> +			goto done;
> +		}
> +
> +		/* Forward Passkey response to SMP */
> +
> +		err = cmd_status(sk, index, mgmt_op, 0);
> +		goto done;
> +	}

Same question as before, why do we have to lookup the ACL_LINK first?

> +	cmd = mgmt_pending_add(sk, mgmt_op, hdev, data, len);
> +	if (!cmd) {
> +		err = -ENOMEM;
> +		goto done;
> +	}
> +
> +	err = hci_send_cmd(hdev, hci_op, len, cp);
> +
> +	if (err < 0)
> +		mgmt_pending_remove(cmd);
> +
> +done:
> +	hci_dev_unlock_bh(hdev);
> +	hci_dev_put(hdev);
> +
> +	return err;
> +}
> +
>  static int set_local_name(struct sock *sk, u16 index, unsigned char *data,
>  								u16 len)
>  {
> @@ -1923,6 +1992,12 @@ int mgmt_control(struct sock *sk, struct msghdr *msg, size_t msglen)
>  	case MGMT_OP_USER_CONFIRM_NEG_REPLY:
>  		err = user_confirm_reply(sk, index, buf + sizeof(*hdr), len, 0);
>  		break;
> +	case MGMT_OP_USER_PASSKEY_REPLY:
> +		err = user_passkey_reply(sk, index, buf + sizeof(*hdr), len, 1);
> +		break;
> +	case MGMT_OP_USER_PASSKEY_NEG_REPLY:
> +		err = user_passkey_reply(sk, index, buf + sizeof(*hdr), len, 0);
> +		break;

This overloading from two different messages is actually not a really
good idea. It becomes especially bad with this

	struct mgmt_cp_user_passkey_reply *cp = (void *) data;

where one of them is actually a different type, but you cast it anyway.

And I did run a git blame on this and it is mostly Johan's code and you
just follow the existing example, but I don't really like it much. This
needs to be split into two separate function (with no success parameter)
and then just a common tail as helper function.

Regards

Marcel

Re: [PATCH 1/2] Bluetooth: Add SMP support to user_confirm_reply

[Brian Gix]

Hi Marcel,

On 11/9/2011 2:26 PM, Marcel Holtmann wrote:

>> +	conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,&cp->bdaddr);
>> +	if (!conn) {
>> +		conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,&cp->bdaddr);
>> +		if (!conn) {
>> +			err = cmd_status(sk, index, mgmt_op, ENOTCONN);
>> +			goto done;
>> +		}
>> +
>> +		/* Forward Confirm response to SMP */
>> +
>> +		err = cmd_status(sk, index, mgmt_op, 0);
>> +		goto done;
>> +
>>   	}
>
> I am a bit lost why we are trying to find an ACL_LINK here first.

We want to route the confirmation to the Secure Simple Pair HCI command 
if this is being done on an ACL link, and to the SMP pairing algorythm 
if it is being done on an LE link.

While I suppose we could route all non-LE results to the related HCI 
command, it seemed cleaner to me to reject it with an ENOTCONN, rather 
than make an invalid HCI call, and depending on the baseband generating 
the error.

-- 
Brian Gix
bgix@codeaurora.org
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Re: [PATCH 1/2] Bluetooth: Add SMP support to user_confirm_reply

[Marcel Holtmann]

Hi Brian,

> >> +	conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,&cp->bdaddr);
> >> +	if (!conn) {
> >> +		conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,&cp->bdaddr);
> >> +		if (!conn) {
> >> +			err = cmd_status(sk, index, mgmt_op, ENOTCONN);
> >> +			goto done;
> >> +		}
> >> +
> >> +		/* Forward Confirm response to SMP */
> >> +
> >> +		err = cmd_status(sk, index, mgmt_op, 0);
> >> +		goto done;
> >> +
> >>   	}
> >
> > I am a bit lost why we are trying to find an ACL_LINK here first.
> 
> We want to route the confirmation to the Secure Simple Pair HCI command 
> if this is being done on an ACL link, and to the SMP pairing algorythm 
> if it is being done on an LE link.
> 
> While I suppose we could route all non-LE results to the related HCI 
> command, it seemed cleaner to me to reject it with an ENOTCONN, rather 
> than make an invalid HCI call, and depending on the baseband generating 
> the error.

now I am getting it. This is not really obvious when you look at the
code for the first time. Reason is that the ENOTCONN is too nested to be
obvious. Care to add a comment about this so I do not ask dumb question
next time ;)

Regards

Marcel