[PATCH] central range check for sids2xids

[Michael Adam]

On 2016-08-10 at 09:25 +0200, Andreas Schneider wrote:
> On Tuesday, 9 August 2016 18:39:36 CEST Michael Adam wrote:
> > Hi all,
> > 
> > The attached patch introduces a central range check
> > for the unix ids produced by the id mapping backends
> > (sids2xids).
> > 
> > I noticed that some backends (at least ad and hash),
> > have no range check any more. This is dangerous
> > because it can lead to ids leaking out of id-mapping
> > that are from ranges that this backend is not
> > responsible for the backward mapping xids2sids
> > would then lead to a different sid than the one
> > started with.
> > 
> > Instead of adding this to all backends, here is
> > a patch that adds the check to the central
> > winbind code.
> > 
> > Opinions?
> 
> I missed that mail yesterday. Normally a bug should be opened before we create 
> the master patch so the bug URL is already present. We need this backported!
> 
> 
> Please open a bug for that. Thanks!

Bug is here: https://bugzilla.samba.org/show_bug.cgi?id=12155

The minimal patch that fixes this in the central place
is attached. The problem with the original patch was that
the idmap_unix_id_is_in_range() function unconditionally
excluded xid == 0. But in the case of a passdb idmap backend
(with implicit range 0-0 / unset), this uid must actually
currently be allowed (AD setup). Hence adapting the
in_range function first.

So this patch implements a centra patch but still
puts it into the idmap child, not in the parent as
Volker had suggested.

If we need a minimal bugfix patch now, I'd say this
is it. I'm currently working on follow-up patches
that will clean up an refactor.

First, with the central check, the range checks
in all the backends can be removed.

Then some refactoring of the idmap code in winbindd_dual_srv.c .
Also some correction of treatment of status SOME_UNMAPPED.

And last, i'm thinking about how to best move the
checks (and some idmap logic) into the parent.
But that will be the most extensive reconstructions.

Cheers - Michael
-------------- next part --------------
From d5fa991ed8deb59e8422c2eaeb7982194dceb964 Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox at samba.org>
Date: Mon, 15 Aug 2016 23:07:33 +0200
Subject: [PATCH 1/2] idmap: don't generally forbid id==0 from
 idmap_unix_id_is_in_range()

If the range allows it, then id==0 should not be forbidden.
This seems to have been taken in from idmap_ldap when the
function was originally created.

See 634cd2e0451d4388c3e3f78239495cf595368b15 .
The other backends don't seem to have had that
extra check for id == 0.

The reasoning for this change is that the range check should
apply to all cases. If the range includes the 0, then it
should be possible to get it as result. In particular,
this way, the function becomes applicable also to the
passdb backend case, e.g. in a samba4-ad-dc setup where
the Admin gets uid == 0.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=12155

Signed-off-by: Michael Adam <obnox at samba.org>
---
 source3/winbindd/idmap_util.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/source3/winbindd/idmap_util.c b/source3/winbindd/idmap_util.c
index 3da39e8..196b4ad 100644
--- a/source3/winbindd/idmap_util.c
+++ b/source3/winbindd/idmap_util.c
@@ -34,11 +34,6 @@
  */
 bool idmap_unix_id_is_in_range(uint32_t id, struct idmap_domain *dom)
 {
-	if (id == 0) {
-		/* 0 is not an allowed unix id for id mapping */
-		return false;
-	}
-
 	if ((dom->low_id && (id < dom->low_id)) ||
 	    (dom->high_id && (id > dom->high_id)))
 	{
-- 
2.5.5


From 0f32b3a7df9488b50bdcef2cec8d3c688bcc64f2 Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox at samba.org>
Date: Tue, 9 Aug 2016 18:25:12 +0200
Subject: [PATCH 2/2] idmap: centrally check that unix IDs returned by the
 idmap backends are in range

Note: in the long run, it might be good to move this kind of
exit check (before handing the result back to the client)
to the parent winbindd code.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=12155

Signed-off-by: Michael Adam <obnox at samba.org>
---
 source3/winbindd/winbindd_dual_srv.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/source3/winbindd/winbindd_dual_srv.c b/source3/winbindd/winbindd_dual_srv.c
index fb65e9d..0484e19 100644
--- a/source3/winbindd/winbindd_dual_srv.c
+++ b/source3/winbindd/winbindd_dual_srv.c
@@ -189,6 +189,10 @@ NTSTATUS _wbint_Sids2UnixIDs(struct pipes_struct *p,
 	for (i=0; i<num_ids; i++) {
 		struct id_map *m = id_map_ptrs[i];
 
+		if (!idmap_unix_id_is_in_range(m->xid.id, dom)) {
+			m->status = ID_UNMAPPED;
+		}
+
 		if (m->status == ID_MAPPED) {
 			ids[i].xid = m->xid;
 		} else {
-- 
2.5.5

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20160816/0d92eb57/signature.sig>