From c8ec6e58c64086fe8852f2b022ad8807c7197182 Mon Sep 17 00:00:00 2001 From: Mark Reynolds Date: Oct 09 2018 18:51:13 +0000 Subject: Ticket 49969 - DOS caused by malformed search operation (security fix) Bug Description: There are two issues here. The one in we don't cloase a connection when an invalid unbind occurs. The other is a search request passing 8MB of NULL bytes as search attributes will keep one thread busy for a long time. The reason is that the attr array is copied/normalized to the searchattrs in the search operation and does this using charray_add() which iterates thru the array to determine the size of the array and then allocate one element more. So this means we iterate 8 million times an array with a then average size of 4 million elements. Fix Description: We already have traversed the array once and know the size, so we can allocate the needed size once and only copy the element. In addition we check for the kind of degenerated attributes "" as used in this test scenario. So the fix will reject invalid attr lists and improve performance for valid ones Author: Ludwig Krispens https://pagure.io/389-ds-base/issue/49969 Reviewed by: tbordaz & mreynolds (Thanks!) (cherry picked from commit a49bd03d6a6debe15be1a70d7109712fba691b8b) --- diff --git a/ldap/servers/slapd/search.c b/ldap/servers/slapd/search.c index 731c651..dc26fc4 100644 --- a/ldap/servers/slapd/search.c +++ b/ldap/servers/slapd/search.c @@ -209,6 +209,7 @@ do_search(Slapi_PBlock *pb) if (attrs != NULL) { char *normaci = slapi_attr_syntax_normalize("aci"); int replace_aci = 0; + int attr_count = 0; if (!normaci) { normaci = slapi_ch_strdup("aci"); } else if (strcasecmp(normaci, "aci")) { @@ -218,9 +219,19 @@ do_search(Slapi_PBlock *pb) /* * . store gerattrs if any * . add "aci" once if "*" is given + * . check that attrs are not degenerated */ for (i = 0; attrs[i] != NULL; i++) { char *p = NULL; + attr_count++; + + if ( attrs[i][0] == '\0') { + log_search_access(pb, base, scope, fstr, "invalid attribute request"); + send_ldap_result(pb, LDAP_PROTOCOL_ERROR, NULL, NULL, 0, NULL); + slapi_ch_free_string(&normaci); + goto free_and_return; + } + /* check if @ is included */ p = strchr(attrs[i], '@'); if (p) { @@ -244,6 +255,7 @@ do_search(Slapi_PBlock *pb) } else if (strcmp(attrs[i], LDAP_ALL_USER_ATTRS /* '*' */) == 0) { if (!charray_inlist(attrs, normaci)) { charray_add(&attrs, slapi_ch_strdup(normaci)); + attr_count++; } } else if (replace_aci && (strcasecmp(attrs[i], "aci") == 0)) { slapi_ch_free_string(&attrs[i]); @@ -263,13 +275,13 @@ do_search(Slapi_PBlock *pb) } } else { /* return the chopped type, e.g., "sn" */ - operation->o_searchattrs = NULL; + operation->o_searchattrs = (char **)slapi_ch_calloc(sizeof(char *), attr_count+1); for (i = 0; attrs[i] != NULL; i++) { char *type; type = slapi_attr_syntax_normalize_ext(attrs[i], ATTR_SYNTAX_NORM_ORIG_ATTR); /* attrs[i] is consumed */ - charray_add(&operation->o_searchattrs, attrs[i]); + operation->o_searchattrs[i] = attrs[i]; attrs[i] = type; } } diff --git a/ldap/servers/slapd/unbind.c b/ldap/servers/slapd/unbind.c index 90f7b15..686e27a 100644 --- a/ldap/servers/slapd/unbind.c +++ b/ldap/servers/slapd/unbind.c @@ -87,8 +87,8 @@ do_unbind(Slapi_PBlock *pb) /* pass the unbind to all backends */ be_unbindall(pb_conn, operation); +free_and_return:; + /* close the connection to the client */ disconnect_server(pb_conn, operation->o_connid, operation->o_opid, SLAPD_DISCONNECT_UNBIND, 0); - -free_and_return:; }