From 7996ef0a063e1440684819afad4d380002317de5 Mon Sep 17 00:00:00 2001 From: Volker Lendecke Date: Sat, 20 Feb 2021 15:50:12 +0100 Subject: [PATCH] CVE-2021-20254 passdb: Simplify sids_to_unixids() Best reviewed with "git show -b", there's a "continue" statement that changes subsequent indentation. Decouple lookup status of ids from ID_TYPE_NOT_SPECIFIED Bug: https://bugzilla.samba.org/show_bug.cgi?id=14571 Signed-off-by: Volker Lendecke Reviewed-by: Jeremy Allison (backported from patch from master) [backport by npower@samba.org as master commit 493f5d6b078e0b0f80d1ef25043e2834cb4fcb87 and 58e9b62222ad62c81cdf11d704859a227cb2902b creates conflicts due to rename of WBC_ID_TYPE_* -> ID_TYPE_*] [backport by jra@samba.org to work around a compiler bug showing this error on gcc 5.6 -> 6.x, seen on Debian 9 and Ubuntu 16.04 under -O3: ../../source3/passdb/lookup_sid.c:1246:6: error: assuming pointer wraparound does not occur when comparing P +- C1 with P +- C2 [-Werror=strict-overflow]] --- source3/passdb/lookup_sid.c | 140 ++++++++++++++++++++++++++++++------ 1 file changed, 118 insertions(+), 22 deletions(-) diff --git a/source3/passdb/lookup_sid.c b/source3/passdb/lookup_sid.c index 82c47b3145b..6887b6ebb62 100644 --- a/source3/passdb/lookup_sid.c +++ b/source3/passdb/lookup_sid.c @@ -29,6 +29,7 @@ #include "../libcli/security/security.h" #include "lib/winbind_util.h" #include "../librpc/gen_ndr/idmap.h" +#include "lib/util/bitmap.h" static bool lookup_unix_user_name(const char *name, struct dom_sid *sid) { @@ -1247,7 +1248,9 @@ bool sids_to_unixids(const struct dom_sid *sids, uint32_t num_sids, { struct wbcDomainSid *wbc_sids = NULL; struct wbcUnixId *wbc_ids = NULL; + struct bitmap *found = NULL; uint32_t i, num_not_cached; + uint32_t wbc_ids_size = 0; wbcErr err; bool ret = false; @@ -1255,6 +1258,20 @@ bool sids_to_unixids(const struct dom_sid *sids, uint32_t num_sids, if (wbc_sids == NULL) { return false; } + found = bitmap_talloc(wbc_sids, num_sids); + if (found == NULL) { + goto fail; + } + + /* + * We go through the requested SID array three times. + * First time to look for global_sid_Unix_Users + * and global_sid_Unix_Groups SIDS, and to look + * for mappings cached in the idmap_cache. + * + * Use bitmap_set() to mark an ids[] array entry as + * being mapped. + */ num_not_cached = 0; @@ -1266,17 +1283,20 @@ bool sids_to_unixids(const struct dom_sid *sids, uint32_t num_sids, &sids[i], &rid)) { ids[i].type = ID_TYPE_UID; ids[i].id = rid; + bitmap_set(found, i); continue; } if (sid_peek_check_rid(&global_sid_Unix_Groups, &sids[i], &rid)) { ids[i].type = ID_TYPE_GID; ids[i].id = rid; + bitmap_set(found, i); continue; } if (idmap_cache_find_sid2unixid(&sids[i], &ids[i], &expired) && !expired) { + bitmap_set(found, i); continue; } ids[i].type = ID_TYPE_NOT_SPECIFIED; @@ -1287,62 +1307,138 @@ bool sids_to_unixids(const struct dom_sid *sids, uint32_t num_sids, if (num_not_cached == 0) { goto done; } - wbc_ids = talloc_array(talloc_tos(), struct wbcUnixId, num_not_cached); + + /* + * For the ones that we couldn't map in the loop above, query winbindd + * via wbcSidsToUnixIds(). + */ + + wbc_ids_size = num_not_cached; + wbc_ids = talloc_array(talloc_tos(), struct wbcUnixId, wbc_ids_size); if (wbc_ids == NULL) { goto fail; } - for (i=0; i 6.x have a bug + * with initializing the wbc_ids[i].type and + * wbc_ids[i].id.gid to (uint32_t)-1 in the + * same loop when using [-Werror=strict-overflow]. + * The error is: + * + * ../../source3/passdb/lookup_sid.c:1246:6: error: assuming + * pointer wraparound does not occur when comparing P +- C1 + * with P +- C2 [-Werror=strict-overflow] + * + * Doing this loop twice fixes the compiler error. + * As wbc_ids_size isn't large this isn't too much + * of a performance burden. + */ + for (i=0; i id is a union anyway */ - ids[i].type = (enum id_type)wbc_ids[num_not_cached].type; - ids[i].id = wbc_ids[num_not_cached].id.gid; - break; - } - num_not_cached += 1; + if (bitmap_query(found, i)) { + continue; } + + SMB_ASSERT(num_not_cached < wbc_ids_size); + + switch (wbc_ids[num_not_cached].type) { + case WBC_ID_TYPE_UID: + ids[i].type = ID_TYPE_UID; + ids[i].id = wbc_ids[num_not_cached].id.uid; + bitmap_set(found, i); + break; + case WBC_ID_TYPE_GID: + ids[i].type = ID_TYPE_GID; + ids[i].id = wbc_ids[num_not_cached].id.gid; + bitmap_set(found, i); + break; + case WBC_ID_TYPE_BOTH: + ids[i].type = ID_TYPE_BOTH; + ids[i].id = wbc_ids[num_not_cached].id.uid; + bitmap_set(found, i); + break; + case WBC_ID_TYPE_NOT_SPECIFIED: + /* + * wbcSidsToUnixIds() wasn't able to map this + * so we still need to check legacy_sid_to_XXX() + * below. Don't mark the bitmap entry + * as being found so the final loop knows + * to try and map this entry. + */ + ids[i].type = ID_TYPE_NOT_SPECIFIED; + ids[i].id = (uint32_t)-1; + break; + default: + /* + * A successful return from wbcSidsToUnixIds() + * cannot return anything other than the values + * checked for above. Ensure this is so. + */ + smb_panic(__location__); + break; + } + num_not_cached += 1; } + /* + * Third and final time through the SID array, + * try legacy_sid_to_gid()/legacy_sid_to_uid() + * for entries we haven't already been able to + * map. + * + * Use bitmap_set() to mark an ids[] array entry as + * being mapped. + */ + for (i=0; i