Bug 10618 - Samba is ignoring supplementary groups
Samba is ignoring supplementary groups
Status: RESOLVED FIXED
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services
4.1.7
x64 Linux
: P5 critical
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2014-05-19 03:09 UTC by nolon
Modified: 2016-06-16 09:13 UTC (History)
7 users (show)

See Also:


Attachments
log level 10 (24.57 KB, application/x-compressed-tar)
2014-05-19 03:13 UTC, nolon
no flags Details
logs since restart (149.76 KB, application/x-compressed-tar)
2014-05-19 11:03 UTC, nolon
no flags Details
A simple patch to remove the code causing missing supplemental group (484 bytes, patch)
2016-03-15 13:26 UTC, Peter Eriksson
no flags Details
Test patch. (890 bytes, patch)
2016-04-12 21:56 UTC, Jeremy Allison
no flags Details
Patch for root cause of bug ? Applies to 4.4.x, 4.3.x (1.56 KB, patch)
2016-05-26 23:43 UTC, Jeremy Allison
no flags Details
Back-port of root-cause patch for 4.2.x, 4.1.x (1.54 KB, patch)
2016-05-26 23:47 UTC, Jeremy Allison
no flags Details
git-am fix for 4.4.next, 4.3.next. (1.80 KB, patch)
2016-05-27 16:33 UTC, Jeremy Allison
vl: review+
Details
git-am fix for 4.2.next, also applies to 4.1.x (1.65 KB, patch)
2016-05-27 17:10 UTC, Jeremy Allison
vl: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description nolon 2014-05-19 03:09:08 UTC
smb.conf:


[global]
        workgroup = TEST
        netbios name = TESTSERVER
        server string = %h server (Samba %v)
        interfaces = eth0,lo
        obey pam restrictions = Yes
        passdb backend = tdbsam
        passwd program = /usr/bin/passwd %u
        passwd chat = *Enter\snew\sUNIX\spassword:* %n\n *Retype\snew\sUNIX\spassword:* %n\n *password\supdated\ssuccessfully* .
        #username map = /etc/samba/smbusers
        log level = 10
        syslog = 0
        security = user
        profile acls = yes
        unix password sync = no
        log file = /var/log/samba/log.%m
        max log size = 100000
        add machine script = /usr/sbin/useradd -s /bin/false -d /var/lib/nobody '%u'
        veto files = /*:Zone.Identifier:*/
        logon path =
        logon home =
        printing = bsd
        domain logons = No
        os level = 65
        unix extensions = yes
        domain master = No
        dns proxy = No
        time server = yes
        wins support = Yes
        panic action = /usr/share/samba/panic-action %d
        socket options = TCP_NODELAY

#[homes]
#        comment = Home Directories
#        create mask = 0700
#        directory mask = 0700
#        browseable = No

[netlogon]
        comment = Network Logon Service
        path = /home/samba/netlogon
        guest ok = Yes
#        share modes = No

[acct]
        comment = acctdrwxrwx--- 2 user1 acct       6 May 19 04:40 acct

        path = /share/acct
#        create mask = 0660
#        directory mask = 0770
        valid users = +"acct"
        writable = yes
        force group = acct
        browseable = yes

Permissions on /share
drwxrwx---   4 user1 acct   29 May 19 04:40 share

Permissions on /share/acct

drwxrwx--- 2 user1 acct       6 May 19 04:40 acct

/etc/passwd:

user1:x:1005:1008::/home/user1:/bin/sh
user2:x:1006:1009::/home/user2:/bin/sh

/etc/group:

acct:x:1007:user1,user2
user1:x:1008:
user2:x:1009:


Samba 4.1.7 on Debian unstable from May 2014
Filesystem is xfs
mount:
/dev/mapper/vgserver-lvroot on / type xfs (rw,noatime,attr2,inode64,noquota)


smbclient //testserver/acct -Uuser2%test222                                                                                                                                                                                          
Domain=[TEST] OS=[Unix] Server=[Samba 4.1.7-Debian]
smb: \> dir
NT_STATUS_ACCESS_DENIED listing \*
smb: \> q



and log attached as a file
Comment 1 nolon 2014-05-19 03:13:04 UTC
Created attachment 9953 [details]
log level 10
Comment 2 Volker Lendecke 2014-05-19 06:09:15 UTC
I'm sorry but the log file start too late, it starts with the share connection. Please upload a log file that contains the session setup phase.

Thanks,

Volker
Comment 3 nolon 2014-05-19 11:03:08 UTC
Created attachment 9954 [details]
logs since restart

Hi Volker
thank you for looking into this I am not sure if all the information that you need is in the logs, to be sure I restarted samba and attached all the logs since the restart. In case the information you need is not in the log let me know, what the session start looks like

Thanks again
Comment 4 Izan Díez Sánchez 2015-12-02 18:13:44 UTC
Found this in 4.3.1 too. User belongs to a group, but it is ignored when the security context struct is created, thus failing with NT_STATUS_ACCESS_DENIED

I'm running it on RedHat6.5 as a ROLE_DOMAIN_MEMBER of a samba3 NT domain. Same functionality works on samba 3.x

Regards,
\\Izan
Comment 5 Ole Hoppe 2015-12-28 14:05:15 UTC
Hi

Ran into this problem, too, and may have found an at least partial solution.

When turning on log level 6 and logging on from a Windows machine, the following log fragment reveals a mismatch between group SIDs and Unix group IDs:

[2015/12/26 11:07:42.559894,  4] ../source3/smbd/sec_ctx.c:321(set_sec_ctx_internal)
  setting sec ctx (10010, 10001) - sec_ctx_stack_ndx = 0
[2015/12/26 11:07:42.559906,  5] ../libcli/security/security_token.c:63(security_token_debug)
  Security token SIDs (7):
    SID[  0]: S-1-22-2-10000
    SID[  1]: S-1-22-2-10001
    SID[  2]: S-1-22-2-10004
    SID[  3]: S-1-1-0
    SID[  4]: S-1-5-2
    SID[  5]: S-1-5-11
    SID[  6]: S-1-22-1-10010
   Privileges (0x               0):
   Rights (0x               0):
[2015/12/26 11:07:42.559947,  5] ../source3/auth/token_util.c:639(debug_unix_user_token)
  UNIX token of user 10010
  Primary group is 10001 and contains 2 supplementary groups
  Group[  0]: 10001
  Group[  1]: 10004
[2015/12/26 11:07:42.559971,  5] ../source3/smbd/uid.c:364(change_to_user_internal)
  Impersonated user: uid=(10010,10010), gid=(0,10001)

There are 3 groups listed by their SIDs[0..2], but only 2 supplementary Unix groups [0..1], the supplementary group with ID 10000 (the first listed by SID) is missing. This makes the user miss out on the permissions this group assignment would grant them.

After debugging Samba code (using 4.3.3 here, but the problem goes back until at least 4.1.9 which I had tried initially), turns out this first group when listed by SID is not copied to the corresponding Unix groups under certain conditions. In source3/auth/auth_util.c, function create_local_token(), towards the end of it, there's a for loop to copy the groups from the SID structure to the Unix group structure:

for (i=0; i<t->num_sids; i++) {

  if (i == 0 && ids[i].type != ID_TYPE_BOTH) {
    continue;
  }

  if (ids[i].type != ID_TYPE_GID &&
      ids[i].type != ID_TYPE_BOTH) {
    DEBUG(10, ("Could not convert SID %s to gid, "
          "ignoring it\n",
          sid_string_dbg(&t->sids[i])));
    continue;
  }
  if (!add_gid_to_array_unique(session_info, ids[i].id,
              &session_info->unix_token->groups,
              &session_info->unix_token->ngroups)) {
    return NT_STATUS_NO_MEMORY;
  }
}

When commenting out the first if block, the first SID group is also copied to the Unix groups and the permissions thereby fixed:

for (i=0; i<t->num_sids; i++) {
/*
  if (i == 0 && ids[i].type != ID_TYPE_BOTH) {
    continue;
  }
*/
  if (ids[i].type != ID_TYPE_GID &&
      ids[i].type != ID_TYPE_BOTH) {
    DEBUG(10, ("Could not convert SID %s to gid, "
          "ignoring it\n",
          sid_string_dbg(&t->sids[i])));
    continue;
  }
  if (!add_gid_to_array_unique(session_info, ids[i].id,
              &session_info->unix_token->groups,
              &session_info->unix_token->ngroups)) {
    return NT_STATUS_NO_MEMORY;
  }
}

Cannot understand why the first SID group should be excluded if it's not both, a UID and a GID (that's what this ID_TYPE_BOTH constant means, I believe). So,
cannot say what effects this change has under different configuration options than mine (Samba as domain member server authenticating against OpenLDAP server). A Samba expert developer should have a deeper look into this.

Hope this helps :-)
Ole
Comment 6 Paul FM 2016-02-24 16:09:34 UTC
Samba gets all groups from Winbind (AD groups, not unix groups)

To use unix groups use this kludge (wish there was an option specifically for this purpose) in the smb.conf file:

username map script = /bin/echo


NEW BUG - I think as of samba 4.3.x (but definitely in 4.3.5):
Samba seems to miss one supplementary Unix group (usually the last one reported by  "id -G user"  - if "id -G user" is not reporting them in numeric order).  So there may be a structure that is defined one object too short to contain the groups or an expectation that they are in numerical order when populating that structure (when they are not always).

I have been able to verify this new bug with a preexec script that calls id and saves the output to a file.  This bug did NOT seem to be present in samba 4.2.3

Example group list from preexec ("id -G" - 59003 missing):
2962 999 40007 40600 50029 50053 50185 50333 59001 59003 59006 59015 59594 59597 59598 59599

Then logged into same machine as the user ("id -G"):
2962 999 40007 40600 50029 50053 50185 50333 59001 59002 59003 59006 59015 59594 59597 59598 59599

Then - output of "id -G user" (logged in as the user):
2962 999 59599 50029 59001 59003 40007 50053 59006 59598 50333 40600 50185 59015 59597 59594 59002

I have been able to see this problem on Suse Enterprise 11 sp1, 11 sp2 and 12
and on openSUSE 13.1

Note in the cases above, the users and groups are coming to the machine via NIS.
Comment 7 Peter Eriksson 2016-03-15 10:02:23 UTC
I'm seeing the same bug on our systems - Samba 4.3.6 running on Solaris 10/x86 and OmniOS (basically Solaris 11 derivate) authenticating against Microsoft Active Directory with unix group memberships retrieved from NIS. Tried switching to getting the group membership directly from AD, but it would still miss one group.

As other users have commented - the problem is the code in:

   source3/auth/auth_util.c:create_local_token()

that for some strange reason excludes the first SID if it isn't ID_TYPE_BOTH (which in our case is ID_TYPE_GID). I noticed this by adding a lof of extra DEBUG print statements to the code in question:

2016/03/15 10:03:56.830214,  4] ../source3/smbd/sec_ctx.c:449(pop_sec_ctx)
  pop_sec_ctx (0, 0) - sec_ctx_stack_ndx = 0
[2016/03/15 10:03:56.830245,  5] ../source3/passdb/lookup_sid.c:1408(sids_to_unixids)
  sids_to_unixids: Done: #0: type=2, id=1511
[2016/03/15 10:03:56.830275,  5] ../source3/passdb/lookup_sid.c:1408(sids_to_unixids)
  sids_to_unixids: Done: #1: type=2, id=14
[2016/03/15 10:03:56.830313,  5] ../source3/passdb/lookup_sid.c:1408(sids_to_unixids)
  sids_to_unixids: Done: #2: type=2, id=10
[2016/03/15 10:03:56.830343,  5] ../source3/passdb/lookup_sid.c:1408(sids_to_unixids)
  sids_to_unixids: Done: #3: type=2, id=290
[2016/03/15 10:03:56.830381,  5] ../source3/passdb/lookup_sid.c:1408(sids_to_unixids)
  sids_to_unixids: Done: #4: type=2, id=1510
[2016/03/15 10:03:56.830418,  5] ../source3/passdb/lookup_sid.c:1408(sids_to_unixids)
  sids_to_unixids: Done: #5: type=0, id=1230188368
[2016/03/15 10:03:56.830449,  5] ../source3/passdb/lookup_sid.c:1408(sids_to_unixids)
  sids_to_unixids: Done: #6: type=0, id=760426308
[2016/03/15 10:03:56.830490,  5] ../source3/passdb/lookup_sid.c:1408(sids_to_unixids)
  sids_to_unixids: Done: #7: type=0, id=12593
[2016/03/15 10:03:56.830568,  5] ../source3/auth/auth_util.c:597(create_local_token)
  create_local_token: Adding gid 14 (sid S-1-22-2-14) to groups list for uid 103
[2016/03/15 10:03:56.830602,  5] ../source3/auth/auth_util.c:597(create_local_token)
  create_local_token: Adding gid 10 (sid S-1-22-2-10) to groups list for uid 103
[2016/03/15 10:03:56.830640,  5] ../source3/auth/auth_util.c:597(create_local_token)
  create_local_token: Adding gid 290 (sid S-1-22-2-290) to groups list for uid 103
[2016/03/15 10:03:56.830677,  5] ../source3/auth/auth_util.c:597(create_local_token)
  create_local_token: Adding gid 1510 (sid S-1-22-2-1510) to groups list for uid 103
[2016/03/15 10:03:56.830713,  5] ../source3/auth/auth_util.c:592(create_local_token)
  Could not convert SID S-1-1-0 to gid, ignoring it
[2016/03/15 10:03:56.830746,  5] ../source3/auth/auth_util.c:592(create_local_token)
  Could not convert SID S-1-5-2 to gid, ignoring it
[2016/03/15 10:03:56.830792,  5] ../source3/auth/auth_util.c:592(create_local_token)
  Could not convert SID S-1-5-11 to gid, ignoring it

If I simply comment out the code in question Samba correctly handles all my Unix groups.

for (i=0; i<t->num_sids; i++) {
#if 0 /* DISABLED 2016-03-15 Peter Eriksson */
  if (i == 0 && ids[i].type != ID_TYPE_BOTH) {
    continue;
  }
#endif
Comment 8 Peter Eriksson 2016-03-15 13:26:41 UTC
Created attachment 11922 [details]
A simple patch to remove the code causing missing supplemental group

Dead simple patch for the problem...
Comment 9 Jeremy Allison 2016-03-21 20:56:03 UTC
Comment on attachment 11922 [details]
A simple patch to remove the code causing missing supplemental group

Problem is I think this is wrong :-(. Jeremy.
Comment 10 Peter Eriksson 2016-03-21 22:39:08 UTC
(In reply to Jeremy Allison from comment #9)

Wrong? Why? Surely it must be a much more robust code to walk thru _all_ the SIDs and look for _all_ GIDs, rather than having a magic first slot that is _silently_ skipped if it just so happen to contain anything other than ID_TYPE_BOTH (for example ID_TYPE_GID) and causing these strange problems (for atleast the people (like me) reporting it here...)

Had it atleast complained in the log files if it finds something other that ID_TYPE_BOTH in ids[0] this would have been easier to find...

When is it a valid case to silently skip ids[0] if it contains ID_TYPE_GID?

(The if statement right after that silent-skipping-check checks for ID_TYPE_GID & ID_TYPE_BOTH and skips it (with a notice logged) if it is something else so anything else other than ID_TYPE_GID or ID_TYPE_BOTH would have been skipped anyway).

- Peter
Comment 11 Jeremy Allison 2016-03-21 22:48:04 UTC
I agree it would be better to report a debug log message. But the logic is such that slot 0 *is* special. I'll look more into this.
Comment 12 Andy Cobaugh 2016-04-08 18:24:19 UTC
We (Penn State) are also seeing this problem. MIT Kerberos realm, OpenLDAP directory. Samba 4.4.0 seems to be dropping the group with the largest GID. I tried the patch and that fixes the behavior. We're curious what else this might break. We're kind of stuck, having just upgraded from 3.5.10 in preparation for badlock.
Comment 13 Jeremy Allison 2016-04-08 19:53:37 UTC
So for (Penn State) are you using winbindd here ? Where *exactly is your group list coming from ?

Can you instrument create_local_token() with debug messages so I know exactly
what the code flow through here is in your case. I need to understand why your ids[0].type != ID_TYPE_BOTH (as it should be).
Comment 14 Andy Cobaugh 2016-04-08 20:23:29 UTC
(In reply to Jeremy Allison from comment #13)

We are not using winbindd

nss_ldap + nsswitch.conf (passwd: files ldap, group: files ldap). Users and groups are stored in LDAP following rfc2307bis.

I can try adding debug statements, but probably not until Wednesday or so of next week.
Comment 15 Jeremy Allison 2016-04-08 20:26:40 UTC
Ah - that is what I suspected. I'm wondering if the clause that ensures ids[0].type == ID_TYPE_BOTH should only be applied if the list came from winbindd (see the checks above that causes create_local_nt_token_from_info3() in the winbindd case, but create_token_from_username() to be called in the non-winbindd case).
Comment 16 Paul FM 2016-04-09 00:12:35 UTC
My problem (of one group being gone) also is with winbindd bypassed for 
group enumeration with:

	username map script = /bin/echo

However - I am authenticating against an AD domain (using Local + NIS 
for group enumeration).
The Linux server itself is NOT bound to AD for user authentication.



On 4/8/16 3:26 PM, samba-bugs@samba.org wrote:
> https://bugzilla.samba.org/show_bug.cgi?id=10618
>
> --- Comment #15 from Jeremy Allison <jra@samba.org> ---
> Ah - that is what I suspected. I'm wondering if the clause that ensures
> ids[0].type == ID_TYPE_BOTH should only be applied if the list came from
> winbindd (see the checks above that causes create_local_nt_token_from_info3()
> in the winbindd case, but create_token_from_username() to be called in the
> non-winbindd case).
>
Comment 17 Jeremy Allison 2016-04-09 03:36:24 UTC
That's why I want a good set of debug traces added to the code so I can see which path it goes down when it's getting the problem. I suspect it only has the problem when going down the !winbindd path.
Comment 18 Jeremy Allison 2016-04-12 21:56:58 UTC
Created attachment 11988 [details]
Test patch.

Can you try this patch ? I think it might do the trick, and if you confirm it works I can submit to master.
Comment 19 Andy Cobaugh 2016-04-13 18:01:20 UTC
(In reply to Jeremy Allison from comment #18)

I can confirm that this patch corrects the behavior that we were seeing after removing the earlier patch that skipped the entire block of code.
Comment 20 Paul FM 2016-05-04 16:11:59 UTC
I was not able to test your patch (it is probably for a specific version 
of samba - I used samba 4.4.2 ).  The smbd created with the patch would 
not provide shares to windows (the unpatched version works fine - except 
for the groups - still missing ONE, but not the last one numerically - 
but the last one listed by ypcat group).

If you would like me to test it, I need to know which precise version(s) 
of samba this patch is for.

Thanks.

On 4/12/16 4:56 PM, samba-bugs@samba.org wrote:
> https://bugzilla.samba.org/show_bug.cgi?id=10618
>
> Jeremy Allison <jra@samba.org> changed:
>
>             What    |Removed                     |Added
> ----------------------------------------------------------------------------
>             Assignee|samba-qa@samba.org          |jra@samba.org
>
> --- Comment #18 from Jeremy Allison <jra@samba.org> ---
> Created attachment 11988 [details]
>    --> https://bugzilla.samba.org/attachment.cgi?id=11988&action=edit
> Test patch.
>
> Can you try this patch ? I think it might do the trick, and if you confirm it
> works I can submit to master.
>
Comment 21 Jeremy Allison 2016-05-26 23:43:39 UTC
Created attachment 12133 [details]
Patch for root cause of bug ? Applies to 4.4.x, 4.3.x

Can anyone who was experiencing this issue please test the following patch ?

It's certainly a flaw that might cause the underlying issue. Applies to 4.3.x and above. Back port for 4.2.x 

Thanks,

Jeremy.
Comment 22 Jeremy Allison 2016-05-26 23:47:50 UTC
Created attachment 12134 [details]
Back-port of root-cause patch for 4.2.x, 4.1.x
Comment 23 Jeremy Allison 2016-05-27 00:06:22 UTC
(In reply to Jeremy Allison from comment #21)

That should be "Back port for 4.2.x to follow", of course.
Comment 24 Paul FM 2016-05-27 14:41:10 UTC
Yes - preliminary testing suggests it is fixed (I get all my UNIX/yp 
groups - unpatched it is always missing one).

I tested with 4.4.2

You caught me on a day I wasn't too busy.



On 5/26/16 7:06 PM, samba-bugs@samba.org wrote:
> https://bugzilla.samba.org/show_bug.cgi?id=10618
>
> Jeremy Allison <jra@samba.org> changed:
>
>             What    |Removed                     |Added
> ----------------------------------------------------------------------------
>    Attachment #12133 [details]|Patch for root cause of bug |Patch for root cause of bug
>          description|?                           |? Applies to 4.4.x, 4.3.x
>
Comment 25 Jeremy Allison 2016-05-27 16:33:12 UTC
Created attachment 12135 [details]
git-am fix for 4.4.next, 4.3.next.

Cherry-picked from master.
Comment 26 Jeremy Allison 2016-05-27 17:10:31 UTC
Created attachment 12137 [details]
git-am fix for 4.2.next, also applies to 4.1.x

Back-port from master. Added master git refspec to commit message.
Comment 27 Jeremy Allison 2016-05-27 20:17:54 UTC
Reassigning to Karolin for inclusion in 4.4.next, 4.3.next, 4.2.next.
Comment 28 Karolin Seeger 2016-05-30 09:11:39 UTC
(In reply to Jeremy Allison from comment #27)
Pushed to autobuild-v4-[4|3]-test.
Comment 29 Karolin Seeger 2016-05-30 09:14:52 UTC
(In reply to Jeremy Allison from comment #27)
Hi Jeremy,

4.2 is in the security fixes only mode.
Is this one severe enough to ship another 4.2 maintenance release?

Please note that 2 other patchsets have been added to the v4-2-test branch which address regressions introduced by the last security release.

Thanks!

Karo
Comment 30 Karolin Seeger 2016-05-31 11:12:18 UTC
(In reply to Karolin Seeger from comment #28)
Pushed to both branches.
Comment 31 Jeremy Allison 2016-05-31 16:14:16 UTC
(In reply to Karolin Seeger from comment #29)

This isn't in and of itself enough for a maintanence release, but if you're doing another release I'd include it I think. It will fix missing group-id elements in non-standard fileserver setups (like the people complaining here).
Comment 32 Karolin Seeger 2016-06-01 07:53:51 UTC
(In reply to Jeremy Allison from comment #31)
Ok, thanks!
It think it would be a good idea to ship anpther 4.2 maintenance release to separate the bug fixes from the security releases.
Comment 33 Karolin Seeger 2016-06-01 07:54:13 UTC
Pushed to autobuild-v4-2-test.
Comment 34 Karolin Seeger 2016-06-16 09:13:00 UTC
(In reply to Karolin Seeger from comment #33)
Pushed to v4-2-test.
Closing out bug report.

Thanks!