[PATCH] Bluetooth: Always request for user confirmation for Just Works (LE SC)

[PATCH] Bluetooth: Always request for user confirmation for Just Works (LE SC)

[Sonny Sasaka]

To improve security, always give the user-space daemon a chance to
accept or reject a Just Works pairing (LE). The daemon may decide to
auto-accept based on the user's intent.

This patch is similar to the previous patch but applies for LE Secure
Connections (SC).

Signed-off-by: Sonny Sasaka <sonnysasaka@chromium.org>
---
 net/bluetooth/smp.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index d0b695ee49f6..daf03339dedd 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -2202,7 +2202,7 @@ static u8 smp_cmd_pairing_random(struct l2cap_conn *conn, struct sk_buff *skb)
 	if (err)
 		return SMP_UNSPECIFIED;
 
-	if (smp->method == JUST_WORKS || smp->method == REQ_OOB) {
+	if (smp->method == REQ_OOB) {
 		if (hcon->out) {
 			sc_dhkey_check(smp);
 			SMP_ALLOW_CMD(smp, SMP_CMD_DHKEY_CHECK);
@@ -2210,6 +2210,18 @@ static u8 smp_cmd_pairing_random(struct l2cap_conn *conn, struct sk_buff *skb)
 		return 0;
 	}
 
+	/* If Just Works, ask user-space for confirmation. */
+	if (smp->method == JUST_WORKS) {
+		err = mgmt_user_confirm_request(hcon->hdev, &hcon->dst,
+				hcon->type, hcon->dst_type, passkey, 1);
+		if (err)
+			return SMP_UNSPECIFIED;
+
+		set_bit(SMP_FLAG_WAIT_USER, &smp->flags);
+
+		return 0;
+	}
+
 	err = smp_g2(smp->tfm_cmac, pkax, pkbx, na, nb, &passkey);
 	if (err)
 		return SMP_UNSPECIFIED;
-- 
2.17.1

Re: [PATCH] Bluetooth: Always request for user confirmation for Just Works (LE SC)

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 5719 bytes --]

Hi Sonny,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on bluetooth-next/master]
[also build test WARNING on v5.6 next-20200405]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Sonny-Sasaka/Bluetooth-Always-request-for-user-confirmation-for-Just-Works-LE-SC/20200405-051523
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master
config: x86_64-randconfig-b002-20200405 (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project be84d2b5b7e9c98e93bf8565e3e178e43ea0ec0a)
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> net/bluetooth/smp.c:2216:33: warning: variable 'passkey' is uninitialized when used here [-Wuninitialized]
                                   hcon->type, hcon->dst_type, passkey, 1);
                                                               ^~~~~~~
   net/bluetooth/smp.c:2127:13: note: initialize the variable 'passkey' to silence this warning
           u32 passkey;
                      ^
                       = 0
   1 warning generated.

vim +/passkey +2216 net/bluetooth/smp.c

  2120	
  2121	static u8 smp_cmd_pairing_random(struct l2cap_conn *conn, struct sk_buff *skb)
  2122	{
  2123		struct l2cap_chan *chan = conn->smp;
  2124		struct smp_chan *smp = chan->data;
  2125		struct hci_conn *hcon = conn->hcon;
  2126		u8 *pkax, *pkbx, *na, *nb, confirm_hint;
  2127		u32 passkey;
  2128		int err;
  2129	
  2130		BT_DBG("conn %p", conn);
  2131	
  2132		if (skb->len < sizeof(smp->rrnd))
  2133			return SMP_INVALID_PARAMS;
  2134	
  2135		memcpy(smp->rrnd, skb->data, sizeof(smp->rrnd));
  2136		skb_pull(skb, sizeof(smp->rrnd));
  2137	
  2138		if (!test_bit(SMP_FLAG_SC, &smp->flags))
  2139			return smp_random(smp);
  2140	
  2141		if (hcon->out) {
  2142			pkax = smp->local_pk;
  2143			pkbx = smp->remote_pk;
  2144			na   = smp->prnd;
  2145			nb   = smp->rrnd;
  2146		} else {
  2147			pkax = smp->remote_pk;
  2148			pkbx = smp->local_pk;
  2149			na   = smp->rrnd;
  2150			nb   = smp->prnd;
  2151		}
  2152	
  2153		if (smp->method == REQ_OOB) {
  2154			if (!hcon->out)
  2155				smp_send_cmd(conn, SMP_CMD_PAIRING_RANDOM,
  2156					     sizeof(smp->prnd), smp->prnd);
  2157			SMP_ALLOW_CMD(smp, SMP_CMD_DHKEY_CHECK);
  2158			goto mackey_and_ltk;
  2159		}
  2160	
  2161		/* Passkey entry has special treatment */
  2162		if (smp->method == REQ_PASSKEY || smp->method == DSP_PASSKEY)
  2163			return sc_passkey_round(smp, SMP_CMD_PAIRING_RANDOM);
  2164	
  2165		if (hcon->out) {
  2166			u8 cfm[16];
  2167	
  2168			err = smp_f4(smp->tfm_cmac, smp->remote_pk, smp->local_pk,
  2169				     smp->rrnd, 0, cfm);
  2170			if (err)
  2171				return SMP_UNSPECIFIED;
  2172	
  2173			if (crypto_memneq(smp->pcnf, cfm, 16))
  2174				return SMP_CONFIRM_FAILED;
  2175		} else {
  2176			smp_send_cmd(conn, SMP_CMD_PAIRING_RANDOM, sizeof(smp->prnd),
  2177				     smp->prnd);
  2178			SMP_ALLOW_CMD(smp, SMP_CMD_DHKEY_CHECK);
  2179	
  2180			/* Only Just-Works pairing requires extra checks */
  2181			if (smp->method != JUST_WORKS)
  2182				goto mackey_and_ltk;
  2183	
  2184			/* If there already exists long term key in local host, leave
  2185			 * the decision to user space since the remote device could
  2186			 * be legitimate or malicious.
  2187			 */
  2188			if (hci_find_ltk(hcon->hdev, &hcon->dst, hcon->dst_type,
  2189					 hcon->role)) {
  2190				/* Set passkey to 0. The value can be any number since
  2191				 * it'll be ignored anyway.
  2192				 */
  2193				passkey = 0;
  2194				confirm_hint = 1;
  2195				goto confirm;
  2196			}
  2197		}
  2198	
  2199	mackey_and_ltk:
  2200		/* Generate MacKey and LTK */
  2201		err = sc_mackey_and_ltk(smp, smp->mackey, smp->tk);
  2202		if (err)
  2203			return SMP_UNSPECIFIED;
  2204	
  2205		if (smp->method == REQ_OOB) {
  2206			if (hcon->out) {
  2207				sc_dhkey_check(smp);
  2208				SMP_ALLOW_CMD(smp, SMP_CMD_DHKEY_CHECK);
  2209			}
  2210			return 0;
  2211		}
  2212	
  2213		/* If Just Works, ask user-space for confirmation. */
  2214		if (smp->method == JUST_WORKS) {
  2215			err = mgmt_user_confirm_request(hcon->hdev, &hcon->dst,
> 2216					hcon->type, hcon->dst_type, passkey, 1);
  2217			if (err)
  2218				return SMP_UNSPECIFIED;
  2219	
  2220			set_bit(SMP_FLAG_WAIT_USER, &smp->flags);
  2221	
  2222			return 0;
  2223		}
  2224	
  2225		err = smp_g2(smp->tfm_cmac, pkax, pkbx, na, nb, &passkey);
  2226		if (err)
  2227			return SMP_UNSPECIFIED;
  2228	
  2229		confirm_hint = 0;
  2230	
  2231	confirm:
  2232		err = mgmt_user_confirm_request(hcon->hdev, &hcon->dst, hcon->type,
  2233						hcon->dst_type, passkey, confirm_hint);
  2234		if (err)
  2235			return SMP_UNSPECIFIED;
  2236	
  2237		set_bit(SMP_FLAG_WAIT_USER, &smp->flags);
  2238	
  2239		return 0;
  2240	}
  2241	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 33205 bytes --]

Re: [PATCH] Bluetooth: Always request for user confirmation for Just Works (LE SC)

[Marcel Holtmann]

Hi Sonny,

> To improve security, always give the user-space daemon a chance to
> accept or reject a Just Works pairing (LE). The daemon may decide to
> auto-accept based on the user's intent.
> 
> This patch is similar to the previous patch but applies for LE Secure
> Connections (SC).
> 
> Signed-off-by: Sonny Sasaka <sonnysasaka@chromium.org>
> ---
> net/bluetooth/smp.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
> index d0b695ee49f6..daf03339dedd 100644
> --- a/net/bluetooth/smp.c
> +++ b/net/bluetooth/smp.c
> @@ -2202,7 +2202,7 @@ static u8 smp_cmd_pairing_random(struct l2cap_conn *conn, struct sk_buff *skb)
> 	if (err)
> 		return SMP_UNSPECIFIED;
> 
> -	if (smp->method == JUST_WORKS || smp->method == REQ_OOB) {
> +	if (smp->method == REQ_OOB) {
> 		if (hcon->out) {
> 			sc_dhkey_check(smp);
> 			SMP_ALLOW_CMD(smp, SMP_CMD_DHKEY_CHECK);
> @@ -2210,6 +2210,18 @@ static u8 smp_cmd_pairing_random(struct l2cap_conn *conn, struct sk_buff *skb)
> 		return 0;
> 	}
> 
> +	/* If Just Works, ask user-space for confirmation. */
> +	if (smp->method == JUST_WORKS) {
> +		err = mgmt_user_confirm_request(hcon->hdev, &hcon->dst,
> +				hcon->type, hcon->dst_type, passkey, 1);
> +		if (err)
> +			return SMP_UNSPECIFIED;
> +
> +		set_bit(SMP_FLAG_WAIT_USER, &smp->flags);
> +
> +		return 0;
> +	}
> +
> 	err = smp_g2(smp->tfm_cmac, pkax, pkbx, na, nb, &passkey);
> 	if (err)
> 		return SMP_UNSPECIFIED;

@@ -2202,7 +2204,7 @@ static u8 smp_cmd_pairing_random(struct l2cap_conn *conn, struct sk_buff *skb)
        if (err)
                return SMP_UNSPECIFIED;
 
-       if (smp->method == JUST_WORKS || smp->method == REQ_OOB) {
+       if (smp->method == REQ_OOB) {
                if (hcon->out) {
                        sc_dhkey_check(smp);
                        SMP_ALLOW_CMD(smp, SMP_CMD_DHKEY_CHECK);
@@ -2214,7 +2216,10 @@ static u8 smp_cmd_pairing_random(struct l2cap_conn *conn, struct sk_buff *skb)
        if (err)
                return SMP_UNSPECIFIED;
 
-       confirm_hint = 0;
+       if (smp->method == JUST_WORKS)
+               confirm_hint = 0;
+       else
+               confirm_hint = 1;
 
 confirm:
        err = mgmt_user_confirm_request(hcon->hdev, &hcon->dst, hcon->type,

Isn’t this what you are actually doing (minus the required comment of course)?

Regards

Marcel

[PATCH v2] Bluetooth: Always request for user confirmation for Just Works (LE SC)

[Sonny Sasaka]

To improve security, always give the user-space daemon a chance to
accept or reject a Just Works pairing (LE). The daemon may decide to
auto-accept based on the user's intent.

This patch is similar to the previous patch but applies for LE Secure
Connections (SC).

Signed-off-by: Sonny Sasaka <sonnysasaka@chromium.org>
---
 net/bluetooth/smp.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index d0b695ee49f6..2f48518d120b 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -2202,7 +2202,7 @@ static u8 smp_cmd_pairing_random(struct l2cap_conn *conn, struct sk_buff *skb)
 	if (err)
 		return SMP_UNSPECIFIED;
 
-	if (smp->method == JUST_WORKS || smp->method == REQ_OOB) {
+	if (smp->method == REQ_OOB) {
 		if (hcon->out) {
 			sc_dhkey_check(smp);
 			SMP_ALLOW_CMD(smp, SMP_CMD_DHKEY_CHECK);
@@ -2217,6 +2217,9 @@ static u8 smp_cmd_pairing_random(struct l2cap_conn *conn, struct sk_buff *skb)
 	confirm_hint = 0;
 
 confirm:
+	if (smp->method == JUST_WORKS)
+		confirm_hint = 1;
+
 	err = mgmt_user_confirm_request(hcon->hdev, &hcon->dst, hcon->type,
 					hcon->dst_type, passkey, confirm_hint);
 	if (err)
-- 
2.17.1

Re: [PATCH v2] Bluetooth: Always request for user confirmation for Just Works (LE SC)

[Sonny Sasaka]

Hi Marcel,

Thanks for the suggestion. I have sent an updated patch based on your
suggestion with a little modification. Let me know if this looks good.
Thanks!

On Mon, Apr 6, 2020 at 11:04 AM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
>
> To improve security, always give the user-space daemon a chance to
> accept or reject a Just Works pairing (LE). The daemon may decide to
> auto-accept based on the user's intent.
>
> This patch is similar to the previous patch but applies for LE Secure
> Connections (SC).
>
> Signed-off-by: Sonny Sasaka <sonnysasaka@chromium.org>
> ---
>  net/bluetooth/smp.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
> index d0b695ee49f6..2f48518d120b 100644
> --- a/net/bluetooth/smp.c
> +++ b/net/bluetooth/smp.c
> @@ -2202,7 +2202,7 @@ static u8 smp_cmd_pairing_random(struct l2cap_conn *conn, struct sk_buff *skb)
>         if (err)
>                 return SMP_UNSPECIFIED;
>
> -       if (smp->method == JUST_WORKS || smp->method == REQ_OOB) {
> +       if (smp->method == REQ_OOB) {
>                 if (hcon->out) {
>                         sc_dhkey_check(smp);
>                         SMP_ALLOW_CMD(smp, SMP_CMD_DHKEY_CHECK);
> @@ -2217,6 +2217,9 @@ static u8 smp_cmd_pairing_random(struct l2cap_conn *conn, struct sk_buff *skb)
>         confirm_hint = 0;
>
>  confirm:
> +       if (smp->method == JUST_WORKS)
> +               confirm_hint = 1;
> +
>         err = mgmt_user_confirm_request(hcon->hdev, &hcon->dst, hcon->type,
>                                         hcon->dst_type, passkey, confirm_hint);
>         if (err)
> --
> 2.17.1
>

Re: [PATCH v2] Bluetooth: Always request for user confirmation for Just Works (LE SC)

[Sonny Sasaka]

Hi Marcel,

Could you please take another look at this v2 patch based on your
suggestions? Thanks.

On Mon, Apr 6, 2020 at 11:04 AM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
>
> To improve security, always give the user-space daemon a chance to
> accept or reject a Just Works pairing (LE). The daemon may decide to
> auto-accept based on the user's intent.
>
> This patch is similar to the previous patch but applies for LE Secure
> Connections (SC).
>
> Signed-off-by: Sonny Sasaka <sonnysasaka@chromium.org>
> ---
>  net/bluetooth/smp.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
> index d0b695ee49f6..2f48518d120b 100644
> --- a/net/bluetooth/smp.c
> +++ b/net/bluetooth/smp.c
> @@ -2202,7 +2202,7 @@ static u8 smp_cmd_pairing_random(struct l2cap_conn *conn, struct sk_buff *skb)
>         if (err)
>                 return SMP_UNSPECIFIED;
>
> -       if (smp->method == JUST_WORKS || smp->method == REQ_OOB) {
> +       if (smp->method == REQ_OOB) {
>                 if (hcon->out) {
>                         sc_dhkey_check(smp);
>                         SMP_ALLOW_CMD(smp, SMP_CMD_DHKEY_CHECK);
> @@ -2217,6 +2217,9 @@ static u8 smp_cmd_pairing_random(struct l2cap_conn *conn, struct sk_buff *skb)
>         confirm_hint = 0;
>
>  confirm:
> +       if (smp->method == JUST_WORKS)
> +               confirm_hint = 1;
> +
>         err = mgmt_user_confirm_request(hcon->hdev, &hcon->dst, hcon->type,
>                                         hcon->dst_type, passkey, confirm_hint);
>         if (err)
> --
> 2.17.1
>

Re: [PATCH v2] Bluetooth: Always request for user confirmation for Just Works (LE SC)

[Marcel Holtmann]

Hi Sonny,

> To improve security, always give the user-space daemon a chance to
> accept or reject a Just Works pairing (LE). The daemon may decide to
> auto-accept based on the user's intent.
> 
> This patch is similar to the previous patch but applies for LE Secure
> Connections (SC).
> 
> Signed-off-by: Sonny Sasaka <sonnysasaka@chromium.org>
> ---
> net/bluetooth/smp.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel