[v2] Bluetooth: hci_event: Ignore NULL link key

Commit Message

Chun-Yi Lee July 18, 2023, 3:43 a.m. UTC

This change is used to relieve CVE-2020-26555. The description of the
CVE:

Bluetooth legacy BR/EDR PIN code pairing in Bluetooth Core Specification
1.0B through 5.2 may permit an unauthenticated nearby device to spoof
the BD_ADDR of the peer device to complete pairing without knowledge
of the PIN. [1]

The detail of this attack is in IEEE paper:
BlueMirror: Reflections on Bluetooth Pairing and Provisioning Protocols
[2]

It's a reflection attack. Base on the paper, attacker can induce the
attacked target to generate null link key (zero key) without PIN code.

We can ignore null link key in the handler of "Link Key Notification
event" to relieve the attack. A similar implementation also shows in
btstack project. [3]

v2:
- Used Link: tag instead of Closes:
- Used bt_dev_dbg instead of BT_DBG
- Added Fixes: tag

Fixes: 55ed8ca10f35 ("Bluetooth: Implement link key handling for the management interface")
Link: https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-26555 [1]
Link: https://ieeexplore.ieee.org/abstract/document/9474325/authors#authors [2]
Link: https://github.com/bluekitchen/btstack/blob/master/src/hci.c#L3722 [3]
Signed-off-by: "Lee, Chun-Yi" <[e-mail]>
---
 net/bluetooth/hci_event.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

[e-mail] July 18, 2023, 4:37 a.m. UTC | #1

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=766763

---Test result---

Test Summary:
CheckPatch                    FAIL      0.81 seconds
GitLint                       PASS      0.27 seconds
SubjectPrefix                 PASS      0.09 seconds
BuildKernel                   PASS      32.47 seconds
CheckAllWarning               PASS      35.57 seconds
CheckSparse                   WARNING   40.49 seconds
CheckSmatch                   WARNING   111.98 seconds
BuildKernel32                 PASS      31.15 seconds
TestRunnerSetup               PASS      476.51 seconds
TestRunner_l2cap-tester       PASS      22.02 seconds
TestRunner_iso-tester         PASS      40.00 seconds
TestRunner_bnep-tester        PASS      10.08 seconds
TestRunner_mgmt-tester        PASS      211.97 seconds
TestRunner_rfcomm-tester      PASS      15.18 seconds
TestRunner_sco-tester         PASS      16.01 seconds
TestRunner_ioctl-tester       PASS      16.96 seconds
TestRunner_mesh-tester        PASS      12.81 seconds
TestRunner_smp-tester         PASS      13.44 seconds
TestRunner_userchan-tester    PASS      10.61 seconds
IncrementalBuild              PASS      29.63 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[v2] Bluetooth: hci_event: Ignore NULL link key
WARNING: From:/Signed-off-by: email address mismatch: 'From: "Lee, Chun-Yi" <[e-mail]>' != 'Signed-off-by: "Lee, Chun-Yi" <[e-mail]>'

total: 0 errors, 1 warnings, 0 checks, 12 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/src/13316718.patch has style problems, please review.

NOTE: Ignored message types: UNKNOWN_COMMIT_ID

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: CheckSparse - WARNING
Desc: Run sparse tool with linux kernel
Output:
net/bluetooth/hci_event.c: note: in included file (through include/net/bluetooth/hci_core.h):
##############################
Test: CheckSmatch - WARNING
Desc: Run smatch tool with source
Output:
net/bluetooth/hci_event.c: note: in included file (through include/net/bluetooth/hci_core.h):


---
Regards,
Linux Bluetooth

Paul Menzel July 18, 2023, 5:40 a.m. UTC | #2

Dear Chun-Yi,


Thank you for your patch.

Am 18.07.23 um 05:43 schrieb Lee, Chun-Yi <[e-mail]>:

[…]

> Signed-off-by: "Lee, Chun-Yi" <[e-mail]>

As checkpatch.pl also reports, please make sure the author and 
Signed-off-by entry match.

     $ git config --global user.name "Chun-Yi  Lee"
     $ git commit --amend --author="Chun-Yi Lee <[e-mail]>" -s

(It’s also common to write the name in the order, so no comma is needed.)

`git format-patch` should not generate a patch with a dedicated `From:` 
at the beginning, so you can send it from a different email account. (No 
idea, why upstream Linux kernel development shouldn’t work with your 
SUSE address.)


Kind regards,

Paul

Luiz Augusto von Dentz July 18, 2023, 5:22 p.m. UTC | #3

Hi Chun-Yi,

On Mon, Jul 17, 2023 at 8:43 PM Lee, Chun-Yi <[e-mail]> wrote:
>
> This change is used to relieve CVE-2020-26555. The description of the
> CVE:
>
> Bluetooth legacy BR/EDR PIN code pairing in Bluetooth Core Specification
> 1.0B through 5.2 may permit an unauthenticated nearby device to spoof
> the BD_ADDR of the peer device to complete pairing without knowledge
> of the PIN. [1]

Btw, it is probably worth mentioning that in BR/EDR the key generation
is actually handled in the controller, below HCI.

> The detail of this attack is in IEEE paper:
> BlueMirror: Reflections on Bluetooth Pairing and Provisioning Protocols
> [2]
>
> It's a reflection attack. Base on the paper, attacker can induce the
> attacked target to generate null link key (zero key) without PIN code.
>
> We can ignore null link key in the handler of "Link Key Notification
> event" to relieve the attack. A similar implementation also shows in
> btstack project. [3]

Perhaps we could clarify this statement by stating that if we ignore
the link key it means the stack will not consider the device is bonded
and will not persist the link key, that said the controller will still
consider it as paired, so I perhaps we should go one step forward and
disconnect if we detect such a key is being used.

> v2:
> - Used Link: tag instead of Closes:
> - Used bt_dev_dbg instead of BT_DBG
> - Added Fixes: tag
>
> Fixes: 55ed8ca10f35 ("Bluetooth: Implement link key handling for the management interface")
> Link: https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-26555 [1]
> Link: https://ieeexplore.ieee.org/abstract/document/9474325/authors#authors [2]
> Link: https://github.com/bluekitchen/btstack/blob/master/src/hci.c#L3722 [3]
> Signed-off-by: "Lee, Chun-Yi" <[e-mail]>
> ---
>  net/bluetooth/hci_event.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 95816a938cea..ff0c331f53d6 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -4684,6 +4684,12 @@ static void hci_link_key_notify_evt(struct hci_dev *hdev, void *data,
>         bool persistent;
>         u8 pin_len = 0;
>
> +       /* Ignore NULL link key against CVE-2020-26555 */
> +       if (!memcmp(ev->link_key, ZERO_KEY, HCI_LINK_KEY_SIZE)) {
> +               bt_dev_dbg(hdev, "Ignore NULL link key (ZERO KEY) for %pMR", &ev->bdaddr);
> +               return;
> +       }
> +
>         bt_dev_dbg(hdev, "");
>
>         hci_dev_lock(hdev);
> --
> 2.35.3
>

joeyli July 19, 2023, 3:38 p.m. UTC | #4

Hi Paul, 

Thanks for your review!

On Tue, Jul 18, 2023 at 07:40:37AM +0200, Paul Menzel wrote:
> Dear Chun-Yi,
> 
> 
> Thank you for your patch.
> 
> Am 18.07.23 um 05:43 schrieb Lee, Chun-Yi <[e-mail]>:
> 
> […]
> 
> > Signed-off-by: "Lee, Chun-Yi" <[e-mail]>
> 
> As checkpatch.pl also reports, please make sure the author and Signed-off-by
> entry match.
> 
>     $ git config --global user.name "Chun-Yi  Lee"
>     $ git commit --amend --author="Chun-Yi Lee <[e-mail]>" -s
> 
> (It’s also common to write the name in the order, so no comma is needed.)
> 
> `git format-patch` should not generate a patch with a dedicated `From:` at
> the beginning, so you can send it from a different email account. (No idea,
> why upstream Linux kernel development shouldn’t work with your SUSE
> address.)
>

I have set the from in .gitconfig and also tried git send-email --from "Lee, Chun-Yi <[e-mail]>".
But gmail always modified it to From: Lee, Chun-Yi <[e-mail]>. I have no idea why.

In next version, I will put Signed-off-by: "Lee, Chun-Yi" <[e-mail]>
to keep the From: to be sync with Signed-off-by.

Thanks a lot!
Joey Lee

joeyli July 19, 2023, 3:40 p.m. UTC | #5

Hi Markus,

On Tue, Jul 18, 2023 at 07:50:13AM +0200, Markus Elfring wrote:
> >                         … Base on the paper, attacker can induce the
> > attacked target to generate …
> 
> Would you like to avoid also any wording weaknesses here?
> 
> 
> > We can ignore null link key in the handler of "Link Key Notification
> > event" to relieve the attack. …
> 
> Will corresponding imperative change descriptions become more helpful?
> 
> See also:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.5-rc2#n94
>

If possible, could you please direct change my original patch description
for your suggestion? Then I will put it in next version.

Thanks a lot!
Joey Lee

joeyli July 19, 2023, 3:49 p.m. UTC | #6

Hi Luiz Augusto von Dentz, 

On Tue, Jul 18, 2023 at 10:22:26AM -0700, Luiz Augusto von Dentz wrote:
> Hi Chun-Yi,
> 
> On Mon, Jul 17, 2023 at 8:43 PM Lee, Chun-Yi <[e-mail]> wrote:
> >
> > This change is used to relieve CVE-2020-26555. The description of the
> > CVE:
> >
> > Bluetooth legacy BR/EDR PIN code pairing in Bluetooth Core Specification
> > 1.0B through 5.2 may permit an unauthenticated nearby device to spoof
> > the BD_ADDR of the peer device to complete pairing without knowledge
> > of the PIN. [1]
> 
> Btw, it is probably worth mentioning that in BR/EDR the key generation
> is actually handled in the controller, below HCI.
>

Yes, the key generation be handled by link manager. I will mention it
in patch description. 
 
> > The detail of this attack is in IEEE paper:
> > BlueMirror: Reflections on Bluetooth Pairing and Provisioning Protocols
> > [2]
> >
> > It's a reflection attack. Base on the paper, attacker can induce the
> > attacked target to generate null link key (zero key) without PIN code.
> >
> > We can ignore null link key in the handler of "Link Key Notification
> > event" to relieve the attack. A similar implementation also shows in
> > btstack project. [3]
> 
> Perhaps we could clarify this statement by stating that if we ignore
> the link key it means the stack will not consider the device is bonded
> and will not persist the link key, that said the controller will still
> consider it as paired, so I perhaps we should go one step forward and
> disconnect if we detect such a key is being used.
>

I am new on bluetooth field. Did you mean like this patch? Sending
HCI_Disconnect when we found zero link key?

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index ff0c331f53d6..3482031cbbb8 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -4698,6 +4700,15 @@ static void hci_link_key_notify_evt(struct hci_dev *hdev, void *data,
        if (!conn)
                goto unlock;
 
+       /* Ignore NULL link key against CVE-2020-26555 */
+       if (!memcmp(ev->link_key, ZERO_KEY, HCI_LINK_KEY_SIZE)) {
+               bt_dev_dbg(hdev, "Ignore NULL link key (ZERO KEY) for %pMR", &ev->bdaddr);
+               hci_disconnect(conn, HCI_ERROR_AUTH_FAILURE);
+               hci_conn_drop(conn);
+               goto unlock;
+       }
+
        hci_conn_hold(conn);
        conn->disc_timeout = HCI_DISCONN_TIMEOUT;
        hci_conn_drop(conn);


Is there anything I'm missing? Thanks a lot!

> > v2:
> > - Used Link: tag instead of Closes:
> > - Used bt_dev_dbg instead of BT_DBG
> > - Added Fixes: tag
> >
> > Fixes: 55ed8ca10f35 ("Bluetooth: Implement link key handling for the management interface")
> > Link: https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-26555 [1]
> > Link: https://ieeexplore.ieee.org/abstract/document/9474325/authors#authors [2]
> > Link: https://github.com/bluekitchen/btstack/blob/master/src/hci.c#L3722 [3]
> > Signed-off-by: "Lee, Chun-Yi" <[e-mail]>
> > ---
> >  net/bluetooth/hci_event.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > index 95816a938cea..ff0c331f53d6 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -4684,6 +4684,12 @@ static void hci_link_key_notify_evt(struct hci_dev *hdev, void *data,
> >         bool persistent;
> >         u8 pin_len = 0;
> >
> > +       /* Ignore NULL link key against CVE-2020-26555 */
> > +       if (!memcmp(ev->link_key, ZERO_KEY, HCI_LINK_KEY_SIZE)) {
> > +               bt_dev_dbg(hdev, "Ignore NULL link key (ZERO KEY) for %pMR", &ev->bdaddr);
> > +               return;
> > +       }
> > +
> >         bt_dev_dbg(hdev, "");
> >
> >         hci_dev_lock(hdev);
> > --
> > 2.35.3
> >

Thanks a lot!
Joey Lee

Dan Carpenter July 19, 2023, 3:59 p.m. UTC | #7

You know how when people are forwarding a patch from someone else it
adds a From: as the first line in the body of the email?  You could set
your From to the @suse.com address that way.

This is sometimes done in companies where the corporate email server
mangles patches so they have to use an outside server.

regards,
dan carpenter

Luiz Augusto von Dentz July 20, 2023, 12:25 a.m. UTC | #8

Hi Joeyli,

On Wed, Jul 19, 2023 at 8:49 AM joeyli <[e-mail]> wrote:
>
> Hi Luiz Augusto von Dentz,
>
> On Tue, Jul 18, 2023 at 10:22:26AM -0700, Luiz Augusto von Dentz wrote:
> > Hi Chun-Yi,
> >
> > On Mon, Jul 17, 2023 at 8:43 PM Lee, Chun-Yi <[e-mail]> wrote:
> > >
> > > This change is used to relieve CVE-2020-26555. The description of the
> > > CVE:
> > >
> > > Bluetooth legacy BR/EDR PIN code pairing in Bluetooth Core Specification
> > > 1.0B through 5.2 may permit an unauthenticated nearby device to spoof
> > > the BD_ADDR of the peer device to complete pairing without knowledge
> > > of the PIN. [1]
> >
> > Btw, it is probably worth mentioning that in BR/EDR the key generation
> > is actually handled in the controller, below HCI.
> >
>
> Yes, the key generation be handled by link manager. I will mention it
> in patch description.
>
> > > The detail of this attack is in IEEE paper:
> > > BlueMirror: Reflections on Bluetooth Pairing and Provisioning Protocols
> > > [2]
> > >
> > > It's a reflection attack. Base on the paper, attacker can induce the
> > > attacked target to generate null link key (zero key) without PIN code.
> > >
> > > We can ignore null link key in the handler of "Link Key Notification
> > > event" to relieve the attack. A similar implementation also shows in
> > > btstack project. [3]
> >
> > Perhaps we could clarify this statement by stating that if we ignore
> > the link key it means the stack will not consider the device is bonded
> > and will not persist the link key, that said the controller will still
> > consider it as paired, so I perhaps we should go one step forward and
> > disconnect if we detect such a key is being used.
> >
>
> I am new on bluetooth field. Did you mean like this patch? Sending
> HCI_Disconnect when we found zero link key?
>
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index ff0c331f53d6..3482031cbbb8 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -4698,6 +4700,15 @@ static void hci_link_key_notify_evt(struct hci_dev *hdev, void *data,
>         if (!conn)
>                 goto unlock;
>
> +       /* Ignore NULL link key against CVE-2020-26555 */
> +       if (!memcmp(ev->link_key, ZERO_KEY, HCI_LINK_KEY_SIZE)) {
> +               bt_dev_dbg(hdev, "Ignore NULL link key (ZERO KEY) for %pMR", &ev->bdaddr);
> +               hci_disconnect(conn, HCI_ERROR_AUTH_FAILURE);
> +               hci_conn_drop(conn);
> +               goto unlock;
> +       }

Yeah, something like that should do it, btw I hope you are testing
these changes do actually work properly, even better if you could
introduce a test into the likes of mgmt-tester to generate a ZERO_KEY
so we are not caught by surprise if something doesn't quite work as
expected, or some change cause a regression where this key is accepted
again.

>         hci_conn_hold(conn);
>         conn->disc_timeout = HCI_DISCONN_TIMEOUT;
>         hci_conn_drop(conn);
>
>
> Is there anything I'm missing? Thanks a lot!
>
> > > v2:
> > > - Used Link: tag instead of Closes:
> > > - Used bt_dev_dbg instead of BT_DBG
> > > - Added Fixes: tag
> > >
> > > Fixes: 55ed8ca10f35 ("Bluetooth: Implement link key handling for the management interface")
> > > Link: https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-26555 [1]
> > > Link: https://ieeexplore.ieee.org/abstract/document/9474325/authors#authors [2]
> > > Link: https://github.com/bluekitchen/btstack/blob/master/src/hci.c#L3722 [3]
> > > Signed-off-by: "Lee, Chun-Yi" <[e-mail]>
> > > ---
> > >  net/bluetooth/hci_event.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > > index 95816a938cea..ff0c331f53d6 100644
> > > --- a/net/bluetooth/hci_event.c
> > > +++ b/net/bluetooth/hci_event.c
> > > @@ -4684,6 +4684,12 @@ static void hci_link_key_notify_evt(struct hci_dev *hdev, void *data,
> > >         bool persistent;
> > >         u8 pin_len = 0;
> > >
> > > +       /* Ignore NULL link key against CVE-2020-26555 */
> > > +       if (!memcmp(ev->link_key, ZERO_KEY, HCI_LINK_KEY_SIZE)) {
> > > +               bt_dev_dbg(hdev, "Ignore NULL link key (ZERO KEY) for %pMR", &ev->bdaddr);
> > > +               return;
> > > +       }
> > > +
> > >         bt_dev_dbg(hdev, "");
> > >
> > >         hci_dev_lock(hdev);
> > > --
> > > 2.35.3
> > >
>
> Thanks a lot!
> Joey Lee

Luiz Augusto von Dentz July 27, 2023, 10:29 p.m. UTC | #9

Hi Joeyli,

On Wed, Jul 19, 2023 at 5:25 PM Luiz Augusto von Dentz
<[e-mail]> wrote:
>
> Hi Joeyli,
>
> On Wed, Jul 19, 2023 at 8:49 AM joeyli <[e-mail]> wrote:
> >
> > Hi Luiz Augusto von Dentz,
> >
> > On Tue, Jul 18, 2023 at 10:22:26AM -0700, Luiz Augusto von Dentz wrote:
> > > Hi Chun-Yi,
> > >
> > > On Mon, Jul 17, 2023 at 8:43 PM Lee, Chun-Yi <[e-mail]> wrote:
> > > >
> > > > This change is used to relieve CVE-2020-26555. The description of the
> > > > CVE:
> > > >
> > > > Bluetooth legacy BR/EDR PIN code pairing in Bluetooth Core Specification
> > > > 1.0B through 5.2 may permit an unauthenticated nearby device to spoof
> > > > the BD_ADDR of the peer device to complete pairing without knowledge
> > > > of the PIN. [1]
> > >
> > > Btw, it is probably worth mentioning that in BR/EDR the key generation
> > > is actually handled in the controller, below HCI.
> > >
> >
> > Yes, the key generation be handled by link manager. I will mention it
> > in patch description.
> >
> > > > The detail of this attack is in IEEE paper:
> > > > BlueMirror: Reflections on Bluetooth Pairing and Provisioning Protocols
> > > > [2]
> > > >
> > > > It's a reflection attack. Base on the paper, attacker can induce the
> > > > attacked target to generate null link key (zero key) without PIN code.
> > > >
> > > > We can ignore null link key in the handler of "Link Key Notification
> > > > event" to relieve the attack. A similar implementation also shows in
> > > > btstack project. [3]
> > >
> > > Perhaps we could clarify this statement by stating that if we ignore
> > > the link key it means the stack will not consider the device is bonded
> > > and will not persist the link key, that said the controller will still
> > > consider it as paired, so I perhaps we should go one step forward and
> > > disconnect if we detect such a key is being used.
> > >
> >
> > I am new on bluetooth field. Did you mean like this patch? Sending
> > HCI_Disconnect when we found zero link key?
> >
> > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > index ff0c331f53d6..3482031cbbb8 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -4698,6 +4700,15 @@ static void hci_link_key_notify_evt(struct hci_dev *hdev, void *data,
> >         if (!conn)
> >                 goto unlock;
> >
> > +       /* Ignore NULL link key against CVE-2020-26555 */
> > +       if (!memcmp(ev->link_key, ZERO_KEY, HCI_LINK_KEY_SIZE)) {
> > +               bt_dev_dbg(hdev, "Ignore NULL link key (ZERO KEY) for %pMR", &ev->bdaddr);
> > +               hci_disconnect(conn, HCI_ERROR_AUTH_FAILURE);
> > +               hci_conn_drop(conn);
> > +               goto unlock;
> > +       }
>
> Yeah, something like that should do it, btw I hope you are testing
> these changes do actually work properly, even better if you could
> introduce a test into the likes of mgmt-tester to generate a ZERO_KEY
> so we are not caught by surprise if something doesn't quite work as
> expected, or some change cause a regression where this key is accepted
> again.

Are you still planning on updating these changes so we can apply it?

> >         hci_conn_hold(conn);
> >         conn->disc_timeout = HCI_DISCONN_TIMEOUT;
> >         hci_conn_drop(conn);
> >
> >
> > Is there anything I'm missing? Thanks a lot!
> >
> > > > v2:
> > > > - Used Link: tag instead of Closes:
> > > > - Used bt_dev_dbg instead of BT_DBG
> > > > - Added Fixes: tag
> > > >
> > > > Fixes: 55ed8ca10f35 ("Bluetooth: Implement link key handling for the management interface")
> > > > Link: https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-26555 [1]
> > > > Link: https://ieeexplore.ieee.org/abstract/document/9474325/authors#authors [2]
> > > > Link: https://github.com/bluekitchen/btstack/blob/master/src/hci.c#L3722 [3]
> > > > Signed-off-by: "Lee, Chun-Yi" <[e-mail]>
> > > > ---
> > > >  net/bluetooth/hci_event.c | 6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > >
> > > > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > > > index 95816a938cea..ff0c331f53d6 100644
> > > > --- a/net/bluetooth/hci_event.c
> > > > +++ b/net/bluetooth/hci_event.c
> > > > @@ -4684,6 +4684,12 @@ static void hci_link_key_notify_evt(struct hci_dev *hdev, void *data,
> > > >         bool persistent;
> > > >         u8 pin_len = 0;
> > > >
> > > > +       /* Ignore NULL link key against CVE-2020-26555 */
> > > > +       if (!memcmp(ev->link_key, ZERO_KEY, HCI_LINK_KEY_SIZE)) {
> > > > +               bt_dev_dbg(hdev, "Ignore NULL link key (ZERO KEY) for %pMR", &ev->bdaddr);
> > > > +               return;
> > > > +       }
> > > > +
> > > >         bt_dev_dbg(hdev, "");
> > > >
> > > >         hci_dev_lock(hdev);
> > > > --
> > > > 2.35.3
> > > >
> >
> > Thanks a lot!
> > Joey Lee
>
>
>
> --
> Luiz Augusto von Dentz

joeyli July 28, 2023, 2:48 p.m. UTC | #10

Hi Luiz Augusto von Dentz,

On Thu, Jul 27, 2023 at 03:29:42PM -0700, Luiz Augusto von Dentz wrote:
> Hi Joeyli,
> 
> On Wed, Jul 19, 2023 at 5:25 PM Luiz Augusto von Dentz
> <[e-mail]> wrote:
> >
> > Hi Joeyli,
> >
> > On Wed, Jul 19, 2023 at 8:49 AM joeyli <[e-mail]> wrote:
> > >
> > > Hi Luiz Augusto von Dentz,
> > >
> > > On Tue, Jul 18, 2023 at 10:22:26AM -0700, Luiz Augusto von Dentz wrote:
> > > > Hi Chun-Yi,
> > > >
> > > > On Mon, Jul 17, 2023 at 8:43 PM Lee, Chun-Yi <[e-mail]> wrote:
> > > > >
> > > > > This change is used to relieve CVE-2020-26555. The description of the
> > > > > CVE:
> > > > >
> > > > > Bluetooth legacy BR/EDR PIN code pairing in Bluetooth Core Specification
> > > > > 1.0B through 5.2 may permit an unauthenticated nearby device to spoof
> > > > > the BD_ADDR of the peer device to complete pairing without knowledge
> > > > > of the PIN. [1]
> > > >
> > > > Btw, it is probably worth mentioning that in BR/EDR the key generation
> > > > is actually handled in the controller, below HCI.
> > > >
> > >
> > > Yes, the key generation be handled by link manager. I will mention it
> > > in patch description.
> > >
> > > > > The detail of this attack is in IEEE paper:
> > > > > BlueMirror: Reflections on Bluetooth Pairing and Provisioning Protocols
> > > > > [2]
> > > > >
> > > > > It's a reflection attack. Base on the paper, attacker can induce the
> > > > > attacked target to generate null link key (zero key) without PIN code.
> > > > >
> > > > > We can ignore null link key in the handler of "Link Key Notification
> > > > > event" to relieve the attack. A similar implementation also shows in
> > > > > btstack project. [3]
> > > >
> > > > Perhaps we could clarify this statement by stating that if we ignore
> > > > the link key it means the stack will not consider the device is bonded
> > > > and will not persist the link key, that said the controller will still
> > > > consider it as paired, so I perhaps we should go one step forward and
> > > > disconnect if we detect such a key is being used.
> > > >
> > >
> > > I am new on bluetooth field. Did you mean like this patch? Sending
> > > HCI_Disconnect when we found zero link key?
> > >
> > > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > > index ff0c331f53d6..3482031cbbb8 100644
> > > --- a/net/bluetooth/hci_event.c
> > > +++ b/net/bluetooth/hci_event.c
> > > @@ -4698,6 +4700,15 @@ static void hci_link_key_notify_evt(struct hci_dev *hdev, void *data,
> > >         if (!conn)
> > >                 goto unlock;
> > >
> > > +       /* Ignore NULL link key against CVE-2020-26555 */
> > > +       if (!memcmp(ev->link_key, ZERO_KEY, HCI_LINK_KEY_SIZE)) {
> > > +               bt_dev_dbg(hdev, "Ignore NULL link key (ZERO KEY) for %pMR", &ev->bdaddr);
> > > +               hci_disconnect(conn, HCI_ERROR_AUTH_FAILURE);
> > > +               hci_conn_drop(conn);
> > > +               goto unlock;
> > > +       }
> >
> > Yeah, something like that should do it, btw I hope you are testing
> > these changes do actually work properly, even better if you could
> > introduce a test into the likes of mgmt-tester to generate a ZERO_KEY
> > so we are not caught by surprise if something doesn't quite work as
> > expected, or some change cause a regression where this key is accepted
> > again.
> 
> Are you still planning on updating these changes so we can apply it?
>

Sorry for my delay! I am stucking at other stuff.

I will improve the patch and send new version again.

THanks a lot!
Joey Lee
 
> > >         hci_conn_hold(conn);
> > >         conn->disc_timeout = HCI_DISCONN_TIMEOUT;
> > >         hci_conn_drop(conn);
> > >
> > >
> > > Is there anything I'm missing? Thanks a lot!
> > >
> > > > > v2:
> > > > > - Used Link: tag instead of Closes:
> > > > > - Used bt_dev_dbg instead of BT_DBG
> > > > > - Added Fixes: tag
> > > > >
> > > > > Fixes: 55ed8ca10f35 ("Bluetooth: Implement link key handling for the management interface")
> > > > > Link: https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-26555 [1]
> > > > > Link: https://ieeexplore.ieee.org/abstract/document/9474325/authors#authors [2]
> > > > > Link: https://github.com/bluekitchen/btstack/blob/master/src/hci.c#L3722 [3]
> > > > > Signed-off-by: "Lee, Chun-Yi" <[e-mail]>
> > > > > ---
> > > > >  net/bluetooth/hci_event.c | 6 ++++++
> > > > >  1 file changed, 6 insertions(+)
> > > > >
> > > > > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > > > > index 95816a938cea..ff0c331f53d6 100644
> > > > > --- a/net/bluetooth/hci_event.c
> > > > > +++ b/net/bluetooth/hci_event.c
> > > > > @@ -4684,6 +4684,12 @@ static void hci_link_key_notify_evt(struct hci_dev *hdev, void *data,
> > > > >         bool persistent;
> > > > >         u8 pin_len = 0;
> > > > >
> > > > > +       /* Ignore NULL link key against CVE-2020-26555 */
> > > > > +       if (!memcmp(ev->link_key, ZERO_KEY, HCI_LINK_KEY_SIZE)) {
> > > > > +               bt_dev_dbg(hdev, "Ignore NULL link key (ZERO KEY) for %pMR", &ev->bdaddr);
> > > > > +               return;
> > > > > +       }
> > > > > +
> > > > >         bt_dev_dbg(hdev, "");
> > > > >
> > > > >         hci_dev_lock(hdev);
> > > > > --
> > > > > 2.35.3
> > > > >
> > >
> > > Thanks a lot!
> > > Joey Lee
> >
> >
> >
> > --
> > Luiz Augusto von Dentz
> 
> 
> 
> -- 
> Luiz Augusto von Dentz

Patch

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 95816a938cea..ff0c331f53d6 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -4684,6 +4684,12 @@  static void hci_link_key_notify_evt(struct hci_dev *hdev, void *data,
 	bool persistent;
 	u8 pin_len = 0;
 
+	/* Ignore NULL link key against CVE-2020-26555 */
+	if (!memcmp(ev->link_key, ZERO_KEY, HCI_LINK_KEY_SIZE)) {
+		bt_dev_dbg(hdev, "Ignore NULL link key (ZERO KEY) for %pMR", &ev->bdaddr);
+		return;
+	}
+
 	bt_dev_dbg(hdev, "");
 
 	hci_dev_lock(hdev);