Bug 1196850

This issue is found in openQA "security/apparmor_profile" test case in recent builds such as sle15sp4 build 101.1/88.1.

Run "# systemctl restart smb" with Apparmor profile "usr.sbin.smbd" in enforcing mode reports "DENIED" audit records.

Here is the manual testing steps FYI:

0. apparmor is enabled/active by default

1. run "# aa-enforce usr.sbin.smbd"

2. run "# echo "" > /var/log/audit/audit.log"

3. run "# systemctl restart smb.service"

4. run "# cat /var/log/audit/audit.log" to check the audit record, got

type=AVC msg=audit(1646702374.267:180): apparmor="DENIED" operation="capable" profile="smbd" pid=1929 comm="smbd" capability=12  capname="net_admin"
type=SERVICE_START msg=audit(1646702374.303:181): pid=1 uid=0 auid=4294967295 ses=4294967295 subj==unconfined msg='unit=smb comm="systemd" exe="/usr/lib/systemd/systemd" hostname=? addr=? terminal=? res=success'
type=AVC msg=audit(1646702374.347:182): apparmor="DENIED" operation="open" profile="samba-bgqd" name="/proc/1933/fd/" pid=1933 comm="samba-bgqd" requested_mask="r" denied_mask="r" fsuid=0 ouid=0
type=AVC msg=audit(1646702374.355:183): apparmor="DENIED" operation="capable" profile="samba-bgqd" pid=1933 comm="samba-bgqd" capability=12  capname="net_admin"

reassign to samba team

afaics only one of the DENIES here can be (safely) squashed. that is

"type=AVC msg=audit(1646702374.347:182): apparmor="DENIED" operation="open" profile="samba-bgqd" name="/proc/1933/fd/" pid=1933 comm="samba-bgqd" requested_mask="r" denied_mask="r" fsuid=0 ouid=0"

The net_admin ones are a bit trickier, while we could deny net_admin in the profile(s) which has disadvantages or allow net_admin (which is inherently dangerous as it unnecessarily allows samba access to privileges it doesn't require (see bug #991901, comment #12 for some more detail)

While bug #991901 refers to a similar bug (one that was fixed in the past in systemd) it appears systemd has reintroduced this issue as part of some code changes or rewrite. 


Below is some analysis on what happens now


Details
=======

audit.log snippet

type=AVC msg=audit(1646832789.603:481): apparmor="DENIED" operation="capable" profile="smbd" pid=4119 comm="smbd" capability=12  capname="net_admin"
type=AVC msg=audit(1646832789.663:482): apparmor="DENIED" operation="capable" profile="samba-bgqd" pid=4123 comm="samba-bgqd" capability=12  capname="net_admin"


strace snippet


pid 4118 (smbd)
pid 4123 (samba-bgqd)

4119  getsockopt(0, SOL_SOCKET, SO_TYPE, 0x7ffd89d820b0, [4]) = -1 ENOTSOCK (Socket operation on non-socket)
4119  socket(AF_UNIX, SOCK_DGRAM|SOCK_CLOEXEC, 0) = 11
4119  getsockopt(11, SOL_SOCKET, SO_SNDBUF, [212992], [4]) = 0
4119  setsockopt(11, SOL_SOCKET, SO_SNDBUF, [8388608], 4) = 0
4119  getsockopt(11, SOL_SOCKET, SO_SNDBUF, [425984], [4]) = 0
4119  setsockopt(11, SOL_SOCKET, SO_SNDBUFFORCE, [8388608], 4) = -1 EPERM (Operation not permitted)
4119  getuid()                          = 0
4119  geteuid()                         = 0
4119  getgid()                          = 0
4119  getegid()                         = 0



4123  close(4)                          = 0
4123  socket(AF_UNIX, SOCK_DGRAM|SOCK_CLOEXEC, 0) = 4
4123  getsockopt(4, SOL_SOCKET, SO_SNDBUF, [212992], [4]) = 0
4123  setsockopt(4, SOL_SOCKET, SO_SNDBUF, [8388608], 4) = 0
4123  getsockopt(4, SOL_SOCKET, SO_SNDBUF, [425984], [4]) = 0
4123  setsockopt(4, SOL_SOCKET, SO_SNDBUFFORCE, [8388608], 4) = -1 EPERM (Operation not permitted)
4123  getuid()                          = 0fd_inc_sndbuf


smbd and samba-bgqd processes call samba function become_daemon

     become_daemon()  (samba/lib/util/become_daemon.c)
        --> sd_notify(0, "STATUS=Starting process..."); (systemd/src/libsystemd/sd-daemon/sd-daemon.c)
            --> sd_pid_notify_with_fds(...)
                --> fd_inc_sndbuf(fd, SNDBUF_SIZE); (systemd/src/basic/socket-util.h)
                     --> fd_set_sndbuf(fd, n, true)


                    
at systemd/src/basic/socket-util.c:680

fd is (whatever fd is passed)
n is SNDBUF_SIZE (8*1024*1024)
bool increase is true

sizeof value below is 4

int fd_set_sndbuf(int fd, size_t n, bool increase)
{


	""		""
	""		""
	""		""

        /* SO_SNDBUF above may set to the kernel limit, instead of the requested size.
         * So, we need to check the actual buffer size here. */
        l = sizeof(value);
        r = getsockopt(fd, SOL_SOCKET, SO_SNDBUF, &value, &l);

*** Note: from strace above seems value is 425984 (which fails the test below)

        if (r >= 0 && l == sizeof(value) && increase ? (size_t) value >= n*2 : (size_t) value == n*2)
                return 1;

        /* If we have the privileges we will ignore the kernel limit. */
        r = setsockopt_int(fd, SOL_SOCKET, SO_SNDBUFFORCE, n);
        if (r < 0)
                return r;

	""		""
	""		""
	""		""
}

so basically systemd tries to use setsockopt to set the buffer size to SO_SNDBUF, in the case where this size exceeds the kernel limit (which is what is happening here) systemd just tries to force the buffer size with a call that needs net_admin capabilities


I will submit a patch for the /proc/blah/fd DENY. I don't think we should fix the other one

(In reply to Noel Power from comment #3)
> 
> 
> I will submit a patch for the /proc/blah/fd DENY. I don't think we should
> fix the other one


what I mean by not fixing the other one I really mean that I don't think we should adjust the samba profile, it would be better if systemd didn't do what it is doing

https://gitlab.com/apparmor/apparmor/-/merge_requests/860

for the access denied on proc/*/fd dir

(In reply to Noel Power from comment #3)
> afaics only one of the DENIES here can be (safely) squashed. that is
> 
> "type=AVC msg=audit(1646702374.347:182): apparmor="DENIED" operation="open"
> profile="samba-bgqd" name="/proc/1933/fd/" pid=1933 comm="samba-bgqd"
> requested_mask="r" denied_mask="r" fsuid=0 ouid=0"
> 
> The net_admin ones are a bit trickier, while we could deny net_admin in the
> profile(s) which has disadvantages or allow net_admin (which is inherently
> dangerous as it unnecessarily allows samba access to privileges it doesn't
> require (see bug #991901, comment #12 for some more detail)
> 
> While bug #991901 refers to a similar bug (one that was fixed in the past in
> systemd) it appears systemd has reintroduced this issue as part of some code
> changes or rewrite. 
> 
> 
> Below is some analysis on what happens now
> 
> 
> Details
> =======
> 
> audit.log snippet
> 
> type=AVC msg=audit(1646832789.603:481): apparmor="DENIED"
> operation="capable" profile="smbd" pid=4119 comm="smbd" capability=12 
> capname="net_admin"
> type=AVC msg=audit(1646832789.663:482): apparmor="DENIED"
> operation="capable" profile="samba-bgqd" pid=4123 comm="samba-bgqd"
> capability=12  capname="net_admin"
> 
> 
> strace snippet
> 
> 
> pid 4118 (smbd)
> pid 4123 (samba-bgqd)
> 
> 4119  getsockopt(0, SOL_SOCKET, SO_TYPE, 0x7ffd89d820b0, [4]) = -1 ENOTSOCK
> (Socket operation on non-socket)
> 4119  socket(AF_UNIX, SOCK_DGRAM|SOCK_CLOEXEC, 0) = 11
> 4119  getsockopt(11, SOL_SOCKET, SO_SNDBUF, [212992], [4]) = 0
> 4119  setsockopt(11, SOL_SOCKET, SO_SNDBUF, [8388608], 4) = 0
> 4119  getsockopt(11, SOL_SOCKET, SO_SNDBUF, [425984], [4]) = 0
> 4119  setsockopt(11, SOL_SOCKET, SO_SNDBUFFORCE, [8388608], 4) = -1 EPERM
> (Operation not permitted)
> 4119  getuid()                          = 0
> 4119  geteuid()                         = 0
> 4119  getgid()                          = 0
> 4119  getegid()                         = 0
> 
> 
> 
> 4123  close(4)                          = 0
> 4123  socket(AF_UNIX, SOCK_DGRAM|SOCK_CLOEXEC, 0) = 4
> 4123  getsockopt(4, SOL_SOCKET, SO_SNDBUF, [212992], [4]) = 0
> 4123  setsockopt(4, SOL_SOCKET, SO_SNDBUF, [8388608], 4) = 0
> 4123  getsockopt(4, SOL_SOCKET, SO_SNDBUF, [425984], [4]) = 0
> 4123  setsockopt(4, SOL_SOCKET, SO_SNDBUFFORCE, [8388608], 4) = -1 EPERM
> (Operation not permitted)
> 4123  getuid()                          = 0fd_inc_sndbuf
> 
> 
> smbd and samba-bgqd processes call samba function become_daemon
> 
>      become_daemon()  (samba/lib/util/become_daemon.c)
>         --> sd_notify(0, "STATUS=Starting process...");
> (systemd/src/libsystemd/sd-daemon/sd-daemon.c)
>             --> sd_pid_notify_with_fds(...)
>                 --> fd_inc_sndbuf(fd, SNDBUF_SIZE);
> (systemd/src/basic/socket-util.h)
>                      --> fd_set_sndbuf(fd, n, true)
> 
> 
>                     
> at systemd/src/basic/socket-util.c:680
> 
> fd is (whatever fd is passed)
> n is SNDBUF_SIZE (8*1024*1024)
> bool increase is true
> 
> sizeof value below is 4
> 
> int fd_set_sndbuf(int fd, size_t n, bool increase)
> {
> 
> 
> 	""		""
> 	""		""
> 	""		""
> 
>         /* SO_SNDBUF above may set to the kernel limit, instead of the
> requested size.
>          * So, we need to check the actual buffer size here. */
>         l = sizeof(value);
>         r = getsockopt(fd, SOL_SOCKET, SO_SNDBUF, &value, &l);
> 
> *** Note: from strace above seems value is 425984 (which fails the test
> below)
> 
>         if (r >= 0 && l == sizeof(value) && increase ? (size_t) value >= n*2
> : (size_t) value == n*2)
>                 return 1;
> 
>         /* If we have the privileges we will ignore the kernel limit. */
>         r = setsockopt_int(fd, SOL_SOCKET, SO_SNDBUFFORCE, n);
>         if (r < 0)
>                 return r;
> 
> 	""		""
> 	""		""
> 	""		""
> }
> 
> so basically systemd tries to use setsockopt to set the buffer size to
> SO_SNDBUF, in the case where this size exceeds the kernel limit (which is
> what is happening here) systemd just tries to force the buffer size with a
> call that needs net_admin capabilities
> 

@systemd-maintainers would be great to get some systemd perspective about this issue. In a nutshell the issue is a process that calls systemd function 'sd_notify' can (too) easily find itself getting into a situation where it is calling a function requiring net_admin capabilities. At the moment we could

a) increase kernel limit (unlikely to happen I'd say)
b) add a deny rule for samba the noisy setsockopt (but this can hide problems (and associated log messages) in the future)
c) add the net_admin capability to samba (but it isn't desirable to allow samba access to privileges it doesn't really require)

On the systemd side the code that calls the noisy setsockopt doesn't even care if it succeeds or not

_public_ int sd_pid_notify_with_fds(
                pid_t pid,
                int unset_environment,
                const char *state,
                const int *fds,
                unsigned n_fds) {


    ""   ""    ""    ""

        fd = socket(AF_UNIX, SOCK_DGRAM|SOCK_CLOEXEC, 0);
        if (fd < 0) {
                r = -errno;
                goto finish;
        }

==>     (void) fd_inc_sndbuf(fd, SNDBUF_SIZE);

        iovec = IOVEC_MAKE_STRING(state);

        send_ucred =
                (pid != 0 && pid != getpid_cached()) ||
                getuid() != geteuid() ||
                getgid() != getegid();


I suppose I wonder why 
  * SNDBUF_SIZE needs to be as large as it is?
  * would systemd consider allowing the value be controlled by the process calling it (either by env variable or modified c-api)
  * or alternatively making the behaviour of trying to override that value (the noisy call) somehow configurable

This is an autogenerated message for OBS integration:
This bug (1196850) was mentioned in
https://build.opensuse.org/request/show/964948 Factory / apparmor

Test case still failed on this on latest build 117.1: https://openqa.suse.de/tests/8418526#step/usr_sbin_smbd/88
I will check on next build.

A new attempt to fix this issue again: https://github.com/systemd/systemd/pull/22895

(In reply to Franck Bui from comment #9)
> A new attempt to fix this issue again:
> https://github.com/systemd/systemd/pull/22895

So this was rejected (as expected) as filtering manually functions based on the effective caps  is not viable, obviously.

Noel, upstream suggested to fix apparmor policy/profile for smbd instead.

Would that work for you ?

(In reply to Franck Bui from comment #10)
> (In reply to Franck Bui from comment #9)
> > A new attempt to fix this issue again:
> > https://github.com/systemd/systemd/pull/22895
> 
> So this was rejected (as expected) as filtering manually functions based on
> the effective caps  is not viable, obviously.
> 
> Noel, upstream suggested to fix apparmor policy/profile for smbd instead.
> 
> Would that work for you ?

well the choices are
 1) putting a deny in the smbd (and other) profiles of any systemd process that wants to be a a daemon from systemd pov
 2) allow the netadmin privilege
 3) ignore the noise in the audit.log
 4) fix systemd :-)


option 1 can make life difficult down the line if functionality really depends on netadmin privilege because any helpful audit log messages will be squashed
option 2 is probably a security hole we don't want to open
option 3 ... this bug says otherwise :-) (personally it doesn't bother me to have some noise in the log (it's only on service start))
open 4 appears not to be a runner :/ however have to say, this smells like a regression to me since this was fixed previously (for the purpose of squashing his error)

@christian, what is your opinion here, it looks like adding the deny might be the only choice. will that fly upstream ?

(In reply to Noel Power from comment #11)
> open 4 appears not to be a runner :/ however have to say, this smells like a
> regression to me since this was fixed previously (for the purpose of
> squashing his error)

I'm trying to discuss this point with Lennart, feel free to join.

(In reply to Noel Power from comment #11)
> well the choices are
>  1) putting a deny in the smbd (and other) profiles of any systemd process
> that wants to be a a daemon from systemd pov
>  2) allow the netadmin privilege
>  3) ignore the noise in the audit.log
>  4) fix systemd :-)

> @christian, what is your opinion here, it looks like adding the deny might
> be the only choice. will that fly upstream ?

Good question ;-)  It probably has better chances than allowing it, but as you already described, none of options 1, 2 or 3 are what I'd call a good idea.
Additionally, this is not only about smbd - it also affects other daemons (for example, I've seen it with apache).

The sane solution would be if systemd would not do something that needs an otherwise superfluous capability (which means not increasing the buffer size beyond the kernel limit). But then, and I have to apologize in advance for this rant, this would be the first time that systemd does something sane ;-)

Just wondering - is increasing the buffer size beyond the kernel limit really needed in practise? Will something break if it stays within the limit, or could systemd just drop the "beyond the limit" call, and nobody will notice?

(In reply to Franck Bui from comment #12)
> I'm trying to discuss this point with Lennart, feel free to join.

TL;DR: Thanks, but no thanks.

Long version:

I already burnt my fingers with proposing ExecRestart= some years ago - unsuccessfully, despite explaining why it's needed, and despite someone else joined the discussion with another usecase for ExecRestart=. But no, according to Lennart, if you need ExecRestart=, then your program and/or usecase is broken. Case closed.

Without ExecRestart=, we have to carry an insane workaround in apparmor.service.

Trust me, the only things I'd write in an upstream discussion wouldn't be helpful. It's probably better if I just stay away.

Therefore the only thing I'll do is to wish you good luck with discussing these superfluous capability requirements upstream ;-)

(In reply to Christian Boltz from comment #13)
> (In reply to Noel Power from comment #11)
> > well the choices are
> >  1) putting a deny in the smbd (and other) profiles of any systemd process
> > that wants to be a a daemon from systemd pov
> >  2) allow the netadmin privilege
> >  3) ignore the noise in the audit.log
> >  4) fix systemd :-)
> 
> > @christian, what is your opinion here, it looks like adding the deny might
> > be the only choice. will that fly upstream ?
> 
> Good question ;-)  It probably has better chances than allowing it, but as
> you already described, none of options 1, 2 or 3 are what I'd call a good
> idea.
> Additionally, this is not only about smbd - it also affects other daemons
> (for example, I've seen it with apache).
good to know there is another who suffers :-)
> 
> The sane solution would be if systemd would not do something that needs an
> otherwise superfluous capability (which means not increasing the buffer size
> beyond the kernel limit). But then, and I have to apologize in advance for
> this rant, this would be the first time that systemd does something sane ;-)
> 
> Just wondering - is increasing the buffer size beyond the kernel limit
> really needed in practise? Will something break if it stays within the
> limit, or could systemd just drop the "beyond the limit" call, and nobody
> will notice?

These are questions I also had also expressed earlier in comment #6

> 
> (In reply to Franck Bui from comment #12)
> > I'm trying to discuss this point with Lennart, feel free to join.
> 
> TL;DR: Thanks, but no thanks.
> 
[...]
> Therefore the only thing I'll do is to wish you good luck with discussing
> these superfluous capability requirements upstream ;-)

I guess I will try at least and point out the details mentioned above in the issue, I don't hold out much hope given what you have said :-(

(In reply to Noel Power from comment #14)
> > (In reply to Franck Bui from comment #12)
> > > I'm trying to discuss this point with Lennart, feel free to join.
> > 
> > TL;DR: Thanks, but no thanks.
> > 
> [...]
> > Therefore the only thing I'll do is to wish you good luck with discussing
> > these superfluous capability requirements upstream ;-)
> 
> I guess I will try at least and point out the details mentioned above in the
> issue, I don't hold out much hope given what you have said :-(

well I tried :/ looks like no consideration will be made to offer a way to avoid this situation. I don't really have either the selinux or apparmor knowledge to argue further about this. So I will, when I get a chance create a pull request to add the deny (and see where that goes) The alternatives if that won't fly are to carry our own change (urgh) or just ignore the noise (it isn't *that* noisy I guess)

(In reply to Noel Power from comment #17)
> well I tried :/ looks like no consideration will be made to offer a way to
> avoid this situation. I don't really have either the selinux or apparmor
> knowledge to argue further about this.

Unfortunately I'm in the same situation.

Maybe the security team could share some insight ?

(In reply to Franck Bui from comment #18)
> (In reply to Noel Power from comment #17)
> > well I tried :/ looks like no consideration will be made to offer a way to
> > avoid this situation. I don't really have either the selinux or apparmor
> > knowledge to argue further about this.
> 
> Unfortunately I'm in the same situation.
> 
> Maybe the security team could share some insight ?

in the mean time lets see if the deny squashing rule will fly

  https://gitlab.com/apparmor/apparmor/-/merge_requests/867

however, it seems there is a precedent, sshd and traceroute also do this so it seems we wouldn't be alone

I'm not surprised you didn't get that into systemd. I think adding the DENY is the least bad option. It's not great to hide this, but at least with SELinux this is a common way to approach this and (advanced) users are aware on how to work around this for trouble shooting. So let's go ahead with this

This is an autogenerated message for OBS integration:
This bug (1196850) was mentioned in
https://build.opensuse.org/request/show/970238 Factory / apparmor

Still failed on latest build 131.2: https://openqa.suse.de/tests/8571023#step/usr_sbin_smbd/88

The openQA test case passed: http://openqa.suse.de/tests/8636969#
Please help to change the status to "Resolved".

comment #27 reports the qa test now passes so marking this as resolved

Thanks for the fix.

This is an autogenerated message for openQA integration by the openqa_review script:

This bug is still referenced in a failing openQA test: mau-apparmor_profile
https://openqa.suse.de/tests/8758668

To prevent further reminder comments one of the following options should be followed:
1. The test scenario is fixed by applying the bug fix to the tested product or the test is adjusted
2. The openQA job group is moved to "Released" or "EOL" (End-of-Life)
3. The bugref in the openQA scenario is removed or replaced, e.g. `label:wontfix:boo1234`

Expect the next reminder at the earliest in 28 days if nothing changes in this ticket.

SUSE-RU-2022:2628-1: An update that has two recommended fixes can now be installed.

Category: recommended (important)
Bug References: 1195463,1196850
CVE References: 
JIRA References: 
Sources used:
openSUSE Leap 15.3 (src):    apparmor-2.13.6-150300.3.15.1, libapparmor-2.13.6-150300.3.15.1
SUSE Linux Enterprise Module for Server Applications 15-SP3 (src):    apparmor-2.13.6-150300.3.15.1
SUSE Linux Enterprise Module for Basesystem 15-SP3 (src):    apparmor-2.13.6-150300.3.15.1, libapparmor-2.13.6-150300.3.15.1
SUSE Linux Enterprise Micro 5.2 (src):    apparmor-2.13.6-150300.3.15.1, libapparmor-2.13.6-150300.3.15.1
SUSE Linux Enterprise Micro 5.1 (src):    apparmor-2.13.6-150300.3.15.1, libapparmor-2.13.6-150300.3.15.1

NOTE: This line indicates an update has been released for the listed product(s). At times this might be only a partial fix. If you have questions please reach out to maintenance coordination.

SUSE-RU-2022:2627-1: An update that has two recommended fixes can now be installed.

Category: recommended (important)
Bug References: 1195463,1196850
CVE References: 
JIRA References: 
Sources used:
SUSE Linux Enterprise Software Development Kit 12-SP5 (src):    apparmor-2.8.2-56.9.1
SUSE Linux Enterprise Server 12-SP5 (src):    apparmor-2.8.2-56.9.1

NOTE: This line indicates an update has been released for the listed product(s). At times this might be only a partial fix. If you have questions please reach out to maintenance coordination.

SUSE-RU-2022:2628-2: An update that has two recommended fixes can now be installed.

Category: recommended (important)
Bug References: 1195463,1196850
CVE References: 
JIRA References: 
Sources used:
openSUSE Leap Micro 5.2 (src):    apparmor-2.13.6-150300.3.15.1, libapparmor-2.13.6-150300.3.15.1

NOTE: This line indicates an update has been released for the listed product(s). At times this might be only a partial fix. If you have questions please reach out to maintenance coordination.