[autobackport: sssd-2-9] memberOf plugin :: mbof_add_operation() optimizations#8507
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to improve performance by introducing several optimizations to mbof_add_operation() in the memberOf plugin, including replacing ldb_dn operations with string-based functions, optimizing array element removal, and removing a redundant check. However, a critical security vulnerability has been identified: the new helper functions, sss_get_linearized_dn_from_ldb_val and sss_linearized_dn_match, incorrectly assume that data within ldb_val structures is NULL-terminated. This assumption, when combined with functions like strstr and strcasecmp, can lead to buffer over-reads, potentially causing process crashes (Denial of Service) or information disclosure. It is crucial to address this by using length-limited memory functions and respecting the length field of the ldb_val structure.
| if (data[0] == '<') { | ||
| const char *p_save = data; | ||
| const char *p = data; | ||
| do { | ||
| p_save = p; | ||
| p = strstr(p, ">;"); | ||
| if (p) { | ||
| p = p + 2; | ||
| } | ||
| } while (p); |
There was a problem hiding this comment.
The function sss_get_linearized_dn_from_ldb_val incorrectly assumes that the input ldb_val data is NULL-terminated, which is not guaranteed by the LDB API. Using strstr on strdn->data without respecting the ldb_val's length field can lead to a buffer over-read if the value is not null-terminated within its allocated length. This vulnerability is propagated to sss_linearized_dn_match and can result in a process crash (Denial of Service) or information disclosure. It is essential to use length-aware functions like memmem instead of strstr and memcmp or strncasecmp with verified lengths, always respecting the length field of the ldb_val structure.
There was a problem hiding this comment.
This is the same code as in ldb_dn_from_ldb_val() that was used previously
There was a problem hiding this comment.
Well... actually, original code has a check:
(strnlen((const char*)strdn->data, strdn->length) != strdn->length)) {
/* The RDN must not contain a character with value 0x0 */
return NULL;
but it didn't protect against non-NULL terminated strings.
It was other way around - to protect against strings containing 0x0 in the payload.
There was a problem hiding this comment.
\note the data is null (0x00) terminated, but the length does not
include the terminator.
*/
struct ldb_val {
uint8_t *data; /*!< result data */
size_t length; /*!< length of data */
};
c06e5b8 to
22f5a7a
Compare
|
Had to add |
'msg->dn' (== 'addop->entry_dn') is already filtered out from 'parents->dns[i]' at the beginning of `mbof_add_operation()` Reviewed-by: Iker Pedrosa <[email protected]> Reviewed-by: Sumit Bose <[email protected]> (cherry picked from commit c1eced6)
when removing a duplicate. DNs order doesn't matter. Reviewed-by: Iker Pedrosa <[email protected]> Reviewed-by: Sumit Bose <[email protected]> (cherry picked from commit 7a7480e)
`ldb_dn_compare()` here is heavy because when DNs are not equal (vast majority of cases), it performs `ldb_dn_casefold_internal()` to return -1 or 1, but it's not important in this context. Note that in general using `str*cmp()` instead of `ldb_dn_get_linearized()` might yield incorrect results due to different DN representations. But since all 'memberof' DNs originate from sysdb cache / memberof plugin itself, it should be safe replacement in this context. Reviewed-by: Iker Pedrosa <[email protected]> Reviewed-by: Sumit Bose <[email protected]> (cherry picked from commit 704c31d)
|
The pull request was accepted by @alexey-tikhonov with the following PR CI status: 🟢 CodeQL (success) There are unsuccessful or unfinished checks. Make sure that the failures are not related to this pull request before merging. |
22f5a7a to
ba83ef9
Compare
4 participants
This is an automatic backport of PR#8464 memberOf plugin :: mbof_add_operation() optimizations to branch sssd-2-9, created by @alexey-tikhonov.
Please make sure this backport is correct.
Note
The commits were cherry-picked without conflicts.
You can push changes to this pull request
Original commits
c1eced6 - memberOf plugin: redundant comparison removed
7a7480e - memberOf plugin: swap instead of a shift
704c31d - memberOf plugin: avoid
ldb_dn_compare()inmbof_add_operation()Backported commits
ldb_dn_compare()inmbof_add_operation()Original Pull Request Body
Using the same test case as described in #8454 and testing on top of that PR:
tu1andtu2are members of the same 5k LDAP groups (RFC2307 case, no nested groups)time id [email protected] | tr ',' '\n' | wc -land thentime id [email protected] | tr ',' '\n' | wc -lare executedThis patch set reduces time spent by
id tu1from 27 to 8 seconds and by consequentid tu2from 23 to 5 seconds on my laptop.