Attachment 16553 Details for Bug 14571 – patch backported to 4.12 (v4)

[patch] patch backported to 4.12 (v4)

CVE-2021-20254-sids_to_unixids-for-4.12-v4.patch (text/plain), 7.35 KB, created by Andrew Bartlett on 2021-03-16 20:43:21 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Andrew Bartlett

Created: 2021-03-16 20:43:21 UTC

Size: 7.35 KB

patch

obsolete

>From 7996ef0a063e1440684819afad4d380002317de5 Mon Sep 17 00:00:00 2001
>From: Volker Lendecke <vl@samba.org>
>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 <vl@samba.org>
>Reviewed-by: Jeremy Allison <jra@samba.org>
>
>(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<num_not_cached; i++) {
>+	for (i=0; i<wbc_ids_size; i++) {
> 		wbc_ids[i].type = WBC_ID_TYPE_NOT_SPECIFIED;
> 	}
>-	err = wbcSidsToUnixIds(wbc_sids, num_not_cached, wbc_ids);
>+	/*
>+	 * gcc versions at least 5.x -> 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<wbc_ids_size; i++) {
>+		wbc_ids[i].id.gid = (uint32_t)-1;
>+	}
>+	err = wbcSidsToUnixIds(wbc_sids, wbc_ids_size, wbc_ids);
> 	if (!WBC_ERROR_IS_OK(err)) {
> 		DEBUG(10, ("wbcSidsToUnixIds returned %s\n",
> 			   wbcErrorString(err)));
> 	}
> 
>+	/*
>+	 * Second time through the SID array, replace
>+	 * the ids[] entries that wbcSidsToUnixIds() was able to
>+	 * map.
>+	 *
>+	 * Use bitmap_set() to mark an ids[] array entry as
>+	 * being mapped.
>+	 */
>+
> 	num_not_cached = 0;
> 
> 	for (i=0; i<num_sids; i++) {
>-		if (ids[i].type == ID_TYPE_NOT_SPECIFIED) {
>-			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;
>-				break;
>-			case WBC_ID_TYPE_GID:
>-				ids[i].type = ID_TYPE_GID;
>-				ids[i].id = wbc_ids[num_not_cached].id.gid;
>-				break;
>-			default:
>-				/* The types match, and wbcUnixId -> 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<num_sids; i++) {
>-		if (ids[i].type != ID_TYPE_NOT_SPECIFIED) {
>+		if (bitmap_query(found, i)) {
> 			continue;
> 		}
> 		if (legacy_sid_to_gid(&sids[i], &ids[i].id)) {
> 			ids[i].type = ID_TYPE_GID;
>+			bitmap_set(found, i);
> 			continue;
> 		}
> 		if (legacy_sid_to_uid(&sids[i], &ids[i].id)) {
> 			ids[i].type = ID_TYPE_UID;
>+			bitmap_set(found, i);
> 			continue;
> 		}
> 	}
> done:
>+	/*
>+	 * Pass through the return array for consistency.
>+	 * Any ids[].id mapped to (uint32_t)-1 must be returned
>+	 * as ID_TYPE_NOT_SPECIFIED.
>+	 */
> 	for (i=0; i<num_sids; i++) {
> 		switch(ids[i].type) {
> 		case WBC_ID_TYPE_GID:
> 		case WBC_ID_TYPE_UID:
> 		case WBC_ID_TYPE_BOTH:
>-			if (ids[i].id == -1) {
>+			if (ids[i].id == (uint32_t)-1) {
> 				ids[i].type = ID_TYPE_NOT_SPECIFIED;
> 			}
> 			break;
>-- 
>2.25.1
>

Flags:

jra: review+
abartlet: ci-passed+

Actions: View

Attachments on bug 14571: 16407 | 16410 | 16466 | 16467 | 16499 | 16505 | 16506 | 16507 | 16508 | 16509 | 16510 | 16511 | 16516 | 16517 | 16519 | 16520 | 16532 | 16533 | 16534 | 16535 | 16536 | 16537 | 16538 | 16539 | 16542 | 16543 | 16544 | 16545 | 16546 | 16548 | 16551 | 16553 | 16595