From cf82e80b4418c69521c48cb005ecd386d5b8c177 Mon Sep 17 00:00:00 2001 From: Alexey Tikhonov Date: Mon, 10 Feb 2025 16:12:49 +0100 Subject: [PATCH] Resolves: RHEL-78061 - 'sssd_kcm' leaks memory --- 0001-KCM-fix-memory-leak.patch | 113 +++++++++++++++++++++++++++++++++ sssd.spec | 7 +- 2 files changed, 118 insertions(+), 2 deletions(-) create mode 100644 0001-KCM-fix-memory-leak.patch diff --git a/0001-KCM-fix-memory-leak.patch b/0001-KCM-fix-memory-leak.patch new file mode 100644 index 0000000..e62c69b --- /dev/null +++ b/0001-KCM-fix-memory-leak.patch @@ -0,0 +1,113 @@ +From 50f703f25914254d2a545f52f504dfa5a6f65546 Mon Sep 17 00:00:00 2001 +From: Alexey Tikhonov +Date: Tue, 4 Feb 2025 18:59:36 +0100 +Subject: [PATCH] KCM: fix memory leak +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +The copy of 'secret' argument - `secret_val.data` - was left hanging +on `sss_sec_ctx`, effectively resulting in a memory leak. +But this copy isn't actually required as this data isn't modified in +below operations. +Skipping alloc+memcpy+erase is also beneficial performance wise. + +:fixes:'sssd_kcm' memory leak was fixed. + +Reviewed-by: Alejandro López +Reviewed-by: Justin Stephenson +(cherry picked from commit 7f1b7c9689827df92e8b2166423d4e80688dbacb) +--- + src/responder/kcm/secrets/secrets.c | 34 ++++++++++------------------- + 1 file changed, 12 insertions(+), 22 deletions(-) + +diff --git a/src/responder/kcm/secrets/secrets.c b/src/responder/kcm/secrets/secrets.c +index 625a09f39..fe7410cb3 100644 +--- a/src/responder/kcm/secrets/secrets.c ++++ b/src/responder/kcm/secrets/secrets.c +@@ -979,7 +979,7 @@ errno_t sss_sec_put(struct sss_sec_req *req, + size_t secret_len) + { + struct ldb_message *msg; +- struct ldb_val secret_val = { .data = NULL }; ++ const struct ldb_val secret_val = { .length = secret_len, .data = secret }; + bool erase_msg = false; + int ret; + +@@ -1029,13 +1029,11 @@ errno_t sss_sec_put(struct sss_sec_req *req, + goto done; + } + +- secret_val.length = secret_len; +- secret_val.data = talloc_memdup(req->sctx, secret, secret_len); +- if (!secret_val.data) { +- ret = ENOMEM; +- goto done; +- } +- ++ /* `ldb_msg_add_value()` does NOT make a copy of secret_val::*data ++ * but rather copies a pointer under the hood. ++ * This is fine since no operations modifying this data are performed ++ * below and 'msg' is freed before function returns. ++ */ + ret = ldb_msg_add_value(msg, SEC_ATTR_SECRET, &secret_val, NULL); + if (ret != EOK) { + DEBUG(SSSDBG_OP_FAILURE, +@@ -1069,9 +1067,6 @@ errno_t sss_sec_put(struct sss_sec_req *req, + + ret = EOK; + done: +- if (secret_val.data != NULL) { +- sss_erase_mem_securely(secret_val.data, secret_val.length); +- } + if (erase_msg) { + db_result_erase_message_securely(msg, SEC_ATTR_SECRET); + } +@@ -1084,7 +1079,7 @@ errno_t sss_sec_update(struct sss_sec_req *req, + size_t secret_len) + { + struct ldb_message *msg; +- struct ldb_val secret_val = { .data = NULL }; ++ const struct ldb_val secret_val = { .length = secret_len, .data = secret }; + bool erase_msg = false; + int ret; + +@@ -1134,13 +1129,6 @@ errno_t sss_sec_update(struct sss_sec_req *req, + goto done; + } + +- secret_val.length = secret_len; +- secret_val.data = talloc_memdup(req->sctx, secret, secret_len); +- if (!secret_val.data) { +- ret = ENOMEM; +- goto done; +- } +- + /* FIXME - should we have a lastUpdate timestamp? */ + ret = ldb_msg_add_empty(msg, SEC_ATTR_SECRET, LDB_FLAG_MOD_REPLACE, NULL); + if (ret != LDB_SUCCESS) { +@@ -1150,6 +1138,11 @@ errno_t sss_sec_update(struct sss_sec_req *req, + goto done; + } + ++ /* `ldb_msg_add_value()` does NOT make a copy of secret_val::*data ++ * but rather copies a pointer under the hood. ++ * This is fine since no operations modifying this data are performed ++ * below and 'msg' is freed before function returns. ++ */ + ret = ldb_msg_add_value(msg, SEC_ATTR_SECRET, &secret_val, NULL); + if (ret != LDB_SUCCESS) { + DEBUG(SSSDBG_MINOR_FAILURE, +@@ -1174,9 +1167,6 @@ errno_t sss_sec_update(struct sss_sec_req *req, + + ret = EOK; + done: +- if (secret_val.data != NULL) { +- sss_erase_mem_securely(secret_val.data, secret_val.length); +- } + if (erase_msg) { + db_result_erase_message_securely(msg, SEC_ATTR_SECRET); + } +-- +2.47.0 + diff --git a/sssd.spec b/sssd.spec index 48f0b1f..d02cb47 100644 --- a/sssd.spec +++ b/sssd.spec @@ -57,7 +57,7 @@ Name: sssd Version: 2.10.2 -Release: 1%{?dist} +Release: 2%{?dist} Summary: System Security Services Daemon License: GPL-3.0-or-later URL: https://github.com/SSSD/sssd/ @@ -65,7 +65,7 @@ Source0: https://github.com/SSSD/sssd/releases/download/2.10.2/sssd-2.10.2.tar.g Source1: sssd.sysusers ### Patches ### -# Patch0001: +Patch0001: 0001-KCM-fix-memory-leak.patch ### Dependencies ### @@ -1116,6 +1116,9 @@ fi %systemd_postun_with_restart sssd.service %changelog +* Mon Feb 10 2025 Alexey Tikhonov - 2.10.2-2 +- Resolves: RHEL-78061 - 'sssd_kcm' leaks memory + * Wed Jan 29 2025 Alexey Tikhonov - 2.10.2-1 - Resolves: RHEL-62725 - Rebase SSSD for RHEL 10.0