apparmor: allow receiving of signals from 'docker kill' #37831

cyphar · 2018-09-12T12:49:27Z

In newer kernels, AppArmor will reject attempts to send signals to a
container because the signal originated from outside of that AppArmor
profile. Correct this by allowing all unconfined signals to be received.

Carry #36822
Fixes #36809
Signed-off-by: Goldwyn Rodrigues rgoldwyn@suse.com
Signed-off-by: Aleksa Sarai asarai@suse.de

vdemeester · 2018-09-12T12:53:41Z

cc @AkihiroSuda @jessfraz

ericchiang · 2018-09-12T14:27:25Z

I tested this on my affected machine by replacing the docker-default profile with the one suggested here: #36809 (comment)

It appears to have worked, and the docker daemon can now kill containers.

ericchiang · 2018-09-12T14:37:00Z

profiles/apparmor/template.go

Just to note, this is the first time that the daemon assumes it runs as "unconfined". I don't run it as that profile, so I had to tweak it.

Since this is dynamically generated, maybe we could replace this with:

signal (receive) peer={{.DaemonProfile}}

Then fill that value in with whatever's in /proc/self/attr/current

There is a self keyword or something I think

I will take a look into whether there's a self keyword or if we'd need to actually read attr/current. I completely forgot that we have a daemon profile.

EDIT: I don't think we could use a self keyword actually because we need to reference whatever profile Docker is under. I'll write a patch with .DaemonProfile.

ah yeah omg that daemon profile is bad but regardless... lgtm :)

jessfraz · 2018-09-12T14:44:15Z

We need to be careful here because the "peer" keyword is only supported on certain versions iirc
Updated: just saw you did that... Should be fine then

codecov · 2018-09-12T15:54:46Z

Codecov Report

Merging #37831 into master will decrease coverage by 0.05%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #37831      +/-   ##
==========================================
- Coverage   36.13%   36.08%   -0.06%     
==========================================
  Files         610      610              
  Lines       45054    48007    +2953     
==========================================
+ Hits        16282    17324    +1042     
- Misses      26532    28324    +1792     
- Partials     2240     2359     +119

In newer kernels, AppArmor will reject attempts to send signals to a container because the signal originated from outside of that AppArmor profile. Correct this by allowing all unconfined signals to be received. Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com> Signed-off-by: Aleksa Sarai <asarai@suse.de>

ericchiang · 2018-09-12T20:03:58Z

I've built and tested this. It works when the docker deamon runs as a profile other than unconstrained.

Before:

$ sudo docker run -d alpine:latest sleep 600
58a1e1b23e17a740c1a7a0f6d7b8e0fa8337fcab59ae7c949f69ba134eda320e
$ sudo docker kill 58a1e1b23e17a740c1a7a0f6d7b8e0fa8337fcab59ae7c949f69ba134eda320e
Error response from daemon: Cannot kill container: 58a1e1b23e17a740c1a7a0f6d7b8e0fa8337fcab59ae7c949f69ba134eda320e: Cannot kill container 58a1e1b23e17a740c1a7a0f6d7b8e0fa8337fcab59ae7c949f69ba134eda320e: unknown error after kill: docker-runc did not terminate sucessfully: container_linux.go:393: signaling init process caused "permission denied"
: unknown

After:

$ sudo docker run -d alpine:latest sleep 600                                                                                                                                                                       
798a35422d6d92855e4d561e7ac9498a26858f2664160f671161ad4794b6bbc8                                                                                                                                                                            
$ sudo docker kill 798a35422d6d92855e4d561e7ac9498a26858f2664160f671161ad4794b6bbc8                                                                                                                                    
798a35422d6d92855e4d561e7ac9498a26858f2664160f671161ad4794b6bbc8

mrueg

Has fixed the in-container signaling issues I saw.

thaJeztah · 2018-10-01T16:42:38Z

@cyphar sorry for the delay; in the original PR you mentioned that the kernel patch that cause this issue was reverted;

I have since discovered that the corresponding kernel patch that required this change was actually reverted in the upstream kernel. We still have it in the SUSE kernels, so this is still a problem for us, but we can drop this from being merged in upstream Docker (unless the kernel change gets re-merged upstream). However the reason it was reverted is because it broke userspace -- so in theory if it ever gets merged we still won't need to modify the default profile.

Did that change?

thaJeztah · 2018-10-01T16:42:50Z

Also ping @justincormack PTAL

cyphar · 2018-10-03T09:15:56Z

So I just checked the commit history and it looks like the code was never reverted torvalds/linux@cd1dbf7 has been in all kernel versions since 4.14 and there is no revert commit (and git blame shows that commit as the genesis of the current aa_may_signal code). I don't remember who told me it was reverted, but that's clearly not true.

However the kernel code currently has a check that should avoid userspace breakage (which is what this patch is attempting to fix):

static int profile_signal_perm(struct aa_profile *profile,
			       struct aa_label *peer, u32 request,
			       struct common_audit_data *sa)
{
	/* ... */
	if (profile_unconfined(profile) ||
	    !PROFILE_MEDIATES(profile, AA_CLASS_SIGNAL))
		return 0;
	/* ... */
}

Which effectively means that if a profile contains no signal entries then there are no restrictions on signals. But if a profile contains any signal entries then we have to apply this patch so that signals within the same profile and signals from an unconfined profile are allowed.

So why are people seeing this? I think it's because of abstractions/base. Some distributions have entries like the following:

  # Allow unconfined processes to send us signals by default
  signal (receive) peer=unconfined,

  # Allow us to signal ourselves
  signal peer=@{profile_name},

  # Checking for PID existence is quite common so add it by default for now
  signal (receive, send) set=("exists"),

Which will be applied by default in Docker -- so I think it depends strongly on how your distribution is configured whether or not you will hit an issue. If you have signal entries but not these ones, then you'll be in trouble. But we still should include an entry for DaemonProfile because the above doesn't handle that properly...

cyphar · 2018-11-14T04:32:33Z

/ping

thaJeztah · 2018-11-14T12:54:27Z

ping @kolyshkin @justincormack PTAL

thaJeztah

LGTM

sorry for the delay, wanted to have a look from @justincormack before going ahead 🤗

In newer kernels, AppArmor will reject attempts to send signals to a container because the signal originated from outside of that AppArmor profile. For much info, please see moby/moby#37831. Fixes:kata-containers#1568 Signed-off-by: lifupan <lifupan@gmail.com>

On newer kernels and systems, AppArmor will block sending signals in many scenarios by default resulting in strange behaviours (container programs cannot signal each other, or host processes like containerd cannot signal containers). The reason this happens only on some distributions (and is not a kernel regression) is that the kernel doesn't enforce signal mediation unless the profile contains signal rules. However because our profies #include the distribution-managed <abstractions/base>, some distributions added signal rules -- which results in AppArmor enforcing signal mediation and thus a regression. On these systems, containers cannot send and receive signals at all -- meaning they cannot signal each other and the container runtime cannot kill them either. This issue was fixed in Docker in 2018[1] but this code was copied before then and thus the patches weren't carried. It also contains a new fix for a more esoteric case[2]. Ideally this code should live in a project like "containerd/apparmor" so that Docker, libpod, and containerd can share it, but that's probably something to do separately. In addition, the copyright header is updated to reference that the code is copied from Docker (and thus was not written entirely by the containerd authors). [1]: moby/moby#37831 [2]: moby/moby#41337 Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>

On newer kernels and systems, AppArmor will block sending signals in many scenarios by default resulting in strange behaviours (container programs cannot signal each other, or host processes like containerd cannot signal containers). The reason this happens only on some distributions (and is not a kernel regression) is that the kernel doesn't enforce signal mediation unless the profile contains signal rules. However because our profies #include the distribution-managed <abstractions/base>, some distributions added signal rules -- which results in AppArmor enforcing signal mediation and thus a regression. On these systems, containers cannot send and receive signals at all -- meaning they cannot signal each other and the container runtime cannot kill them either. This issue was fixed in Docker in 2018[1] but this code was copied before then and thus the patches weren't carried. It also contains a new fix for a more esoteric case[2]. Ideally this code should live in a project like "containerd/apparmor" so that Docker, libpod, and containerd can share it, but that's probably something to do separately. In addition, the copyright header is updated to reference that the code is copied from Docker (and thus was not written entirely by the containerd authors). [1]: moby/moby#37831 [2]: moby/moby#41337 Signed-off-by: Aleksa Sarai <cyphar@cyphar.com> (cherry picked from commit d8572b6) Signed-off-by: Brad Davidson <brad.davidson@rancher.com>

GordonTheTurtle added the status/0-triage label Sep 12, 2018

vdemeester requested a review from justincormack September 12, 2018 12:53

vdemeester added status/2-code-review and removed status/0-triage labels Sep 12, 2018

ericchiang reviewed Sep 12, 2018

View reviewed changes

cyphar force-pushed the apparmor-external-templates branch from 76cce67 to 2d4461b Compare September 12, 2018 15:54

cyphar force-pushed the apparmor-external-templates branch from 2d4461b to 4822fb1 Compare September 12, 2018 16:05

mrueg approved these changes Sep 13, 2018

View reviewed changes

GordonTheTurtle assigned johnstep Sep 27, 2018

thaJeztah added the area/security/apparmor label Oct 1, 2018

cyphar mentioned this pull request Nov 14, 2018

Failing to stop containers, if apparmor enabled. #36809

Closed

thaJeztah added the rebuild/* label Nov 14, 2018

GordonTheTurtle removed the rebuild/* label Nov 14, 2018

justincormack approved these changes Nov 14, 2018

View reviewed changes

thaJeztah added the rebuild/janky label Nov 14, 2018

GordonTheTurtle removed the rebuild/janky label Nov 14, 2018

thaJeztah approved these changes Nov 14, 2018

View reviewed changes

thaJeztah added status/4-merge and removed status/2-code-review labels Nov 14, 2018

cyphar mentioned this pull request Aug 11, 2020

apparmor: handle signal mediation containerd/containerd#4467

Merged

This was referenced Jan 29, 2021

[release-1.3] apparmor: handle signal mediation k3s-io/containerd#8

Merged

[release-1.4] apparmor: handle signal mediation k3s-io/containerd#9

Merged

oneiro-naut mentioned this pull request Aug 9, 2021

Policies not enforced correctly when HostPolicy is enabled for docker version 18.06 and earlier kubearmor/KubeArmor#223

Closed

apparmor: allow receiving of signals from 'docker kill' #37831

apparmor: allow receiving of signals from 'docker kill' #37831

Uh oh!

Conversation

cyphar commented Sep 12, 2018

Uh oh!

vdemeester commented Sep 12, 2018

Uh oh!

ericchiang commented Sep 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericchiang Sep 12, 2018

Choose a reason for hiding this comment

Uh oh!

jessfraz Sep 12, 2018

Choose a reason for hiding this comment

Uh oh!

cyphar Sep 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jessfraz Sep 12, 2018

Choose a reason for hiding this comment

Uh oh!

jessfraz commented Sep 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Sep 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ericchiang commented Sep 12, 2018

Uh oh!

mrueg left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Oct 1, 2018

Uh oh!

thaJeztah commented Oct 1, 2018

Uh oh!

cyphar commented Oct 3, 2018

Uh oh!

cyphar commented Nov 14, 2018

Uh oh!

thaJeztah commented Nov 14, 2018

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

ericchiang commented Sep 12, 2018 •

edited

Loading

cyphar Sep 12, 2018 •

edited

Loading

jessfraz commented Sep 12, 2018 •

edited

Loading

codecov bot commented Sep 12, 2018 •

edited

Loading