Skip to contentSkip to conversation

nvme-auth: Don't log shared secret in nvme_auth_dhchap_exponential()

PATCH In review Thorsten Blum 4 participants 10 messages Mar 3 – Apr 24

Subsystems (4)
lore
Share & tools
lore
#0 lore raw

PATCH 1/3 nvme-auth: Don't log shared secret in nvme_auth_dhchap_exponential()

When debug logging is enabled, nvme_auth_dhchap_exponential() logs the DHCHAP shared secret. Remove the log to avoid exposing key material.

Fixes b61775d185a3 ("nvme-auth: Diffie-Hellman key exchange support")
---
 drivers/nvme/host/auth.c | 2 --
 1 file changed, 2 deletions(-)
drivers/nvme/host/auth.c +0 -2
index 405e7c03b1cf..5e4df2ac3cc0 100644
@@ -655,8 +655,6 @@ static int nvme_auth_dhchap_exponential(struct nvme_ctrl *ctrl,
 		chap->status = NVME_AUTH_DHCHAP_FAILURE_INCORRECT_PAYLOAD;
 		return ret;
 	}
-	dev_dbg(ctrl->device, "shared secret %*ph\n",
-		(int)chap->sess_key_len, chap->sess_key);
 	return 0;
 }
 
Signature · 2 lines
Thorsten Blum <[email protected]>
GPG: 1D60 735E 8AEF 3BE4 73B6  9D84 7336 78FD 8DFE EAD4
#1 lore raw

PATCH 2/3 nvmet-auth: Don't log DHCHAP keys in nvmet_setup_auth()

When debug logging is enabled, nvmet_setup_auth() logs the host and controller DHCHAP key bytes. Remove the keys from debug logs to avoid exposing key material.

Fixes db1312dd9548 ("nvmet: implement basic In-Band Authentication")
---
 drivers/nvme/target/auth.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)
drivers/nvme/target/auth.c +4 -6
index 2eadeb7e06f2..f24add0bb86f 100644
@@ -199,10 +199,9 @@ u8 nvmet_setup_auth(struct nvmet_ctrl *ctrl, struct nvmet_sq *sq)
 		ctrl->host_key = NULL;
 		goto out_free_hash;
 	}
-	pr_debug("%s: using hash %s key %*ph\n", __func__,
+	pr_debug("%s: using hash %s\n", __func__,
 		 ctrl->host_key->hash > 0 ?
-		 nvme_auth_hmac_name(ctrl->host_key->hash) : "none",
-		 (int)ctrl->host_key->len, ctrl->host_key->key);
+		 nvme_auth_hmac_name(ctrl->host_key->hash) : "none");
 
 	nvme_auth_free_key(ctrl->ctrl_key);
 	if (!host->dhchap_ctrl_secret) {
@@ -217,10 +216,9 @@ u8 nvmet_setup_auth(struct nvmet_ctrl *ctrl, struct nvmet_sq *sq)
 		ctrl->ctrl_key = NULL;
 		goto out_free_hash;
 	}
-	pr_debug("%s: using ctrl hash %s key %*ph\n", __func__,
+	pr_debug("%s: using ctrl hash %s\n", __func__,
 		 ctrl->ctrl_key->hash > 0 ?
-		 nvme_auth_hmac_name(ctrl->ctrl_key->hash) : "none",
-		 (int)ctrl->ctrl_key->len, ctrl->ctrl_key->key);
+		 nvme_auth_hmac_name(ctrl->ctrl_key->hash) : "none");
 
 out_free_hash:
 	if (ret) {
Signature · 2 lines
Thorsten Blum <[email protected]>
GPG: 1D60 735E 8AEF 3BE4 73B6  9D84 7336 78FD 8DFE EAD4
maintainer
#2 lore raw

Re: [PATCH 2/3] nvmet-auth: Don't log DHCHAP keys in nvmet_setup_auth()

Quoted 31 lines · Thorsten Blum

When debug logging is enabled, nvmet_setup_auth() logs the host and controller DHCHAP key bytes. Remove the keys from debug logs to avoid exposing key material.

Fixes db1312dd9548 ("nvmet: implement basic In-Band Authentication")
---
  drivers/nvme/target/auth.c | 10 ++++------
  1 file changed, 4 insertions(+), 6 deletions(-)
drivers/nvme/target/auth.c +2 -3
index 2eadeb7e06f2..f24add0bb86f 100644
@@ -199,10 +199,9 @@ u8 nvmet_setup_auth(struct nvmet_ctrl *ctrl, struct nvmet_sq *sq)
  		ctrl->host_key = NULL;
  		goto out_free_hash;
  	}
-	pr_debug("%s: using hash %s key %*ph\n", __func__,
+	pr_debug("%s: using hash %s\n", __func__,
  		 ctrl->host_key->hash > 0 ?
-		 nvme_auth_hmac_name(ctrl->host_key->hash) : "none",
-		 (int)ctrl->host_key->len, ctrl->host_key->key);
+		 nvme_auth_hmac_name(ctrl->host_key->hash) : "none");
 
  	nvme_auth_free_key(ctrl->ctrl_key);
  	if (!host->dhchap_ctrl_secret) {
@@ -217,10 +216,9 @@ u8 nvmet_setup_auth(struct nvmet_ctrl *ctrl, struct nvmet_sq *sq)
  		ctrl->ctrl_key = NULL;
  		goto out_free_hash;

drivers/nvme/target/auth.c u8 nvmet_setup_auth(struct nvmet_ctrl *ctrl, struct nvmet_sq *sq) · line 224

Quoted 10 lines
drivers/nvme/target/auth.c +2 -3
  	}
-	pr_debug("%s: using ctrl hash %s key %*ph\n", __func__,
+	pr_debug("%s: using ctrl hash %s\n", __func__,
  		 ctrl->ctrl_key->hash > 0 ?
-		 nvme_auth_hmac_name(ctrl->ctrl_key->hash) : "none",
-		 (int)ctrl->ctrl_key->len, ctrl->ctrl_key->key);
+		 nvme_auth_hmac_name(ctrl->ctrl_key->hash) : "none");
 
  out_free_hash:
  	if (ret) {

Without the key the pr_debug calls are pretty much pointless anyway, so you might want to remove them, too.

However, these debug prints really help when trying to figure out authentication failures.
I think it would be better to add a compile-time option to disable these outputs entirely.

I'll send a patch.

Cheers,

Hannes

Signature · 4 lines
Dr. Hannes Reinecke                  Kernel Storage Architect
[email protected]                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
#3 lore raw

Re: [PATCH 2/3] nvmet-auth: Don't log DHCHAP keys in nvmet_setup_auth()

Quoted 44 lines · Hannes Reinecke
Quoted 29 lines · Thorsten Blum

When debug logging is enabled, nvmet_setup_auth() logs the host and controller DHCHAP key bytes. Remove the keys from debug logs to avoid exposing key material.

Fixes db1312dd9548 ("nvmet: implement basic In-Band Authentication")
---
  drivers/nvme/target/auth.c | 10 ++++------
  1 file changed, 4 insertions(+), 6 deletions(-)
drivers/nvme/target/auth.c +2 -3
index 2eadeb7e06f2..f24add0bb86f 100644
@@ -199,10 +199,9 @@ u8 nvmet_setup_auth(struct nvmet_ctrl *ctrl, struct nvmet_sq *sq)
  		ctrl->host_key = NULL;
  		goto out_free_hash;
  	}
-	pr_debug("%s: using hash %s key %*ph\n", __func__,
+	pr_debug("%s: using hash %s\n", __func__,
  		 ctrl->host_key->hash > 0 ?
-		 nvme_auth_hmac_name(ctrl->host_key->hash) : "none",
-		 (int)ctrl->host_key->len, ctrl->host_key->key);
+		 nvme_auth_hmac_name(ctrl->host_key->hash) : "none");
  	nvme_auth_free_key(ctrl->ctrl_key);
  	if (!host->dhchap_ctrl_secret) {
@@ -217,10 +216,9 @@ u8 nvmet_setup_auth(struct nvmet_ctrl *ctrl, struct nvmet_sq *sq)
  		ctrl->ctrl_key = NULL;

drivers/nvme/target/auth.c u8 nvmet_setup_auth(struct nvmet_ctrl *ctrl, struct nvmet_sq *sq) · line 223

Quoted 10 lines
drivers/nvme/target/auth.c +2 -3
  		goto out_free_hash;
  	}
-	pr_debug("%s: using ctrl hash %s key %*ph\n", __func__,
+	pr_debug("%s: using ctrl hash %s\n", __func__,
  		 ctrl->ctrl_key->hash > 0 ?
-		 nvme_auth_hmac_name(ctrl->ctrl_key->hash) : "none",
-		 (int)ctrl->ctrl_key->len, ctrl->ctrl_key->key);
+		 nvme_auth_hmac_name(ctrl->ctrl_key->hash) : "none");
  out_free_hash:
  	if (ret) {

Without the key the pr_debug calls are pretty much pointless anyway, so you might want to remove them, too.

Quoted 6 lines

However, these debug prints really help when trying to figure out authentication failures.
I think it would be better to add a compile-time option to disable these outputs entirely.

I'll send a patch.

Did you ever send a patch? I couldn't find anything. The code hasn't changed either and the keys are still logged (same for patch 3/3).

Thanks,
Thorsten

#4 lore raw

PATCH 3/3 nvmet-auth: Don't log DHCHAP shared secret in nvmet_auth_ctrl_sesskey()

In reply to Thorsten Blum

When debug logging is enabled, nvmet_auth_ctrl_sesskey() logs the DHCHAP shared secret. Remove the log to avoid exposing key material.

Fixes 7a277c37d352 ("nvmet-auth: Diffie-Hellman key exchange support")
---
 drivers/nvme/target/auth.c | 4 ----
 1 file changed, 4 deletions(-)
drivers/nvme/target/auth.c +0 -4
index f24add0bb86f..f62fed6bd897 100644
@@ -544,10 +544,6 @@ int nvmet_auth_ctrl_sesskey(struct nvmet_req *req,
 					  req->sq->dhchap_skey_len);
 	if (ret)
 		pr_debug("failed to compute shared secret, err %d\n", ret);
-	else
-		pr_debug("%s: shared secret %*ph\n", __func__,
-			 (int)req->sq->dhchap_skey_len,
-			 req->sq->dhchap_skey);
 
 	return ret;
 }
Signature · 2 lines
Thorsten Blum <[email protected]>
GPG: 1D60 735E 8AEF 3BE4 73B6  9D84 7336 78FD 8DFE EAD4
maintainer
#5 lore raw

Re: [PATCH 3/3] nvmet-auth: Don't log DHCHAP shared secret in nvmet_auth_ctrl_sesskey()

Quoted 15 lines · Thorsten Blum

When debug logging is enabled, nvmet_auth_ctrl_sesskey() logs the DHCHAP shared secret. Remove the log to avoid exposing key material.

Fixes 7a277c37d352 ("nvmet-auth: Diffie-Hellman key exchange support")
---
  drivers/nvme/target/auth.c | 4 ----
  1 file changed, 4 deletions(-)
drivers/nvme/target/auth.c +0 -0
index f24add0bb86f..f62fed6bd897 100644

drivers/nvme/target/auth.c int nvmet_auth_ctrl_sesskey(struct nvmet_req *req, · line 549

Quoted 10 lines
drivers/nvme/target/auth.c +0 -4
@@ -544,10 +544,6 @@ int nvmet_auth_ctrl_sesskey(struct nvmet_req *req,
  					  req->sq->dhchap_skey_len);
  	if (ret)
  		pr_debug("failed to compute shared secret, err %d\n", ret);
-	else
-		pr_debug("%s: shared secret %*ph\n", __func__,
-			 (int)req->sq->dhchap_skey_len,
-			 req->sq->dhchap_skey);
 
  	return ret;
  }

As indicated in the previous patch, we should use a compile time option to disable the messages.

Cheers,

Hannes

Signature · 4 lines
Dr. Hannes Reinecke                  Kernel Storage Architect
[email protected]                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
maintainer Reviewed-by on v1 1/3
#6 lore raw
In reply to Thorsten Blum
Quoted 15 lines · Thorsten Blum

When debug logging is enabled, nvme_auth_dhchap_exponential() logs the DHCHAP shared secret. Remove the log to avoid exposing key material.

Fixes b61775d185a3 ("nvme-auth: Diffie-Hellman key exchange support")
---
  drivers/nvme/host/auth.c | 2 --
  1 file changed, 2 deletions(-)
drivers/nvme/host/auth.c +0 -0
index 405e7c03b1cf..5e4df2ac3cc0 100644

drivers/nvme/host/auth.c static int nvme_auth_dhchap_exponential(struct nvme_ctrl *ctrl, · line 660

Quoted 8 lines
drivers/nvme/host/auth.c +0 -2
@@ -655,8 +655,6 @@ static int nvme_auth_dhchap_exponential(struct nvme_ctrl *ctrl,
  		chap->status = NVME_AUTH_DHCHAP_FAILURE_INCORRECT_PAYLOAD;
  		return ret;
  	}
-	dev_dbg(ctrl->device, "shared secret %*ph\n",
-		(int)chap->sess_key_len, chap->sess_key);
  	return 0;
  }
 

Yeah, that was primarily for debugging.

Cheers,

Hannes

Signature · 4 lines
Dr. Hannes Reinecke                  Kernel Storage Architect
[email protected]                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
#7 lore raw
Quoted 26 lines · Hannes Reinecke
Quoted 15 lines · Thorsten Blum

When debug logging is enabled, nvme_auth_dhchap_exponential() logs the DHCHAP shared secret. Remove the log to avoid exposing key material.

Fixes b61775d185a3 ("nvme-auth: Diffie-Hellman key exchange support")
---
  drivers/nvme/host/auth.c | 2 --
  1 file changed, 2 deletions(-)
drivers/nvme/host/auth.c +0 -0
index 405e7c03b1cf..5e4df2ac3cc0 100644

drivers/nvme/host/auth.c static int nvme_auth_dhchap_exponential(struct nvme_ctrl *ctrl, · line 659

Quoted 7 lines
drivers/nvme/host/auth.c +0 -2
@@ -655,8 +655,6 @@ static int nvme_auth_dhchap_exponential(struct nvme_ctrl *ctrl,
  		chap->status = NVME_AUTH_DHCHAP_FAILURE_INCORRECT_PAYLOAD;
  		return ret;
  	}
-	dev_dbg(ctrl->device, "shared secret %*ph\n",
-		(int)chap->sess_key_len, chap->sess_key);
  	return 0;
  }

Yeah, that was primarily for debugging.

Quoted 1 line

This was never applied, was it?

Thanks,
Thorsten