[1/2] Bluetooth: hci_event: Fix using memcmp when comparing keys

Commit Message

Luiz Augusto von Dentz Oct. 5, 2023, 9:23 p.m. UTC

From: Luiz Augusto von Dentz <[e-mail]>

memcmp is not consider safe to use with cryptographic secrets:

 'Do  not  use memcmp() to compare security critical data, such as
 cryptographic secrets, because the required CPU time depends on the
 number of equal bytes.'

While usage of memcmp for ZERO_KEY may not be considered a security
critical data, it can lead to more usage of memcmp with pairing keys
which could introduce more security problems.

Fixes: fe7a9da4fa54 ("Bluetooth: hci_event: Ignore NULL link key")
Signed-off-by: Luiz Augusto von Dentz <[e-mail]>
---
 net/bluetooth/hci_event.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

Comments

[e-mail] Oct. 5, 2023, 9:47 p.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=790428

---Test result---

Test Summary:
CheckPatch                    PASS      1.49 seconds
GitLint                       FAIL      0.98 seconds
SubjectPrefix                 PASS      0.26 seconds
BuildKernel                   PASS      35.31 seconds
CheckAllWarning               PASS      37.84 seconds
CheckSparse                   WARNING   43.53 seconds
CheckSmatch                   WARNING   116.97 seconds
BuildKernel32                 PASS      33.67 seconds
TestRunnerSetup               FAIL      41.23 seconds
TestRunner_l2cap-tester       FAIL      0.14 seconds
TestRunner_iso-tester         FAIL      0.14 seconds
TestRunner_bnep-tester        FAIL      0.14 seconds
TestRunner_mgmt-tester        FAIL      0.14 seconds
TestRunner_rfcomm-tester      FAIL      0.14 seconds
TestRunner_sco-tester         FAIL      0.13 seconds
TestRunner_ioctl-tester       FAIL      0.13 seconds
TestRunner_mesh-tester        FAIL      0.14 seconds
TestRunner_smp-tester         FAIL      0.13 seconds
TestRunner_userchan-tester    FAIL      0.14 seconds
IncrementalBuild              PASS      44.86 seconds

Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[2/2] Bluetooth: hci_event: Fix coding style

WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
8: B3 Line contains hard tab characters (\t): "+	if (!bacmp(&hdev->bdaddr, &ev->bdaddr))"
9: B3 Line contains hard tab characters (\t): "+	{"
##############################
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):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):net/bluetooth/hci_event.c: note: in included file (through include/net/bluetooth/hci_core.h):
##############################
Test: TestRunnerSetup - FAIL
Desc: Setup kernel and bluez for test-runner
Output:
Bluez: 
make[1]: *** No rule to make target 'unit/test-micp.c', needed by 'unit/test-micp.o'.  Stop.
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:4582: all] Error 2
##############################
Test: TestRunner_l2cap-tester - FAIL
Desc: Run l2cap-tester with test-runner
Output:
No tester found
##############################
Test: TestRunner_iso-tester - FAIL
Desc: Run iso-tester with test-runner
Output:
No tester found
##############################
Test: TestRunner_bnep-tester - FAIL
Desc: Run bnep-tester with test-runner
Output:
No tester found
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
No tester found
##############################
Test: TestRunner_rfcomm-tester - FAIL
Desc: Run rfcomm-tester with test-runner
Output:
No tester found
##############################
Test: TestRunner_sco-tester - FAIL
Desc: Run sco-tester with test-runner
Output:
No tester found
##############################
Test: TestRunner_ioctl-tester - FAIL
Desc: Run ioctl-tester with test-runner
Output:
No tester found
##############################
Test: TestRunner_mesh-tester - FAIL
Desc: Run mesh-tester with test-runner
Output:
No tester found
##############################
Test: TestRunner_smp-tester - FAIL
Desc: Run smp-tester with test-runner
Output:
No tester found
##############################
Test: TestRunner_userchan-tester - FAIL
Desc: Run userchan-tester with test-runner
Output:
No tester found


---
Regards,
Linux Bluetooth

[e-mail] Oct. 7, 2023, 1 a.m. UTC | #2

Hello:

This series was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <[e-mail]>:

On Thu,  5 Oct 2023 14:23:21 -0700 you wrote:
> From: Luiz Augusto von Dentz <[e-mail]>
> 
> memcmp is not consider safe to use with cryptographic secrets:
> 
>  'Do  not  use memcmp() to compare security critical data, such as
>  cryptographic secrets, because the required CPU time depends on the
>  number of equal bytes.'
> 
> [...]

Here is the summary with links:
  - [1/2] Bluetooth: hci_event: Fix using memcmp when comparing keys
    https://git.kernel.org/bluetooth/bluetooth-next/c/0ce9e7726323
  - [2/2] Bluetooth: hci_event: Fix coding style
    https://git.kernel.org/bluetooth/bluetooth-next/c/f1b0dea09e3e

You are awesome, thank you!

Patch

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 43ed691d0d90..d9c1bfb3082f 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -26,6 +26,8 @@ 
 /* Bluetooth HCI event handling. */
 
 #include <asm/unaligned.h>
+#include <linux/crypto.h>
+#include <crypto/algapi.h>
 
 #include <net/bluetooth/bluetooth.h>
 #include <net/bluetooth/hci_core.h>
@@ -4754,7 +4756,7 @@  static void hci_link_key_notify_evt(struct hci_dev *hdev, void *data,
 		goto unlock;
 
 	/* Ignore NULL link key against CVE-2020-26555 */
-	if (!memcmp(ev->link_key, ZERO_KEY, HCI_LINK_KEY_SIZE)) {
+	if (!crypto_memneq(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);
@@ -5294,8 +5296,8 @@  static u8 bredr_oob_data_present(struct hci_conn *conn)
 		 * available, then do not declare that OOB data is
 		 * present.
 		 */
-		if (!memcmp(data->rand256, ZERO_KEY, 16) ||
-		    !memcmp(data->hash256, ZERO_KEY, 16))
+		if (!crypto_memneq(data->rand256, ZERO_KEY, 16) ||
+		    !crypto_memneq(data->hash256, ZERO_KEY, 16))
 			return 0x00;
 
 		return 0x02;
@@ -5305,8 +5307,8 @@  static u8 bredr_oob_data_present(struct hci_conn *conn)
 	 * not supported by the hardware, then check that if
 	 * P-192 data values are present.
 	 */
-	if (!memcmp(data->rand192, ZERO_KEY, 16) ||
-	    !memcmp(data->hash192, ZERO_KEY, 16))
+	if (!crypto_memneq(data->rand192, ZERO_KEY, 16) ||
+	    !crypto_memneq(data->hash192, ZERO_KEY, 16))
 		return 0x00;
 
 	return 0x01;