From: Anton Kasimov Subject: Re: Fix memory leaks in ldapd caused by missing ober_free_elements calls To: Theo Buehler Cc: tech@openbsd.org Date: Sun, 11 May 2025 16:45:46 +0200 Hi Theo, Thanks for the review and your feedback. The ldap_free_elements_and_respond() helper was added to avoid repeating the cleanup pattern and to follow the style of ldap_respond(). That said, I understand your preference for inlining, especially since it's only needed in a few places. I've updated the code accordingly. The revised diff is attached. Best regards, Anton Kasimov 10.05.2025 12:31, Theo Buehler пишет: > On Sat, May 10, 2025 at 11:41:25AM +0200, Anton Kasimov wrote: >> Hi tech@, >> >> This patch fixes several memory leaks in ldapd caused by missing calls >> to ober_free_elements after using namespace_get. The namespace_get >> function eventually calls ober_read_element, which allocates memory >> for parsed BER elements. >> >> The most significant leak happens during processing of bind requests >> from non-root users in auth.c. The returned ber_element was not freed >> after being processed. >> >> Similar issues were found and fixed in ldape.c and modify.c. In these >> cases, ber_element returned by namespace_get was also left unfreed. >> >> Additionally, minor cosmetic cleanups were made to simplify some >> NULL checks. Since ober_free_elements safely handles NULL arguments, >> explicit pointer checks before calling it are unnecessary and were >> removed. >> >> Tested with multiple bind and modify operations to confirm no >> regressions or new leaks. >> >> The diff is in the attachment. > Thanks. This all looks correct to me. Just one comment: > >> @@ -205,6 +202,14 @@ ldap_respond(struct request *req, int co >> } >> >> int >> +ldap_free_elements_and_respond(struct ber_element *elm, struct request *req, >> + int code) >> +{ >> + ober_free_elements(elm); >> + return ldap_respond(req, code); >> +} >> + >> +int >> ldap_abandon(struct request *req) >> { >> long long msgid; >> @@ -266,19 +271,22 @@ ldap_compare(struct request *req) >> return ldap_respond(req, LDAP_NO_SUCH_OBJECT); >> >> if ((attr = ldap_find_attribute(entry, at)) == NULL) >> - return ldap_respond(req, LDAP_NO_SUCH_ATTRIBUTE); >> + return ldap_free_elements_and_respond(entry, req, >> + LDAP_NO_SUCH_ATTRIBUTE); > I understand why you added ldap_free_elements_and_respond() but it feels > a bit odd to me. I would prefer not adding it and simply inlining it > the few times you use it, like this: > > if ((attr = ldap_find_attribute(entry, at)) == NULL) { > ober_free_elements(entry); > return ldap_respond(req, LDAP_NO_SUCH_ATTRIBUTE); > } > > if ((attr = attr->be_next) == NULL) { /* skip attribute name */ > ober_free_elements(entry); > return ldap_respond(req, LDAP_OTHER); > } > > etc. Index: auth.c =================================================================== RCS file: /cvs/src/usr.sbin/ldapd/auth.c,v diff -u -p -u -p -r1.15 auth.c --- auth.c 29 Jun 2022 09:10:13 -0000 1.15 +++ auth.c 11 May 2025 14:26:02 -0000 @@ -301,7 +301,7 @@ ldap_auth_simple(struct request *req, ch char *password; char *user_password; struct namespace *ns; - struct ber_element *elm = NULL, *pw = NULL; + struct ber_element *entry = NULL, *elm = NULL, *pw = NULL; if (*binddn == '\0') { free(req->conn->binddn); /* anonymous bind */ @@ -337,15 +337,15 @@ ldap_auth_simple(struct request *req, ch NULL, LDAP_SCOPE_BASE)) return LDAP_INSUFFICIENT_ACCESS; - elm = namespace_get(ns, binddn); - if (elm == NULL && errno == ESTALE) { + entry = namespace_get(ns, binddn); + if (entry == NULL && errno == ESTALE) { if (namespace_queue_request(ns, req) != 0) return LDAP_BUSY; return -1; /* Database is being reopened. */ } - if (elm != NULL) - pw = ldap_get_attribute(elm, "userPassword"); + if (entry != NULL) + pw = ldap_get_attribute(entry, "userPassword"); if (pw != NULL) { for (elm = pw->be_next->be_sub; elm; elm = elm->be_next) { @@ -356,6 +356,7 @@ ldap_auth_simple(struct request *req, ch break; } } + ober_free_elements(entry); } free(req->conn->binddn); Index: ldape.c =================================================================== RCS file: /cvs/src/usr.sbin/ldapd/ldape.c,v diff -u -p -u -p -r1.38 ldape.c --- ldape.c 17 Jan 2024 08:28:15 -0000 1.38 +++ ldape.c 11 May 2025 14:26:02 -0000 @@ -105,8 +105,7 @@ send_ldap_extended_response(struct conn return; fail: - if (root) - ober_free_elements(root); + ober_free_elements(root); } int @@ -180,10 +179,8 @@ ldap_refer(struct request *req, const ch return LDAP_REFERRAL; fail: - if (root != NULL) - ober_free_elements(root); - if (ref_root != NULL) - ober_free_elements(ref_root); + ober_free_elements(root); + ober_free_elements(ref_root); request_free(req); return LDAP_REFERRAL; } @@ -265,19 +262,28 @@ ldap_compare(struct request *req) if ((entry = namespace_get(ns, dn)) == NULL) return ldap_respond(req, LDAP_NO_SUCH_OBJECT); - if ((attr = ldap_find_attribute(entry, at)) == NULL) + if ((attr = ldap_find_attribute(entry, at)) == NULL) { + ober_free_elements(entry); return ldap_respond(req, LDAP_NO_SUCH_ATTRIBUTE); + } - if ((attr = attr->be_next) == NULL) /* skip attribute name */ + if ((attr = attr->be_next) == NULL) { /* skip attribute name */ + ober_free_elements(entry); return ldap_respond(req, LDAP_OTHER); + } for (elm = attr->be_sub; elm != NULL; elm = elm->be_next) { - if (ober_get_string(elm, &s) != 0) + if (ober_get_string(elm, &s) != 0) { + ober_free_elements(entry); return ldap_respond(req, LDAP_OTHER); - if (strcasecmp(value, s) == 0) + } + if (strcasecmp(value, s) == 0) { + ober_free_elements(entry); return ldap_respond(req, LDAP_COMPARE_TRUE); + } } + ober_free_elements(entry); return ldap_respond(req, LDAP_COMPARE_FALSE); } Index: modify.c =================================================================== RCS file: /cvs/src/usr.sbin/ldapd/modify.c,v diff -u -p -u -p -r1.24 modify.c --- modify.c 20 Dec 2021 13:26:11 -0000 1.24 +++ modify.c 11 May 2025 14:26:02 -0000 @@ -36,7 +36,7 @@ ldap_delete(struct request *req) struct namespace *ns; struct referrals *refs; struct cursor *cursor; - struct ber_element *entry, *elm, *a; + struct ber_element *entry = NULL, *elm, *a; int rc = LDAP_OTHER; ++stats.req_mod; @@ -114,6 +114,7 @@ ldap_delete(struct request *req) rc = LDAP_SUCCESS; done: + ober_free_elements(entry); btree_cursor_close(cursor); btval_reset(&key); namespace_abort(ns); @@ -226,8 +227,7 @@ ldap_add(struct request *req) return ldap_respond(req, rc); fail: - if (set != NULL) - ober_free_elements(set); + ober_free_elements(set); namespace_abort(ns); return ldap_respond(req, LDAP_OTHER); } @@ -384,8 +384,8 @@ ldap_modify(struct request *req) rc = LDAP_OTHER; done: - if (vals != NULL) - ober_free_elements(vals); + ober_free_elements(vals); + ober_free_elements(entry); namespace_abort(ns); return ldap_respond(req, rc); }