[PATCH] Fix wbinfo --sids-to-unix_ids with some unmapped sids

[Volker Lendecke]

On Fri, Dec 21, 2018 at 04:41:17PM +0100, Stefan Metzmacher wrote:
> Hi Volker,
> 
> > We have to reply all we have if some SIDs fail to map.
> > 
> > Review appreciated!
> 
> I'd like to have this, but somethings are not clear to me.
> 
> Is this an wbinfo only thing?

That is not wbinfo only.

> Or is smbd also affected?

smbd is also affected.

> Do we need backports?

This only popped up at a customer site. Nobody public has complained
yet. So it depends if you think that many people are affected.

> Please don't use S-1-5-1000 for the test, it's really
> a bug (a different one) that lookup_wellknown_sid() doesn't handle
> it. I'll see if I can come up with a fix for that... (we already handle
> it in dom_sid_lookup_predefined_name() and we need to unify the code)

Which one do you know for 100% sure will never be mapped? At my
customer site this is the one that caused trouble. I thought that in
principle idmap tdb maps everything and I was happy that I found one
that is not mappable.

> The commit message title:
> "idmap: In --sids-to-unix-ids, pass back what we have"
> is a bit misleading, I first thought it's a patch to wbinfo.c,
> can you replace "--sids-to-unix-ids" by _wbint_Sids2UnixIDs().

Attached.

Volker

-- 
Besuchen Sie die verinice.XP 2019 in Berlin!
Anwenderkonferenz für Informationssicherheit
26.-28. Februar 2019 - im Hotel Radisson Blu
Info & Anmeldung hier: http://veriniceXP.org

SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:kontakt at sernet.de
-------------- next part --------------
From 55bee045284a0e99349e9e6d74559aa913cba516 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Tue, 18 Dec 2018 15:53:21 +0100
Subject: [PATCH 1/3] idmap_tdb: If one SID fails to map, try the rest

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 source3/winbindd/idmap_tdb_common.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/source3/winbindd/idmap_tdb_common.c b/source3/winbindd/idmap_tdb_common.c
index e130be08245..34269e3fe56 100644
--- a/source3/winbindd/idmap_tdb_common.c
+++ b/source3/winbindd/idmap_tdb_common.c
@@ -577,8 +577,11 @@ static NTSTATUS idmap_tdb_common_sids_to_unixids_action(struct db_context *db,
 			ret =
 			    idmap_tdb_common_new_mapping(state->dom,
 							 state->ids[i]);
+			DBG_DEBUG("idmap_tdb_common_new_mapping returned %s\n",
+				  nt_errstr(ret));
 			if (!NT_STATUS_IS_OK(ret)) {
-				goto done;
+				ret = STATUS_SOME_UNMAPPED;
+				continue;
 			}
 			num_mapped += 1;
 		}
-- 
2.11.0


From 612fae86b22fd19b1400b19aa4f3af0a0dd8e5fb Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Tue, 18 Dec 2018 15:53:48 +0100
Subject: [PATCH 2/3] idmap: In _wbint_Sids2UnixIDs, pass back what we have

SOME_UNMAPPED does not mean that nothing worthwhile is in here. We
need to pass what we have.

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 source3/winbindd/winbindd_dual_srv.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/source3/winbindd/winbindd_dual_srv.c b/source3/winbindd/winbindd_dual_srv.c
index 62224bf313e..ab14f5d51a0 100644
--- a/source3/winbindd/winbindd_dual_srv.c
+++ b/source3/winbindd/winbindd_dual_srv.c
@@ -204,6 +204,15 @@ NTSTATUS _wbint_Sids2UnixIDs(struct pipes_struct *p,
 
 	status = dom->methods->sids_to_unixids(dom, id_map_ptrs);
 
+	if (NT_STATUS_EQUAL(status, STATUS_SOME_UNMAPPED)) {
+		/*
+		 * This is okay. We need to transfer the mapped ones
+		 * up to our caller. The individual mappings carry the
+		 * information whether they are mapped or not.
+		 */
+		status = NT_STATUS_OK;
+	}
+
 	if (!NT_STATUS_IS_OK(status)) {
 		DEBUG(10, ("sids_to_unixids returned %s\n",
 			   nt_errstr(status)));
-- 
2.11.0


From 3cd0c0ca5331138157db833d898286180930e7ef Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Fri, 21 Dec 2018 11:49:33 +0100
Subject: [PATCH 3/3] selftest: Test sids-to-xids with one failing sid

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 nsswitch/tests/test_wbinfo_sids_to_xids.sh | 13 +++++++++++++
 source3/selftest/tests.py                  |  6 ++++++
 2 files changed, 19 insertions(+)
 create mode 100755 nsswitch/tests/test_wbinfo_sids_to_xids.sh

diff --git a/nsswitch/tests/test_wbinfo_sids_to_xids.sh b/nsswitch/tests/test_wbinfo_sids_to_xids.sh
new file mode 100755
index 00000000000..2e3aeaa477a
--- /dev/null
+++ b/nsswitch/tests/test_wbinfo_sids_to_xids.sh
@@ -0,0 +1,13 @@
+#!/bin/sh
+
+incdir=`dirname $0`/../../testprogs/blackbox
+. $incdir/subunit.sh
+
+# S-1-5-1000 fails, but S-1-5-11 succeeds. Check that S-1-5-11 is
+# mapped successfully with a GID in the 1000x range
+
+testit_grep "wbinfo" "gid 1000" \
+	    $VALGRIND $BINDIR/wbinfo --sids-to-unix-ids=s-1-5-1000,s-1-5-11 ||
+    failed=`expr $failed + 1`
+
+testok $0 $failed
diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
index a3bb1c4feac..d531856e9b7 100755
--- a/source3/selftest/tests.py
+++ b/source3/selftest/tests.py
@@ -262,6 +262,12 @@ plantestsuite("samba3.wbinfo_user_info", env,
                             "nsswitch/tests/test_wbinfo_user_info.sh"),
                '$TRUST_DOMAIN', '$TRUST_REALM', '$DOMAIN', 'alice', 'alice', 'jane', 'jane.doe', env])
 
+plantestsuite(
+    "samba3.wbinfo_sids_to_xids",
+    "nt4_member",
+    [os.path.join(srcdir(),
+                  "nsswitch/tests/test_wbinfo_sids_to_xids.sh")])
+
 env = "ad_member"
 t = "WBCLIENT-MULTI-PING"
 plantestsuite("samba3.smbtorture_s3.%s" % t, env, [os.path.join(samba3srcdir, "script/tests/test_smbtorture_s3.sh"), t, '//foo/bar', '""', '""', smbtorture3, ""])
-- 
2.11.0