Remove capabilities after setting SmackLabel of executed processes.

[jobol]

Submission type

• Bug report

systemd version the issue has been seen with

systemd 232 (+PAM -AUDIT -SELINUX +IMA -APPARMOR +SMACK +SYSVINIT +UTMP -LIBCRYPTSETUP -GCRYPT -GNUTLS +ACL +XZ -LZ4 -SECCOMP +BLKID -ELFUTILS +KMOD -IDN)

Used distribution

AGL(DD) / yocto(pyro)

In case of bug report: Expected behaviour you didn't see

Setting the smack label and dropping all capabilities of the executed processes should be possible

In case of bug report: Unexpected behaviour you saw

It is not possible to set the smack label because capabilities are dropped first

In case of bug report: Steps to reproduce the problem

Put in the unit file:

SmackProcessLabel=XXX
User=XXX
CapabilityBoundingSet=
AmbientCapabilities=

Then any process to start raise an error.

Appendice

I would really like to get your opinion on the topic because I can not believe that there is a mistake here on your side. So there is perhaps an other way to solve that issue. Some time ago (and some version ago) I had a look at how capabilities were handled when setting security context. IIRC it was well done so I'm surprized to raise that issue.

I use a patch to avoid the issue, it set smack label before dropping capabilities: https://gerrit.automotivelinux.org/gerrit/gitweb?p=AGL/meta-agl.git;a=blob_plain;f=meta-agl/recipes-core/systemd/systemd/0001-Switch-Smack-label-earlier.patch;h=a0e7456f82a5cfa3c391ce4d23260fdca8dec476;hb=e46ae4ae39e8b8924c86f213c14791573a10c9b0

[yuwata]

The version of systemd you use is old. Please read CONTRIBUTING.md

We only track bugs in the two most recently released versions of systemd in the GitHub Issue tracker. If you are using an older version of systemd, please contact your distribution's bug tracker instead.

Anyway, the change is done by 5cd9cd3. And it seems that the logic in the current code is almost same as that time. I am not sure that your approach is right or not. But if the patch works fine for you, I recommend you to create a PR by rebasing it on top of the current master.

[poettering]

@walyong any chance you can comment here? you appears to be the best SMACK guru to comment on this?

@jobol note that the maintainers of systemd do not use or run SMACK, we rely on contributors such as @walyong to submit and review relevant patches.

Anyway, if you have a proposed fix, please file it as PR for us to consider it.

[jobol]

Thank you @yuwata and @poettering for your clear advises. I now understand that you have no specific already forged thoughts about what it has to be for smack related things.

We are agreeing together that the exec must be done with the label set by SmackExecLabel except if prefixed with +. That matters.

I'll wait a little for the advises of @walyong. I guess that the next step will be to go deeper in the understanding of the code (that I only reviewed fast) to propose some really appropriate code.

[walyong]

I'm also using very old version of systemd now. (Because of kernel dependency.)
But I'm also thinking the SmackProcessLabel= has to be performed before CapabilityBoundingSet=.
According to kernel code, all of smack set/write operations need CAP_MAC_ADMIN.

But I'd looked into recent systemd code. And I found below comments:

/* Apply the MAC contexts late, but before seccomp syscall filtering, as those should really be last to
* influence our own codepaths as little as possible. Moreover, applying MAC contexts usually requires
* syscalls that are subject to seccomp filtering, hence should probably be applied before the syscalls
* are restricted. */

@poettering I think there is some of similar issue in exec restriction. #3993.
You are the above comment author. :)
If we have no special reason to perform set MAC label after capability bounding set, we need to move up those.

[poettering]

yeah, i cleaned up the execute.c order a bit, so that all MAC transitions are done at the same location. If SMACK transitions require more privileges than SELinux transitions, then I figure we need to move that back. Sorry for breaking that.

I cannot test that unfortunately. Any chance you can look into this, and provide a patch that makes this work again for SMACK?

[jobol]

Smack needs CAP_MAC_ADMIN. All explained.
I'll emit a PR including a rechecked version of the patch linked in intro in November (on latest master).
@walyong Do you agree?

[walyong]

Yeah. Why not? :) Agree.

[topimiettinen]

I think the way to end this would be to implement a new way in kernel to set SMACK label that is delayed to execve(), like SELinux.

[cschaufler]

With privilege: write the label you want to change to into /sys/fs/smackfs/relabel-self.
Drop privilege and take whatever other actions you like.
Without privilege: write the same label to /proc/self/attr/current.
Your label changes! The first write notifies the system that it's OK for you to change to that label after you've dropped privilege. The second changes the label. You only get to do the change once, and you can't change it back. It's not inherited over exec.

[cschaufler]

Smack issues of all sorts should be sent to smack-discuss@lists.01.org and casey@schaufler-ca.com.

[jobol]

see #7378

[poettering]

Closing, since #7378 has been merged.