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.
Created attachment 12373 [details] patch for 4.5
Created attachment 12374 [details] patch for 4.4
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.
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.
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 on attachment 12388 [details] updated patch for 4.4, backported from master LGTM
Karolin, please add the patches to the relevant branches. Thanks!
Pushed to autobuild-v4-5-test.
Pushed to v4-5-test.
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 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!
Luckily, this had not gone into v4-4-test yet. Now it can be pushed.
Pushed to autobuild-v4-4-test.
Pushed to both branches. Closing out bug report. Thanks!
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).
(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,
(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
(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.
(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; }
(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; }
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.
It seems that on 4.6.x you have to set 'log level = 0' in a DCs smb.conf. If you don't, various tools are logging 'idmap range not specified for domain '*''. This can be multiple lines and when I say multiple, perhaps I should say possibly hundreds, depending on what command is run.