Bug 12155 - Some idmap backends don't perform range checks for the result of sids_to_xids
Some idmap backends don't perform range checks for the result of sids_to_xids
Status: REOPENED
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Winbind
unspecified
All All
: P5 normal
: ---
Assigned To: Michael Adam
Samba QA Contact
:
Depends on:
Blocks: 12410
  Show dependency treegraph
 
Reported: 2016-08-16 10:34 UTC by Michael Adam
Modified: 2016-11-16 08:09 UTC (History)
7 users (show)

See Also:


Attachments
patch for 4.5 (2.95 KB, patch)
2016-08-17 05:35 UTC, Andreas Schneider
obnox: review+
abartlet: review-
Details
patch for 4.4 (12.50 KB, patch)
2016-08-17 05:35 UTC, Andreas Schneider
obnox: review-
Details
patch for 4.4, backported from master (3.04 KB, patch)
2016-08-19 07:20 UTC, Michael Adam
obnox: review+
Details
updated patch for 4.4, backported from master (2.82 KB, patch)
2016-08-19 07:23 UTC, Michael Adam
obnox: review+
asn: review+
Details
fixed patch for 4.4, backported from master (2.83 KB, patch)
2016-08-30 16:28 UTC, Michael Adam
obnox: review+
asn: review+
abartlet: review-
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Adam 2016-08-16 10:34:41 UTC
This applies notably to the hash and ad backends.
The autorid backend does not do explicit range checking either, but here, the
implicit calculations seem to be safe, afaict.
Comment 1 Andreas Schneider 2016-08-17 05:35:05 UTC
Created attachment 12373 [details]
patch for 4.5
Comment 2 Andreas Schneider 2016-08-17 05:35:23 UTC
Created attachment 12374 [details]
patch for 4.4
Comment 3 Michael Adam 2016-08-17 07:06:21 UTC
Comment on attachment 12374 [details]
patch for 4.4

I think picking the 2 first patches for context in _wbint_Sids2UnixIDs here is wrong: They are based on the assumption that now the callers of _wbint_Sids2UnixIDs will only ever call it with one domain only. This prerequisite work has not been picked to 4.4 though.

I will propose a more minimal backport patch that will put the range check at the correct, and corresponding position.
Comment 4 Michael Adam 2016-08-19 07:20:52 UTC
Created attachment 12387 [details]
patch for 4.4, backported from master

updated more minimal patch that puts the check into the corresponding place in old code.
Comment 5 Michael Adam 2016-08-19 07:23:21 UTC
Created attachment 12388 [details]
updated patch for 4.4, backported from master

removed the review flags from master, since this is not a cherry-pick any more, really.
Comment 6 Andreas Schneider 2016-08-23 06:22:30 UTC
Comment on attachment 12388 [details]
updated patch for 4.4, backported from master

LGTM
Comment 7 Andreas Schneider 2016-08-23 06:23:00 UTC
Karolin, please add the patches to the relevant branches. Thanks!
Comment 8 Stefan Metzmacher 2016-08-24 09:46:59 UTC
Pushed to autobuild-v4-5-test.
Comment 9 Stefan Metzmacher 2016-08-28 15:54:03 UTC
Pushed to v4-5-test.
Comment 10 Michael Adam 2016-08-30 16:28:03 UTC
Created attachment 12420 [details]
fixed patch for 4.4, backported from master

The previous patch was buggy it used the wrong id variable to check with idmap_unixid_is_in_range()...

This one should be correct. :-)
Comment 11 Andreas Schneider 2016-08-30 16:29:06 UTC
Comment on attachment 12420 [details]
fixed patch for 4.4, backported from master

+			if (!idmap_unix_id_is_in_range(ids[j].xid.id, dom)) {

Looks good now!
Comment 12 Michael Adam 2016-08-30 16:31:59 UTC
Luckily, this had not gone into v4-4-test yet.
Now it can be pushed.
Comment 13 Karolin Seeger 2016-09-13 09:58:02 UTC
Pushed to autobuild-v4-4-test.
Comment 14 Karolin Seeger 2016-09-20 07:06:30 UTC
Pushed to both branches.
Closing out bug report.

Thanks!
Comment 15 Andrew Bartlett 2016-10-29 09:34:15 UTC
We need code in the AD DC case to bail out hard if the range was specified in the smb.conf, as that is maintained only in idmap.ldb and sam.ldb (for rfc2307 entries).
Comment 16 Andrew Bartlett 2016-11-08 17:50:57 UTC
(In reply to Andrew Bartlett from comment #15)
This bug causes a regression, see bug 12410.  Therefore, the patches need to be reverted from Samba 4.4 and 4.5. 

I'll prepare revert patches, on the basis that it breaks upgrades on existing installs, and the issue addressed by this patch isn't serious enough to justify the breakage.

In particular, allowing this patch to remain would ensure that applying a security fix in a rush would break existing sites that mistakenly have idmap configuration elements specified on the AD DC.  See the example on bug 12410.

For Samba 4.6 we can change the code to clearly warn about such invalid configuration, but we can't do it for 4.4 and 4.5 as no clear WHATSNEW was provided.

Sorry,
Comment 17 Michael Adam 2016-11-09 10:14:36 UTC
(In reply to Andrew Bartlett from comment #16)
> (In reply to Andrew Bartlett from comment #15)
> This bug causes a regression, see bug 12410.  Therefore, the patches need to
> be reverted from Samba 4.4 and 4.5. 
> 
> I'll prepare revert patches, on the basis that it breaks upgrades on
> existing installs, and the issue addressed by this patch isn't serious
> enough to justify the breakage.

I violently disagree and will without further discussion NACK revert patches. If I get you right, the workaround for the issue you mention is fixing a broken configuration for one particular use-case? I don't see how this justifies reverting a real fix for many file-serving use cases. Can't this be fixed for the specific needs of the ad-server setup? (It is imho a design mistake anyways if the ad-server id mapping does not honor the idmap config.)

> In particular, allowing this patch to remain would ensure that applying a
> security fix in a rush would break existing sites that mistakenly have idmap
> configuration elements specified on the AD DC.  See the example on bug 12410.

But the workaround is easy: fix the config.

> For Samba 4.6 we can change the code to clearly warn about such invalid
> configuration, but we can't do it for 4.4 and 4.5 as no clear WHATSNEW was
> provided.

Obviously not: It fixed a long-standing bug, and apparently the selftest was not complete enough to catch this regression. So it went unnoticed. No reason to revert it, imho. This needs to be fixed specifically for the ad-server.

Cheers - Michael
Comment 18 Stefan Metzmacher 2016-11-09 10:16:58 UTC
(In reply to Michael Adam from comment #17)

Yes, I guess changing the default range for the ad server case to
0-4294967295, would also restore the old behavior.
Comment 19 Stefan Metzmacher 2016-11-09 10:25:19 UTC
(In reply to Stefan Metzmacher from comment #18)

Maybe something like:

diff --git a/source3/winbindd/idmap_passdb.c b/source3/winbindd/idmap_passdb.c
index cf8ad74..e6f9d25 100644
--- a/source3/winbindd/idmap_passdb.c
+++ b/source3/winbindd/idmap_passdb.c
@@ -31,7 +31,15 @@
 *****************************/
 
 static NTSTATUS idmap_pdb_init(struct idmap_domain *dom)
-{      
+{
+       if (!pdb_is_responsible_for_everything_else()) {
+               return NT_STATUS_OK;
+       }
+
+       if (dom->low_id == 0 && dom->high_id == 0) {
+               dom->high_id = UINT32_MAX;
+       }
+
        return NT_STATUS_OK;
 }
Comment 20 Stefan Metzmacher 2016-11-09 10:37:37 UTC
(In reply to Stefan Metzmacher from comment #19)

Ok, looking at https://bugzilla.samba.org/show_bug.cgi?id=12410#c2
this is just an invalid configuration, just removing (or adjusting)

    idmap config * : range = 100000 - 33554431

should fix it.

We could have the following in order to ignore the
idmap config * : range = 100000 - 33554431
line:

 static NTSTATUS idmap_pdb_init(struct idmap_domain *dom)
-{      
+{
+       if (!pdb_is_responsible_for_everything_else()) {
+               return NT_STATUS_OK;
+       }
+
+       dom->low_id = 0;
+       dom->high_id = 0;
+
        return NT_STATUS_OK;
 }
Comment 21 Kris Karas 2016-11-14 04:29:22 UTC
I agree with Andrew Bartlett in comment #16 that this is a serious bug.
For my wife and baby, this was definitely not fun, as soon as my belt pager went off at 3am and the phone started ringing as nobody could log in.

Why I am writing this here is that this bug is NOT easy to find.  I immediately reverted to 4.4.5, then spent the next several weeks googling for variations on, "samba 4.4.6 breaks domain authentication," and I came up empty time and again.  Today, in utter frustration, I decided to "git bisect" from samba 4.4.5 to 4.4.6 to see what broke, which led me to e0deeddc948cbf2a32ac5ca99962827001102025, and bug 12155.

The takeaway?  As an admin looking for a fix, I never would have thought to google for "centrally check unix IDs".  Poignantly, machine domain trust accounts don't have unixuid/unixgid attributes by design, thus should not suffer any range-check fate.  Furthermore, for those objects that DO have a unixuid/unixgid attribute, they are all properly in-range.  In my smb.conf:
    idmap config DOMA : range = 300 - 99999
And all of my authenticable objects (type user) have unixuid/unixgid values in the 1000-2000 range as specified.

The error message one sees on the client machine is confusing:
client~# net ads testjoin
Join is OK
client~# net rpc testjoin
Join to 'DOMA' is OK
client ~# wbinfo -t
checking the trust secret for domain DOMA via RPC calls failed
wbcCheckTrustCredentials(DOMA): error code was NT_STATUS_DOMAIN_CONTROLLER_NOT_FOUND (0xc0000233)
failed to call wbcCheckTrustCredentials: WBC_ERR_AUTH_ERROR
Could not check secret
client~#

Who would have linked UNIX id ranges to "domain controller not found?"  Very misleading.  I wireshark'ed the transmissions to know that the PDC was indeed talking with the client.  But this error message sure sent me on a wild goose chase at 3am.

In any case, I am idly curious how my PDC should continue to authenticate local clients if I remove the "idmap config ..." lines from its smb.conf.  After all, that's why I deliberately have them in there.