[PATCH] Bluetooth: Fix to auto accept pairing request for no MITM case

[PATCH] Bluetooth: Fix to auto accept pairing request for no MITM case

[Bing Zhao]

From: Hiren Tandel <hirent@marvell.com>

With existing implementation, during incoming SSP if no side
requires MITM protection, user confirm request event is
forwarded to user space for further user confirmation without
checking io capability of local host. But in case if there is no
agent registered with bluez user space daemon which means device
has NoInputNoOutput capability at that moment, it rejects the
request.

This patch will check for io capability of local host before
forwarding request to user space. If local host has
NoInputNoOutput capability, there is no point in forwarding
request to user space. Hence local host kernel goes ahead with
auto accept, by sending User Confirmation Request Reply command.

Signed-off-by: Hiren Tandel <hirent@marvell.com>
Signed-off-by: Rahul Tank <rahult@marvell.com>
Signed-off-by: Quinton Yuan <qyuan@marvell.com>
Signed-off-by: Bing Zhao <bzhao@marvell.com>
---
 net/bluetooth/hci_event.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index d2c6878..b61f0c6 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -3239,8 +3239,13 @@ static void hci_user_confirm_request_evt(struct hci_dev *hdev,
 
 		/* If we're not the initiators request authorization to
 		 * proceed from user space (mgmt_user_confirm with
-		 * confirm_hint set to 1). */
-		if (!test_bit(HCI_CONN_AUTH_PEND, &conn->flags)) {
+		 * confirm_hint set to 1).
+		 * But if local host has NoInputNoOutput capability,
+		 * then no use of passing request to user space,
+		 * so go ahead with auto accept.
+		 */
+		if (!test_bit(HCI_CONN_AUTH_PEND, &conn->flags) &&
+		    conn->io_capability != HCI_IO_NO_INPUT_OUTPUT) {
 			BT_DBG("Confirming auto-accept as acceptor");
 			confirm_hint = 1;
 			goto confirm;
-- 
1.8.0

RE: [PATCH] Bluetooth: Fix to auto accept pairing request for no MITM case

[Bing Zhao]

Hi Marcel, Gustavo, Johan,

> From: Hiren Tandel <hirent@marvell.com>
>=20
> With existing implementation, during incoming SSP if no side
> requires MITM protection, user confirm request event is
> forwarded to user space for further user confirmation without
> checking io capability of local host. But in case if there is no
> agent registered with bluez user space daemon which means device
> has NoInputNoOutput capability at that moment, it rejects the
> request.
>=20
> This patch will check for io capability of local host before
> forwarding request to user space. If local host has
> NoInputNoOutput capability, there is no point in forwarding
> request to user space. Hence local host kernel goes ahead with
> auto accept, by sending User Confirmation Request Reply command.

Could you please review this patch?

Thanks,
Bing

>=20
> Signed-off-by: Hiren Tandel <hirent@marvell.com>
> Signed-off-by: Rahul Tank <rahult@marvell.com>
> Signed-off-by: Quinton Yuan <qyuan@marvell.com>
> Signed-off-by: Bing Zhao <bzhao@marvell.com>
> ---
>  net/bluetooth/hci_event.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>=20
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index d2c6878..b61f0c6 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -3239,8 +3239,13 @@ static void hci_user_confirm_request_evt(struct hc=
i_dev *hdev,
>=20
>  		/* If we're not the initiators request authorization to
>  		 * proceed from user space (mgmt_user_confirm with
> -		 * confirm_hint set to 1). */
> -		if (!test_bit(HCI_CONN_AUTH_PEND, &conn->flags)) {
> +		 * confirm_hint set to 1).
> +		 * But if local host has NoInputNoOutput capability,
> +		 * then no use of passing request to user space,
> +		 * so go ahead with auto accept.
> +		 */
> +		if (!test_bit(HCI_CONN_AUTH_PEND, &conn->flags) &&
> +		    conn->io_capability !=3D HCI_IO_NO_INPUT_OUTPUT) {
>  			BT_DBG("Confirming auto-accept as acceptor");
>  			confirm_hint =3D 1;
>  			goto confirm;
> --
> 1.8.0

Re: [PATCH] Bluetooth: Fix to auto accept pairing request for no MITM case

[Johan Hedberg]

Hi,

On Wed, Feb 05, 2014, Bing Zhao wrote:
> With existing implementation, during incoming SSP if no side
> requires MITM protection, user confirm request event is
> forwarded to user space for further user confirmation without
> checking io capability of local host. But in case if there is no
> agent registered with bluez user space daemon which means device
> has NoInputNoOutput capability at that moment, it rejects the
> request.
> 
> This patch will check for io capability of local host before
> forwarding request to user space. If local host has
> NoInputNoOutput capability, there is no point in forwarding
> request to user space. Hence local host kernel goes ahead with
> auto accept, by sending User Confirmation Request Reply command.
> 
> Signed-off-by: Hiren Tandel <hirent@marvell.com>
> Signed-off-by: Rahul Tank <rahult@marvell.com>
> Signed-off-by: Quinton Yuan <qyuan@marvell.com>
> Signed-off-by: Bing Zhao <bzhao@marvell.com>
> ---
>  net/bluetooth/hci_event.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index d2c6878..b61f0c6 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -3239,8 +3239,13 @@ static void hci_user_confirm_request_evt(struct hci_dev *hdev,
>  
>  		/* If we're not the initiators request authorization to
>  		 * proceed from user space (mgmt_user_confirm with
> -		 * confirm_hint set to 1). */
> -		if (!test_bit(HCI_CONN_AUTH_PEND, &conn->flags)) {
> +		 * confirm_hint set to 1).
> +		 * But if local host has NoInputNoOutput capability,
> +		 * then no use of passing request to user space,
> +		 * so go ahead with auto accept.
> +		 */
> +		if (!test_bit(HCI_CONN_AUTH_PEND, &conn->flags) &&
> +		    conn->io_capability != HCI_IO_NO_INPUT_OUTPUT) {
>  			BT_DBG("Confirming auto-accept as acceptor");
>  			confirm_hint = 1;
>  			goto confirm;

So basically this changes the policy from rejecting pairings when an
agent is not registered to accepting them in case it's a just-works
pairing? I'm not convinced we want to do such a policy change that
relaxes security in this regard.

Do you have an actual product use case for this? What's the background
and reasoning behind it?

Johan

RE: [PATCH] Bluetooth: Fix to auto accept pairing request for no MITM case

[Hirenkumar Tandel]

Hi Johan,

-----Original Message-----
From: Johan Hedberg [mailto:johan.hedberg@gmail.com]=20
Sent: Saturday, March 29, 2014 12:54 PM
To: Bing Zhao
Cc: linux-bluetooth@vger.kernel.org; Marcel Holtmann; Gustavo Padovan; Hire=
nkumar Tandel; Rahul Tank; Quinton Yuan
Subject: Re: [PATCH] Bluetooth: Fix to auto accept pairing request for no M=
ITM case

Hi,

On Wed, Feb 05, 2014, Bing Zhao wrote:
> With existing implementation, during incoming SSP if no side requires=20
> MITM protection, user confirm request event is forwarded to user space=20
> for further user confirmation without checking io capability of local=20
> host. But in case if there is no agent registered with bluez user=20
> space daemon which means device has NoInputNoOutput capability at that=20
> moment, it rejects the request.
>=20
> This patch will check for io capability of local host before=20
> forwarding request to user space. If local host has NoInputNoOutput=20
> capability, there is no point in forwarding request to user space.=20
> Hence local host kernel goes ahead with auto accept, by sending User=20
> Confirmation Request Reply command.
>=20
> Signed-off-by: Hiren Tandel <hirent@marvell.com>
> Signed-off-by: Rahul Tank <rahult@marvell.com>
> Signed-off-by: Quinton Yuan <qyuan@marvell.com>
> Signed-off-by: Bing Zhao <bzhao@marvell.com>
> ---
>  net/bluetooth/hci_event.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>=20
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c=20
> index d2c6878..b61f0c6 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -3239,8 +3239,13 @@ static void hci_user_confirm_request_evt(struct=20
> hci_dev *hdev,
> =20
>  		/* If we're not the initiators request authorization to
>  		 * proceed from user space (mgmt_user_confirm with
> -		 * confirm_hint set to 1). */
> -		if (!test_bit(HCI_CONN_AUTH_PEND, &conn->flags)) {
> +		 * confirm_hint set to 1).
> +		 * But if local host has NoInputNoOutput capability,
> +		 * then no use of passing request to user space,
> +		 * so go ahead with auto accept.
> +		 */
> +		if (!test_bit(HCI_CONN_AUTH_PEND, &conn->flags) &&
> +		    conn->io_capability !=3D HCI_IO_NO_INPUT_OUTPUT) {
>  			BT_DBG("Confirming auto-accept as acceptor");
>  			confirm_hint =3D 1;
>  			goto confirm;

So basically this changes the policy from rejecting pairings when an agent =
is not registered to accepting them in case it's a just-works pairing? I'm =
not convinced we want to do such a policy change that relaxes security in t=
his regard.

Do you have an actual product use case for this? What's the background and =
reasoning behind it?

[Hiren]A use case is for chrome book, for user not logged in case, Bluetoot=
h is working at this time, but default-agent is not registered, however use=
r will expect  Bluetooth to work for certain simple devices like mouse.

Johan

Thanks,
Hiren

Re: [PATCH] Bluetooth: Fix to auto accept pairing request for no MITM case

[Johan Hedberg]

Hi Hiren,

On Sun, Mar 30, 2014, Hirenkumar Tandel wrote:
>>> --- a/net/bluetooth/hci_event.c
>>> +++ b/net/bluetooth/hci_event.c
>>> @@ -3239,8 +3239,13 @@ static void hci_user_confirm_request_evt(struct 
>>> hci_dev *hdev,
>>>  
>>>  		/* If we're not the initiators request authorization to
>>>  		 * proceed from user space (mgmt_user_confirm with
>>> -		 * confirm_hint set to 1). */
>>> -		if (!test_bit(HCI_CONN_AUTH_PEND, &conn->flags)) {
>>> +		 * confirm_hint set to 1).
>>> +		 * But if local host has NoInputNoOutput capability,
>>> +		 * then no use of passing request to user space,
>>> +		 * so go ahead with auto accept.
>>> +		 */
>>> +		if (!test_bit(HCI_CONN_AUTH_PEND, &conn->flags) &&
>>> +		    conn->io_capability != HCI_IO_NO_INPUT_OUTPUT) {
>>>  			BT_DBG("Confirming auto-accept as acceptor");
>>>  			confirm_hint = 1;
>>>  			goto confirm;
>>
>> So basically this changes the policy from rejecting pairings when an
>> agent is not registered to accepting them in case it's a just-works
>> pairing? I'm not convinced we want to do such a policy change that
>> relaxes security in this regard.
>> 
>> Do you have an actual product use case for this? What's the background
>> and reasoning behind it?
> 
> A use case is for chrome book, for user not logged in case, Bluetooth
> is working at this time, but default-agent is not registered, however
> user will expect  Bluetooth to work for certain simple devices like
> mouse.

I still don't completely understand this. If the user has paired the
mouse previously (which must be the case since otherwise the mouse would
not be connecting to the PC) then there will not be any user confirm
request during the connection creation.

Johan

Re: [PATCH] Bluetooth: Fix to auto accept pairing request for no MITM case

[Marcel Holtmann]

Hi Johan,

>>>> --- a/net/bluetooth/hci_event.c
>>>> +++ b/net/bluetooth/hci_event.c
>>>> @@ -3239,8 +3239,13 @@ static void hci_user_confirm_request_evt(struct 
>>>> hci_dev *hdev,
>>>> 
>>>> 		/* If we're not the initiators request authorization to
>>>> 		 * proceed from user space (mgmt_user_confirm with
>>>> -		 * confirm_hint set to 1). */
>>>> -		if (!test_bit(HCI_CONN_AUTH_PEND, &conn->flags)) {
>>>> +		 * confirm_hint set to 1).
>>>> +		 * But if local host has NoInputNoOutput capability,
>>>> +		 * then no use of passing request to user space,
>>>> +		 * so go ahead with auto accept.
>>>> +		 */
>>>> +		if (!test_bit(HCI_CONN_AUTH_PEND, &conn->flags) &&
>>>> +		    conn->io_capability != HCI_IO_NO_INPUT_OUTPUT) {
>>>> 			BT_DBG("Confirming auto-accept as acceptor");
>>>> 			confirm_hint = 1;
>>>> 			goto confirm;
>>> 
>>> So basically this changes the policy from rejecting pairings when an
>>> agent is not registered to accepting them in case it's a just-works
>>> pairing? I'm not convinced we want to do such a policy change that
>>> relaxes security in this regard.
>>> 
>>> Do you have an actual product use case for this? What's the background
>>> and reasoning behind it?
>> 
>> A use case is for chrome book, for user not logged in case, Bluetooth
>> is working at this time, but default-agent is not registered, however
>> user will expect  Bluetooth to work for certain simple devices like
>> mouse.
> 
> I still don't completely understand this. If the user has paired the
> mouse previously (which must be the case since otherwise the mouse would
> not be connecting to the PC) then there will not be any user confirm
> request during the connection creation.

actually bluetoothd must be running to accept an incoming connection from a HID device. The initial connection handling is done in bluetoothd. Also without bluetoothd running, you do not have page scan enabled either. The kernel starts out with page scan disabled.

Regards

Marcel

Re: [PATCH] Bluetooth: Fix to auto accept pairing request for no MITM case

[Johan Hedberg]

Hi Marcel,

On Mon, Mar 31, 2014, Marcel Holtmann wrote:
> >>>> --- a/net/bluetooth/hci_event.c
> >>>> +++ b/net/bluetooth/hci_event.c
> >>>> @@ -3239,8 +3239,13 @@ static void hci_user_confirm_request_evt(struct 
> >>>> hci_dev *hdev,
> >>>> 
> >>>> 		/* If we're not the initiators request authorization to
> >>>> 		 * proceed from user space (mgmt_user_confirm with
> >>>> -		 * confirm_hint set to 1). */
> >>>> -		if (!test_bit(HCI_CONN_AUTH_PEND, &conn->flags)) {
> >>>> +		 * confirm_hint set to 1).
> >>>> +		 * But if local host has NoInputNoOutput capability,
> >>>> +		 * then no use of passing request to user space,
> >>>> +		 * so go ahead with auto accept.
> >>>> +		 */
> >>>> +		if (!test_bit(HCI_CONN_AUTH_PEND, &conn->flags) &&
> >>>> +		    conn->io_capability != HCI_IO_NO_INPUT_OUTPUT) {
> >>>> 			BT_DBG("Confirming auto-accept as acceptor");
> >>>> 			confirm_hint = 1;
> >>>> 			goto confirm;
> >>> 
> >>> So basically this changes the policy from rejecting pairings when an
> >>> agent is not registered to accepting them in case it's a just-works
> >>> pairing? I'm not convinced we want to do such a policy change that
> >>> relaxes security in this regard.
> >>> 
> >>> Do you have an actual product use case for this? What's the background
> >>> and reasoning behind it?
> >> 
> >> A use case is for chrome book, for user not logged in case, Bluetooth
> >> is working at this time, but default-agent is not registered, however
> >> user will expect  Bluetooth to work for certain simple devices like
> >> mouse.
> > 
> > I still don't completely understand this. If the user has paired the
> > mouse previously (which must be the case since otherwise the mouse would
> > not be connecting to the PC) then there will not be any user confirm
> > request during the connection creation.
> 
> actually bluetoothd must be running to accept an incoming connection
> from a HID device. The initial connection handling is done in
> bluetoothd. Also without bluetoothd running, you do not have page scan
> enabled either. The kernel starts out with page scan disabled.

Sure, but if I understand the situation here bluetoothd *is* already
running at the login prompt stage but there is no agent registered. In
such a case I think it should be fine to accept connections from already
paired and trusted devices but we should probably keep rejecting them
from any other device (even if the pairing would trigger just-works).

Johan

Re: [PATCH] Bluetooth: Fix to auto accept pairing request for no MITM case

[Scott James Remnant]

On Sun, Mar 30, 2014 at 9:34 PM, Hirenkumar Tandel <hirent@marvell.com> wrote:
>
> So basically this changes the policy from rejecting pairings when an agent is not registered to accepting them in case it's a just-works pairing? I'm not convinced we want to do such a policy change that relaxes security in this regard.
>
> Do you have an actual product use case for this? What's the background and reasoning behind it?
>
> [Hiren]A use case is for chrome book, for user not logged in case, Bluetooth is working at this time, but default-agent is not registered, however user will expect  Bluetooth to work for certain simple devices like mouse.
>

That is NOT a use case that is valid for a Chromebook. We do NOT want
to accept random Bluetooth pairings without a user logged in to
confirm.

Scott
-- 
Scott James Remnant | Chrome OS Systems | keybuk@google.com | Google

RE: [PATCH] Bluetooth: Fix to auto accept pairing request for no MITM case

[Hirenkumar Tandel]

Hi Johan,

-----Original Message-----
From: Johan Hedberg [mailto:johan.hedberg@gmail.com]=20
Sent: Monday, March 31, 2014 3:20 PM
To: Marcel Holtmann
Cc: Hirenkumar Tandel; Bing Zhao; linux-bluetooth@vger.kernel.org; Gustavo =
F. Padovan; Rahul Tank; Quinton Yuan
Subject: Re: [PATCH] Bluetooth: Fix to auto accept pairing request for no M=
ITM case

Hi Marcel,

On Mon, Mar 31, 2014, Marcel Holtmann wrote:
> >>>> --- a/net/bluetooth/hci_event.c
> >>>> +++ b/net/bluetooth/hci_event.c
> >>>> @@ -3239,8 +3239,13 @@ static void=20
> >>>> hci_user_confirm_request_evt(struct
> >>>> hci_dev *hdev,
> >>>>=20
> >>>> 		/* If we're not the initiators request authorization to
> >>>> 		 * proceed from user space (mgmt_user_confirm with
> >>>> -		 * confirm_hint set to 1). */
> >>>> -		if (!test_bit(HCI_CONN_AUTH_PEND, &conn->flags)) {
> >>>> +		 * confirm_hint set to 1).
> >>>> +		 * But if local host has NoInputNoOutput capability,
> >>>> +		 * then no use of passing request to user space,
> >>>> +		 * so go ahead with auto accept.
> >>>> +		 */
> >>>> +		if (!test_bit(HCI_CONN_AUTH_PEND, &conn->flags) &&
> >>>> +		    conn->io_capability !=3D HCI_IO_NO_INPUT_OUTPUT) {
> >>>> 			BT_DBG("Confirming auto-accept as acceptor");
> >>>> 			confirm_hint =3D 1;
> >>>> 			goto confirm;
> >>>=20
> >>> So basically this changes the policy from rejecting pairings when=20
> >>> an agent is not registered to accepting them in case it's a=20
> >>> just-works pairing? I'm not convinced we want to do such a policy=20
> >>> change that relaxes security in this regard.
> >>>=20
> >>> Do you have an actual product use case for this? What's the=20
> >>> background and reasoning behind it?
> >>=20
> >> A use case is for chrome book, for user not logged in case,=20
> >> Bluetooth is working at this time, but default-agent is not=20
> >> registered, however user will expect  Bluetooth to work for certain=20
> >> simple devices like mouse.
> >=20
> > I still don't completely understand this. If the user has paired the=20
> > mouse previously (which must be the case since otherwise the mouse=20
> > would not be connecting to the PC) then there will not be any user=20
> > confirm request during the connection creation.
>=20
> actually bluetoothd must be running to accept an incoming connection=20
> from a HID device. The initial connection handling is done in=20
> bluetoothd. Also without bluetoothd running, you do not have page scan=20
> enabled either. The kernel starts out with page scan disabled.

Sure, but if I understand the situation here bluetoothd *is* already runnin=
g at the login prompt stage but there is no agent registered. In such a cas=
e I think it should be fine to accept connections from already paired and t=
rusted devices but we should probably keep rejecting them from any other de=
vice (even if the pairing would trigger just-works).

We are ok to drop this patch if, without agent registered, it is not comfor=
table to allow incoming pairing which even ask for just work model.

Johan

Thanks,
Hiren