Installing Samba unexpectedly adds many unknown local users to sambashare group

[Richard Earnshaw]

[Impact]

Up until Ubuntu 11.10, administrator access using the sudo tool was granted via the "admin" Unix group. The samba postinst script has some logic that automatically adds users in the "admin" group to the sambashare group.

In Ubuntu >= 12.04, administrator access is granted via the "sudo" group [1], and the "admin" group is not automatically created anymore. However the samba postinst functionality that auto-populates sambashare from "admin" has not been removed. This means that users an "admin" group, which now has no special meaning in Ubuntu, are automatically added to the sambashare group. This is wrong, and can have security implications given that the "admin" group can be a remote group (this is how this bug was first discovered, see the Original Description below).

[1] https://wiki.ubuntu.com/PrecisePangolin/ReleaseNotes/UbuntuDesktop#PrecisePangolin.2FReleaseNotes.2FCommonInfrastructure.Common_Infrastructure

[Test Case]

Reproducer:

1. Start with a clean Ubuntu system
2. Created the "admin" group and add some users to it
3. Install samba
4. Verify that such users are added to sambashare

Fix verification:

4. Verify that such users are NOT added to sambashare.

Test PPA: https://launchpad.net/~paride/+archive/ubuntu/samba-lp1942195

[Where problems could occur]

Problems may occur if new systems are deployed with the expectation that users in the "admin" group get auto-added to sambashare. This can only happen is the admin group is manually created before installing samba.

[Development Fix]

The admin -> sambashare auto-add function has been removed from the postinst script. This change was made in Debian.

[Stable Fix]

Same as the Development Fix.

[Original Description]

I'm running Ubuntu 20.04 in an enterprise environment. I recently installed the samba package on my machine which is configured to get most account details from a central ldap server. I was very surprised, therefore, to see the install script adding a large number of remote users who have no local account to the samabashare group in my local groups file.

It turns out that this is because the postinstall script creates an initial sambashare group and then tries to populate it from the 'admin' group. However, since that is a group that is defined in the ldap database it ends up copying a large number of remote userids into the local group file.

This is a bad idea in a centrally managed environment as the contents of that centrally managed group could change at any time. Surely the script should only try to do this if the admin group is local to the machine? Perhaps at the very least it should seek confirmation before performing such a change.

[Launchpad Janitor (janitor)]

Status changed to 'Confirmed' because the bug affects multiple users.

[Paride Legovini (paride)]

Hello Richard and thanks for this bug report. You are right: the samba.postinst script adds all the users in the 'admin' group to the 'sambashare' group:

for USER in `getent group admin | cut -f4 -d:`; do
    adduser "$USER" sambashare \
    || ! getent passwd "$USER" >/dev/null
done

The interesting thing is that I had a look in some Ubuntu systems I have access to and there's no 'admin' group at all. I think the 'admin' group is a legacy thing: the Ubuntu 12.04 release notes [1] report that:

    Up until Ubuntu 11.10, administrator access using the
    sudo tool was granted via the "admin" Unix group. In
    Ubuntu 12.04, administrator access will be granted
    via the "sudo" group.

So in the older Ubuntu releases the 'admin' did always exist as a local group.

The obvious fix here is to have the postinst script add users from the 'sudo' group instead, however I'm not sure I really like this idea. We'll again hardcode a group name in the postinst, and as apparently nobody noticed that the "auto-add admin/sudo users to sambashare" mechanism was broken in all these years, maybe it's not really worth having it in the first place.

Not having it will also make Ubuntu's samba behave like Debian in this respect. Note however that the postinst snippet is present in the Debian package (not a delta), so we should really try to land the fix in Debian, as it would be awkward to have an Ubuntu delta that fixes Ubuntu specific code.

This is worth some more discussion.

[1] https://wiki.ubuntu.com/PrecisePangolin/ReleaseNotes/UbuntuDesktop#PrecisePangolin.2FReleaseNotes.2FCommonInfrastructure.Common_Infrastructure

[Paride Legovini (paride)]

This is what I propose, see the commit message for the rationale:

https://salsa.debian.org/paride/samba/-/commit/4385c3920afa81e2c30a74d80f2ee0eb04753bb9

I didn't open a MR yet, let's agree on the approach first.

[Sergio Durigan Junior (sergiodj)]

Thanks for the commit, Paride. I left a comment there, but I'm also going to copy it over here:

First of all, thank you for taking the time to investigate and write an insightful commit message explaining the situation.

My first thought when reviewing this was: "where did this excerpt of code came from?" I did some archaeology (`git blame` FTW) and found that this was added by commit fe5cef5014db5b5d6cf55e036583f8f84962e9b2. Unfortunately, there doesn't seem to be much context around the decision to add this code to the postinst script; I couldn't find a related Ubuntu bug, nor a mailing list thread that could provide some explanation here.

Either way, after thinking a little bit, and especially after considering the arguments you raised in the commit message, I find myself aligned with your rationale. While I can understand why one would want to use the members of the `admin` group as the initial members of the `sambashare` group, I also tend to think that this is perhaps "too invasive" without much benefit. The user should be the one deciding who is part of `sambashare` and who isn't, just like it is with the Debian samba package.

Given that this is an Ubuntu-related topic, I will copy this comment to the Launchpad bug and we can continue the discussion there if you want. I'm particularly interested in knowing whether you foresee any problems with this approach (I don't).

Thanks!

[Paride Legovini (paride)]

Thanks for reviewing Sergio. I did the same archeology and indeed the first line of that commit message is "This reverts commit fe5cef50." :-)

I don't foresee any problem with the approach that commit implements: it just feels strange to fix a bug by dropping functionality, even if broken and apparently not missed by anyone. However as we agree on it I'll go ahead and open a MR on salsa.

[Paride Legovini (paride)]

Here it is: https://salsa.debian.org/samba-team/samba/-/merge_requests/54

[Launchpad Janitor (janitor)]

This bug was fixed in the package samba - 2:4.13.5+dfsg-2ubuntu3

---------------
samba (2:4.13.5+dfsg-2ubuntu3) impish; urgency=medium

  * d/samba.postinst: do not populate sambashare from the admin group
    (Debian packaging cherry-pick. LP: #1942195)

 -- Paride Legovini <email address hidden> Wed, 06 Oct 2021 10:31:14 +0200

[Paride Legovini (paride)]

The upload to the devel release targeted impish, but actually landed in jammy. Added an Impish task for the SRU.

[Richard Earnshaw (richard-earnshaw)]

Paride, firstly thanks for picking this up. Looking at another machine I have I notice that the lxd group may have a similar issue. This suggests the problem may affect other packages as well. Is there a way to trigger an audit for other packages that might be affected?

[Paride Legovini (paride)]

Hi Richard, very interesting, you are right. The Bionic lxd.preinst script has:

# Add each admin user to the lxd group - for systems installed
# before precise
for u in $(getent group admin | sed -e "s/^.*://" -e "s/,/ /g"); do
    adduser "$u" lxd >/dev/null || true
done

I tried to install Bionic from the ISO image and I can confirm the admin group doesn't get automatically created at install time. This only affects Bionic as in >= Focal lxd is a snap, and I'm pretty sure the snap doesn't touch the groups.

I filed this bug against the lxd package for the lxd maintainers to have a look:

https://bugs.launchpad.net/ubuntu/+source/lxd/+bug/1950635

Thanks again for raising the issue!

[Paride Legovini (paride)]

SRUs to fix this got uploaded to all the supported releases (thanks Sergio!):

 bionic
 focal
 hirsute
 impish

Now it's up to the SRU team to review the change.

[Chris Halse Rogers (raof)]

Hello Richard, or anyone else affected,

Accepted samba into impish-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/samba/2:4.13.14+dfsg-0ubuntu0.21.10.2 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, what testing has been performed on the package and change the tag from verification-needed-impish to verification-done-impish. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-impish. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

[Chris Halse Rogers (raof)]

Hello Richard, or anyone else affected,

Accepted samba into hirsute-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/samba/2:4.13.14+dfsg-0ubuntu0.21.04.2 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, what testing has been performed on the package and change the tag from verification-needed-hirsute to verification-done-hirsute. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-hirsute. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

[Chris Halse Rogers (raof)]

Hello Richard, or anyone else affected,

Accepted samba into focal-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/samba/2:4.13.14+dfsg-0ubuntu0.20.04.2 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, what testing has been performed on the package and change the tag from verification-needed-focal to verification-done-focal. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-focal. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

[Chris Halse Rogers (raof)]

Hello Richard, or anyone else affected,

Accepted samba into bionic-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/samba/2:4.7.6+dfsg~ubuntu-0ubuntu2.24 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, what testing has been performed on the package and change the tag from verification-needed-bionic to verification-done-bionic. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-bionic. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

[Ubuntu SRU Bot (ubuntu-sru-bot)]

All autopkgtests for the newly accepted samba (2:4.13.14+dfsg-0ubuntu0.21.10.2) for impish have finished running.
The following regressions have been reported in tests triggered by the package:

adsys/0.7.1 (arm64)

Please visit the excuses page listed below and investigate the failures, proceeding afterwards as per the StableReleaseUpdates policy regarding autopkgtest regressions [1].

https://people.canonical.com/~ubuntu-archive/proposed-migration/impish/update_excuses.html#samba

[1] https://wiki.ubuntu.com/StableReleaseUpdates#Autopkgtest_Regressions

Thank you!

[Paride Legovini (paride)]

The failed adsys/0.7.1 autopkgtest is due to a timeout:

*** Test killed with quit: ran too long (11m0s).
FAIL github.com/ubuntu/adsys/internal/policies/ad 660.011s

I'm trying with a simple retrigger for now. I'll do the verification once all the tests pass.

[Paride Legovini (paride)]

It failed again (total 3 times) in the same way:

https://autopkgtest.ubuntu.com/results/autopkgtest-impish/impish/arm64/a/adsys/20211118_112720_82d83@/log.gz

[Paride Legovini (paride)]

Verification done per [Test Case].

Note: the adsys autopkgtest failure on impish/arm64 is unrelated to the samba upload:

1. The part of the test that exercises samba passes. The failure is in an unrelated test.
2. The autopkgtest run fails even with adsys from impish-release; the test failure is not a regression caused by the samba upload.
3. The issue was discussed with didrocks (adsys "upstream"), and he agrees there isn't anything pointing at samba in the test failure. Discussion happened in #ubuntu-devel on 2021-11-19 [1].

[1] https://irclogs.ubuntu.com/2021/11/19/%23ubuntu-devel.html

[Paride Legovini (paride)]

Per team discussion we leave the decision whether adding a hint or triggering a migration-reference/0 autopkgtest run to the SRU team.

[Łukasz Zemczak (sil2100)]

It indeed looks to be a bit unreliable, the failure that is. As it has been discussed with adsys upstream, let me hint it before releasing. Thanks!

[Łukasz Zemczak (sil2100)]

Oh, and for the future: please always state which exact versions of packages for each series have been tested when performing verification, as other wise - in some cases - the verification can be rejected because of that.

[Launchpad Janitor (janitor)]

This bug was fixed in the package samba - 2:4.13.14+dfsg-0ubuntu0.21.10.2

---------------
samba (2:4.13.14+dfsg-0ubuntu0.21.10.2) impish; urgency=medium

  * samba.postinst: do not populate sambashare from the Ubuntu admin group
    (LP: #1942195)

 -- Paride Legovini <email address hidden> Fri, 12 Nov 2021 11:17:14 +0100

[Łukasz Zemczak (sil2100)]

The verification of the Stable Release Update for samba has completed successfully and the package is now being released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.

[Launchpad Janitor (janitor)]

This bug was fixed in the package samba - 2:4.13.14+dfsg-0ubuntu0.21.04.2

---------------
samba (2:4.13.14+dfsg-0ubuntu0.21.04.2) hirsute; urgency=medium

  * samba.postinst: do not populate sambashare from the Ubuntu admin group
    (LP: #1942195)

 -- Paride Legovini <email address hidden> Fri, 12 Nov 2021 14:44:50 +0100

[Launchpad Janitor (janitor)]

This bug was fixed in the package samba - 2:4.13.14+dfsg-0ubuntu0.20.04.2

---------------
samba (2:4.13.14+dfsg-0ubuntu0.20.04.2) focal; urgency=medium

  * samba.postinst: do not populate sambashare from the Ubuntu admin group
    (LP: #1942195)

 -- Paride Legovini <email address hidden> Fri, 12 Nov 2021 14:42:02 +0100

[Launchpad Janitor (janitor)]

This bug was fixed in the package samba - 2:4.7.6+dfsg~ubuntu-0ubuntu2.24

---------------
samba (2:4.7.6+dfsg~ubuntu-0ubuntu2.24) bionic; urgency=medium

  * samba.postinst: do not populate sambashare from the Ubuntu admin group
    (LP: #1942195)

 -- Paride Legovini <email address hidden> Wed, 10 Nov 2021 15:29:48 +0100