[PATCH 0/4] smb: server: Clear sensitive data before freeing it

[PATCH 0/4] smb: server: Clear sensitive data before freeing it

[Thomas Huth]

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.

Disclaimer: The spots that need clearing have been identified with AI,
but the patches have been created manually (for double-checking whether
the findings really make sense). Anyway, I'm not very familiar with the
smb code, so please review carefully. Thanks!

Thomas Huth (4):
  smb: server: Clear sensitive stack and heap data in auth.c
  smb: server: Make sure that passkey is not leaked on the heap in
    user_config.c
  smb: server: Free session data with kfree_sensitive() to avoid leaking
    of data
  smb: server: Free sensitive connection data with kfree_sensitive()

 fs/smb/server/auth.c              | 11 +++++++++--
 fs/smb/server/connection.c        |  4 ++--
 fs/smb/server/mgmt/user_config.c  |  6 +++---
 fs/smb/server/mgmt/user_session.c |  4 ++--
 4 files changed, 16 insertions(+), 9 deletions(-)

-- 
2.55.0

[PATCH 1/4] smb: server: Clear sensitive stack and heap data in auth.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, or via the heap when using
only normal kfree() functions. There is no known vulnaribility for this
right now, but it's good security style to explicitly zeroize this
sensitive matieral as soon as possible to avoid that it could be exploited
together with other bugs later.

In calc_ntlmv2_hash(), the struct hmac_md5_ctx is normally cleared during
hmac_md5_final() already, but in case of errors, this function is skipped
and ctx is never zeroized, so add a memzero_explicit(&ctx, sizeof(ctx))
there to fix the problem.

In ksmbd_krb5_authenticate(), the ksmbd_spnego_authen_response contains
the session key in the payload. It's currently freed with plain kvfree().
Let's better use kvfree_sensitive() instead.

In generate_key(), the prfhash[] array is used to calculate the key,
but it's never cleared, so it leaks on the stack. Thus clear this with
a memzero_explicit(), too.

In ksmbd_crypt_message(), the sign[] and key[] arrays are leaked via
the stack, too. Make sure to clear them via memzero_explicit() at the
end.

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

diff --git a/fs/smb/server/auth.c b/fs/smb/server/auth.c
index 4e7b6f0e6b8cd..998b40c35e97c 100644
--- a/fs/smb/server/auth.c
+++ b/fs/smb/server/auth.c
@@ -122,6 +122,8 @@ static int calc_ntlmv2_hash(struct ksmbd_conn *conn, struct ksmbd_session *sess,
 out:
 	kfree(uniname);
 	kfree(domain);
+	if (ret)	/* Done by hmac_md5_final() already if ret == 0 */
+		memzero_explicit(&ctx, sizeof(ctx));
 	return ret;
 }
 
@@ -464,7 +466,7 @@ int ksmbd_krb5_authenticate(struct ksmbd_session *sess, char *in_blob,
 	*out_len = resp->spnego_blob_len;
 	retval = 0;
 out:
-	kvfree(resp);
+	kvfree_sensitive(resp, sizeof(*resp));
 	return retval;
 }
 #else
@@ -556,6 +558,7 @@ static void generate_key(struct ksmbd_conn *conn, const char *sess_key,
 
 	hmac_sha256_final(&ctx, prfhash);
 	memcpy(key, prfhash, key_size);
+	memzero_explicit(prfhash, sizeof(prfhash));
 }
 
 static int generate_smb3signingkey(struct ksmbd_session *sess,
@@ -848,7 +851,8 @@ int ksmbd_crypt_message(struct ksmbd_work *work, struct kvec *iov,
 		ctx = ksmbd_crypto_ctx_find_ccm();
 	if (!ctx) {
 		pr_err("crypto alloc failed\n");
-		return -ENOMEM;
+		rc = -ENOMEM;
+		goto zeroize_key;
 	}
 
 	if (conn->cipher_type == SMB2_ENCRYPTION_AES128_GCM ||
@@ -928,5 +932,8 @@ int ksmbd_crypt_message(struct ksmbd_work *work, struct kvec *iov,
 	aead_request_free(req);
 free_ctx:
 	ksmbd_release_crypto_ctx(ctx);
+zeroize_key:
+	memzero_explicit(key, sizeof(key));
+	memzero_explicit(sign, sizeof(sign));
 	return rc;
 }
-- 
2.55.0

[PATCH 2/4] smb: server: Make sure that passkey is not leaked on the heap in user_config.c

[Thomas Huth]

From: Thomas Huth <thuth@redhat.com>

Use kfree_sensitive() to free the user->passkey (and the struct
ksmbd_login_response in ksmbd_login_user() that contains the same
information) to avoid that this information could leak somewhere
else via the heap.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 fs/smb/server/mgmt/user_config.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/smb/server/mgmt/user_config.c b/fs/smb/server/mgmt/user_config.c
index cf45841d9d1b9..76b74d68369d8 100644
--- a/fs/smb/server/mgmt/user_config.c
+++ b/fs/smb/server/mgmt/user_config.c
@@ -27,7 +27,7 @@ struct ksmbd_user *ksmbd_login_user(const char *account)
 
 	user = ksmbd_alloc_user(resp, resp_ext);
 out:
-	kvfree(resp);
+	kvfree_sensitive(resp, sizeof(*resp));
 	return user;
 }
 
@@ -70,7 +70,7 @@ struct ksmbd_user *ksmbd_alloc_user(struct ksmbd_login_response *resp,
 
 err_free:
 	kfree(user->name);
-	kfree(user->passkey);
+	kfree_sensitive(user->passkey);
 	kfree(user);
 	return NULL;
 }
@@ -80,7 +80,7 @@ void ksmbd_free_user(struct ksmbd_user *user)
 	ksmbd_ipc_logout_request(user->name, user->flags);
 	kfree(user->sgid);
 	kfree(user->name);
-	kfree(user->passkey);
+	kfree_sensitive(user->passkey);
 	kfree(user);
 }
 
-- 
2.55.0

[PATCH 3/4] smb: server: Free session data with kfree_sensitive() to avoid leaking of data

[Thomas Huth]

From: Thomas Huth <thuth@redhat.com>

struct ksmbd_session contains some arrays with sensitive information, like
sess_key, smb3encryptionkey, smb3decryptionkey and smb3signingkey. Thus
let's make sure that this information cannot leak via the heap and use
kfree_sensitive() to free it.

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

diff --git a/fs/smb/server/mgmt/user_session.c b/fs/smb/server/mgmt/user_session.c
index f99c86284ba3d..9cb990a22fd56 100644
--- a/fs/smb/server/mgmt/user_session.c
+++ b/fs/smb/server/mgmt/user_session.c
@@ -389,10 +389,10 @@ void ksmbd_session_destroy(struct ksmbd_session *sess)
 	ksmbd_launch_ksmbd_durable_scavenger();
 	ksmbd_session_rpc_clear_list(sess);
 	free_channel_list(sess);
-	kfree(sess->Preauth_HashValue);
+	kfree_sensitive(sess->Preauth_HashValue);
 	ksmbd_release_id(&session_ida, sess->id);
 	ida_destroy(&sess->tree_conn_ida);
-	kfree(sess);
+	kfree_sensitive(sess);
 }
 
 struct ksmbd_session *__session_lookup(unsigned long long id)
-- 
2.55.0

[PATCH 4/4] smb: server: Free sensitive connection data with kfree_sensitive()

[Thomas Huth]

From: Thomas Huth <thuth@redhat.com>

struct ksmbd_conn contains an embedded struct ntlmssp_auth with the
ciphertext[] and cryptkey[] arrays, so to avoid leaking this information
via the heap, it should be freed with kfree_sensitive().

While we're at it, also use kfree_sensitive() for freeing preauth_info
in ksmbd_conn_free() to avoid that the Preauth_HashValue[] could leak
via the heap here, too.

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

diff --git a/fs/smb/server/connection.c b/fs/smb/server/connection.c
index ef6f202f4024c..bcb688c9af918 100644
--- a/fs/smb/server/connection.c
+++ b/fs/smb/server/connection.c
@@ -117,7 +117,7 @@ static void __ksmbd_conn_release_work(struct work_struct *work)
 
 	ida_destroy(&conn->async_ida);
 	conn->transport->ops->free_transport(conn->transport);
-	kfree(conn);
+	kfree_sensitive(conn);
 }
 
 /**
@@ -183,7 +183,7 @@ void ksmbd_conn_free(struct ksmbd_conn *conn)
 	 */
 	xa_destroy(&conn->sessions);
 	kvfree(conn->request_buf);
-	kfree(conn->preauth_info);
+	kfree_sensitive(conn->preauth_info);
 	kfree(conn->mechToken);
 	ksmbd_conn_put(conn);
 }
-- 
2.55.0