[PATCH 0/5] smb: client: Avoid leaking of sensitive data to the stack or heap

[PATCH 0/5] smb: client: Avoid leaking of sensitive data to the stack or heap

[Thomas Huth]

Similar to the patch series that I sent for the server code recently,
here's a clean-up series for the client part:

Sensitive data like keys that are stored in stack-local arrays could be
leaked via the stack to the calling functions, or via the heap when using
only normal kfree() functions.

It's good security practice to clear sensitive data on the stack first
with memzero_explicit() before leaving the context, and to use
kfree_sensitive() for data that is returned to the heap.

Thomas Huth (5):
  smb: client: Clear sensitive stack data in smb2transport.c
  smb: client: Clear sensitive stack and heap data in smb2ops.c
  smb: client: Clear sensitive stack data in cifsencrypt.c
  smb: client: Clear sensitive stack data in smb1encrypt.c
  smb: client: Avoid leaking sensitive data to the heap in connect.c

 fs/smb/client/cifsencrypt.c   | 12 +++++++++---
 fs/smb/client/connect.c       |  2 +-
 fs/smb/client/smb1encrypt.c   | 19 +++++++++----------
 fs/smb/client/smb2ops.c       |  4 ++--
 fs/smb/client/smb2transport.c |  4 ++++
 5 files changed, 25 insertions(+), 16 deletions(-)

-- 
2.55.0

[PATCH 1/5] smb: client: Clear sensitive stack data in smb2transport.c

[Thomas Huth]

From: Thomas Huth <thuth@redhat.com>

Sensitive data like keys that are stored in stack-local arrays could
be leaked via the stack to the calling functions. There is no known
vulnerability for this right now, but it's good security style to
explicitly zeroize this sensitive material as soon as possible to
avoid that it could be exploited together with other bugs later.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 fs/smb/client/smb2transport.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/smb/client/smb2transport.c b/fs/smb/client/smb2transport.c
index 1143ee52470a7..fdc634d99da03 100644
--- a/fs/smb/client/smb2transport.c
+++ b/fs/smb/client/smb2transport.c
@@ -249,6 +249,8 @@ smb2_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info *server)
 	if (!rc)
 		memcpy(shdr->Signature, smb2_signature, SMB2_SIGNATURE_SIZE);
 
+	memzero_explicit(key, sizeof(key));
+	memzero_explicit(&hmac_ctx, sizeof(hmac_ctx));
 	return rc;
 }
 
@@ -283,6 +285,7 @@ static void generate_key(struct cifs_ses *ses, struct kvec label,
 	hmac_sha256_final(&hmac_ctx, prfhash);
 
 	memcpy(key, prfhash, key_size);
+	memzero_explicit(prfhash, sizeof(prfhash));
 }
 
 struct derivation {
@@ -482,6 +485,7 @@ smb3_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info *server)
 	memset(shdr->Signature, 0x0, SMB2_SIGNATURE_SIZE);
 
 	rc = aes_cmac_preparekey(&cmac_key, key, SMB2_CMACAES_SIZE);
+	memzero_explicit(key, sizeof(key));
 	if (rc) {
 		cifs_server_dbg(VFS, "%s: Could not set key for cmac aes\n", __func__);
 		return rc;
-- 
2.55.0

[PATCH 2/5] smb: client: Clear sensitive stack and heap data in smb2ops.c

[Thomas Huth]

From: Thomas Huth <thuth@redhat.com>

Make sure to not leak key-related data via the heap or the stack
by using kfree_sensitive() or memzero_explicit() here.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 fs/smb/client/smb2ops.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
index 192649fec25d5..a3099a0e5c2be 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -1569,7 +1569,7 @@ SMB2_request_res_key(const unsigned int xid, struct cifs_tcon *tcon,
 	memcpy(pcchunk->SourceKey, res_key->ResumeKey, COPY_CHUNK_RES_KEY_SIZE);
 
 req_res_key_exit:
-	kfree(res_key);
+	kfree_sensitive(res_key);
 	return rc;
 }
 
@@ -4636,7 +4636,7 @@ crypt_message(struct TCP_Server_Info *server, int num_rqst,
 		rc = crypto_aead_setkey(tfm, key, SMB3_GCM256_CRYPTKEY_SIZE);
 	else
 		rc = crypto_aead_setkey(tfm, key, SMB3_GCM128_CRYPTKEY_SIZE);
-
+	memzero_explicit(key, sizeof(key));
 	if (rc) {
 		cifs_server_dbg(VFS, "%s: Failed to set aead key %d\n", __func__, rc);
 		return rc;
-- 
2.55.0

[PATCH 3/5] smb: client: Clear sensitive stack data in cifsencrypt.c

[Thomas Huth]

From: Thomas Huth <thuth@redhat.com>

Make sure to not leak hash data via the stack, clear it
with memzero_explicit() before leaving the function.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 fs/smb/client/cifsencrypt.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/fs/smb/client/cifsencrypt.c b/fs/smb/client/cifsencrypt.c
index 34804e9842a80..71a2a59123f57 100644
--- a/fs/smb/client/cifsencrypt.c
+++ b/fs/smb/client/cifsencrypt.c
@@ -249,12 +249,13 @@ static int calc_ntlmv2_hash(struct cifs_ses *ses, char *ntlmv2_hash,
 	E_md4hash(ses->password, nt_hash, nls_cp);
 
 	hmac_md5_init_usingrawkey(&hmac_ctx, nt_hash, CIFS_NTHASH_SIZE);
+	memzero_explicit(nt_hash, sizeof(nt_hash));
 
 	/* convert ses->user_name to unicode */
 	len = ses->user_name ? strlen(ses->user_name) : 0;
 	user = kmalloc(2 + (len * 2), GFP_KERNEL);
 	if (user == NULL)
-		return -ENOMEM;
+		goto out_nomem;
 
 	if (len) {
 		len = cifs_strtoUTF16(user, ses->user_name, len, nls_cp);
@@ -272,7 +273,7 @@ static int calc_ntlmv2_hash(struct cifs_ses *ses, char *ntlmv2_hash,
 
 		domain = kmalloc(2 + (len * 2), GFP_KERNEL);
 		if (domain == NULL)
-			return -ENOMEM;
+			goto out_nomem;
 
 		len = cifs_strtoUTF16((__le16 *)domain, ses->domainName, len,
 				      nls_cp);
@@ -284,7 +285,7 @@ static int calc_ntlmv2_hash(struct cifs_ses *ses, char *ntlmv2_hash,
 
 		server = kmalloc(2 + (len * 2), GFP_KERNEL);
 		if (server == NULL)
-			return -ENOMEM;
+			goto out_nomem;
 
 		len = cifs_strtoUTF16((__le16 *)server, ses->ip_addr, len, nls_cp);
 		hmac_md5_update(&hmac_ctx, (const u8 *)server, 2 * len);
@@ -293,6 +294,10 @@ static int calc_ntlmv2_hash(struct cifs_ses *ses, char *ntlmv2_hash,
 
 	hmac_md5_final(&hmac_ctx, ntlmv2_hash);
 	return 0;
+
+out_nomem:
+	memzero_explicit(&hmac_ctx, sizeof(hmac_ctx));
+	return -ENOMEM;
 }
 
 static void CalcNTLMv2_response(const struct cifs_ses *ses, char *ntlmv2_hash)
@@ -463,6 +468,7 @@ setup_ntlmv2_rsp(struct cifs_ses *ses, const struct nls_table *nls_cp)
 	rc = 0;
 unlock:
 	cifs_server_unlock(ses->server);
+	memzero_explicit(ntlmv2_hash, sizeof(ntlmv2_hash));
 setup_ntlmv2_rsp_ret:
 	kfree_sensitive(tiblob);
 
-- 
2.55.0

[PATCH 4/5] smb: client: Clear sensitive stack data in smb1encrypt.c

[Thomas Huth]

From: Thomas Huth <thuth@redhat.com>

Make sure to not leak signature data via the stack, clear it
with memzero_explicit() before leaving the function.

To avoid that we have to introduce "goto"-cleanup here, we re-arrange
the code a little bit (and drop the commented cifs_dump_mem debug
code that looks like a leftover from very early days).

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 fs/smb/client/smb1encrypt.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/fs/smb/client/smb1encrypt.c b/fs/smb/client/smb1encrypt.c
index bf10fdeeedcab..c9eb68f04e7b0 100644
--- a/fs/smb/client/smb1encrypt.c
+++ b/fs/smb/client/smb1encrypt.c
@@ -81,6 +81,7 @@ int cifs_sign_rqst(struct smb_rqst *rqst, struct TCP_Server_Info *server,
 	else
 		memcpy(cifs_pdu->Signature.SecuritySignature, smb_signature, 8);
 
+	memzero_explicit(smb_signature, sizeof(smb_signature));
 	return rc;
 }
 
@@ -126,15 +127,13 @@ int cifs_verify_signature(struct smb_rqst *rqst,
 	rc = cifs_calc_signature(rqst, server, what_we_think_sig_should_be);
 	cifs_server_unlock(server);
 
-	if (rc)
-		return rc;
-
-/*	cifs_dump_mem("what we think it should be: ",
-		      what_we_think_sig_should_be, 16); */
-
-	if (crypto_memneq(server_response_sig, what_we_think_sig_should_be, 8))
-		return -EACCES;
-	else
-		return 0;
+	if (!rc) {
+		if (crypto_memneq(server_response_sig,
+				  what_we_think_sig_should_be, 8))
+			rc = -EACCES;
+	}
 
+	memzero_explicit(what_we_think_sig_should_be,
+			 sizeof(what_we_think_sig_should_be));
+	return rc;
 }
-- 
2.55.0

[PATCH 5/5] smb: client: Avoid leaking sensitive data to the heap in connect.c

[Thomas Huth]

From: Thomas Huth <thuth@redhat.com>

TCP_Server_Info contains a preauth_sha_hash[] and a cryptkey[] array
that might contain sensitive data. Thus free its memory with
kfree_sensitive() to avoid that we are leaking this information to
the heap.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 fs/smb/client/connect.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index ba749ec25a59f..5675d7fc2c818 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -1143,7 +1143,7 @@ clean_demultiplex_info(struct TCP_Server_Info *server)
 	put_net(cifs_net_ns(server));
 	kfree(server->leaf_fullpath);
 	kfree(server->hostname);
-	kfree(server);
+	kfree_sensitive(server);
 
 	length = atomic_dec_return(&tcpSesAllocCount);
 	if (length > 0)
-- 
2.55.0