interfaces/apparmor: conditionally add explicit deny rules for ptrace #5980

jdstrand · 2018-10-12T22:19:52Z

AppArmor deny rules cannot be undone by allow rules which makes deny
rules difficult to work with arbitrary combinations of interfaces.
Sometimes it is useful to suppress noisy denials and because that can
currently only be done with explicit deny rules, adding explicit deny
rules unconditionally makes it difficult for interfaces to be used in
combination. Define suppressPtraceTrace to allow an interface to request
suppression and define usesPtraceTrace to omit the explict deny rules
such that:

  if suppressPtraceTrace && !usesPtraceTrace {
      add 'deny ptrace (trace),'
  }

Add UsesPtraceTrace() and SuppressPtraceTrace() that can be called in
the interfaces to set the above and adjust addContent() to use the above
logic to conditionally add a single explicit deny ptrace (trace) rule.

Adjust system-observe and network-observe to suppress via the common
interface and browser-support to use the new SuppressPtraceTrace().

With the above, the policy is equivalent to what was generated before
(except we only have a single explicit denial rather than several when
the above 3 interfaces are all connected at once). Future PRs will
adjust other interfaces to use UsesPtraceTrace() as needed so these
interfaces can still ptrace but continue to use the other interfaces.

chipaca · 2018-10-12T23:20:40Z

interfaces/apparmor/spec.go:54:32: "explict" is a misspelling of "explicit"

AppArmor deny rules cannot be undone by allow rules which makes deny rules difficult to work with arbitrary combinations of interfaces. Sometimes it is useful to suppress noisy denials and because that can currently only be done with explicit deny rules, adding explicit deny rules unconditionally makes it difficult for interfaces to be used in combination. Define suppressPtraceTrace to allow an interface to request suppression and define usesPtraceTrace to omit the explicit deny rules such that: if suppressPtraceTrace && !usesPtraceTrace { add 'deny ptrace (trace),' } Add UsesPtraceTrace() and SuppressPtraceTrace() that can be called in the interfaces to set the above and adjust addContent() to use the above logic to conditionally add a single explicit deny ptrace (trace) rule. Adjust system-observe and network-observe to suppress via the common interface and browser-support to use the new SuppressPtraceTrace(). With the above, the policy is equivalent to what was generated before (except we only have a single explicit denial rather than several when the above 3 interfaces are all connected at once). Future PRs will adjust other interfaces to use UsesPtraceTrace() as needed so these interfaces can still ptrace but continue to use the other interfaces.

zyga

LGTM with some ideas on how to simplify the backend processing and a suggestion on naming the new functions and variables, IMO, more in the spirit of go naming. Please look at the question about jalimode though, I think this is a mistake given how jail mode was supposed to be a way to request strict.

zyga · 2018-10-15T07:42:21Z

interfaces/apparmor/backend.go

@@ -419,12 +419,12 @@ func (b *Backend) deriveContent(spec *Specification, snapInfo *snap.Info, opts i
 	// Add profile for each app.
 	for _, appInfo := range snapInfo.Apps {
 		securityTag := appInfo.SecurityTag()
-		addContent(securityTag, snapInfo, opts, spec.SnippetForTag(securityTag), content)
+		addContent(securityTag, snapInfo, opts, spec.SnippetForTag(securityTag), content, spec)


Since we now pass both spec and securityTag into addContent we could make it call SnippetForTag internally. I can do this as a follow up, not to block this.

That sounds fine in a followup PR.

zyga · 2018-10-15T07:43:28Z

interfaces/apparmor/backend_test.go

+			suppress: true,
+			expected: false,
+		},
+		// classic with jail, never suppress


Hmm, wasn't classic with jail supposed to be the same as strict?

zyga · 2018-10-15T07:46:47Z

interfaces/apparmor/spec.go

@@ -477,3 +491,21 @@ func (spec *Specification) AddPermanentSlot(iface interfaces.Interface, slot *sn
 	}
 	return nil
 }
+
+// UsesPtraceTrace records when to omit explicit ptrace deny rules
+func (spec *Specification) UsesPtraceTrace() {


I would have expected UsesPtraceTrace to return bool and not set any internal state. How about calling the set UsePtraceTrace, SupressPtraceTrace. Since this is all a part of the package we could drop the getter functions entirely and just either expose a single public function that computes uses && !suppress or just access the fields directly.

I agree with zyga, this is a bit unusual in terms of go style. Most getters of the form "GetFoo() bool" are simply called "Foo() bool". Maybe something along the lines of what zyga suggested, e.g. http://paste.ubuntu.com/p/chSKC2jSBp/ ? This diff would add an explicit Set{Uses,Suppresses}PtraceTrace which simplifies some if/else. I kept the Get* to keep the diff small but I would like to rename them as zyga suggested. Happy to work on this fwiw.

I'm fine with reorganizing as requested. I'd prefer not to have Set* allow setting both ways because the interfaces will only need to call it with true. If they call it with anything else, then it could introduce difficult to diagnose bugs where sometimes the rule is added and sometimes not based on on interface connection ordering. Eg, interface foo uses SetSuppressPtraceTrace(true), then interface bar does SetSuppressPtraceTrace(false). We'll get different behavior depending on which one snapd decides to connect first. Unless you strongly feel otherwise, I'll leave the one-way behavior (but still name it Set*).

@zyga - regarding dropping getter, I use them in common_test.go and felt this was cleaner.

I noticed but for such cases we can use export_test.go, though at this stage it's not a strong feeling.

@zyga - I recall having some trouble with mocking certain things in the PRs though I can't recall if this was one of them. If you don't have a strong feeling that it is a blocker, I suggest letting it in as is (it might be useful down the line). If it is strong enough for you wanting me to do a followup PR, I can.

zyga · 2018-10-15T07:48:09Z

interfaces/apparmor/template.go

@@ -566,6 +566,19 @@ var overlayRootSnippet = `
  "###UPPERDIR###/{,**/}" r,
 `

+var ptraceTraceDenySnippet = `


The comment is very nice and informational. Thank you! I love that you included the commit ID for the relevant mainline bug fix.

zyga · 2018-10-15T07:49:34Z

interfaces/builtin/common.go

@@ -55,6 +55,9 @@ type commonInterface struct {
 	connectedSlotKModModules []string
 	permanentPlugKModModules []string
 	permanentSlotKModModules []string
+
+	usesPtraceTrace     bool


If you follow my naming suggestion above this could become

usePtraceTrace bool suppressPtraceTrace bool

I chose 'usesPtraceTrace' since the interface is declaring it uses it. I chose 'suppressPtraceTrace' because the interface is saying "please supress the ptrace trace denials if you can". I can rename things if you still want.

zyga · 2018-10-15T07:50:21Z

tests/main/interfaces-browser-support/task.yaml

@@ -106,6 +106,14 @@ execute: |
    fi

    if [ "$(snap debug confinement)" = strict ] ; then
+        if [ "$ALLOW_SANDBOX" = "false" ]; then


Thank you for updating the spread tests to capture the improvement!

mvo5

Looks great, thanks for working on this! I have some inline ideas to make slightly more go-ish, happy to work on those and push to this (or a follow) if you like them.

mvo5 · 2018-10-15T08:45:47Z

interfaces/apparmor/spec.go

@@ -477,3 +491,21 @@ func (spec *Specification) AddPermanentSlot(iface interfaces.Interface, slot *sn
 	}
 	return nil
 }
+
+// UsesPtraceTrace records when to omit explicit ptrace deny rules
+func (spec *Specification) UsesPtraceTrace() {


I agree with zyga, this is a bit unusual in terms of go style. Most getters of the form "GetFoo() bool" are simply called "Foo() bool". Maybe something along the lines of what zyga suggested, e.g. http://paste.ubuntu.com/p/chSKC2jSBp/ ? This diff would add an explicit Set{Uses,Suppresses}PtraceTrace which simplifies some if/else. I kept the Get* to keep the diff small but I would like to rename them as zyga suggested. Happy to work on this fwiw.

mvo5 · 2018-10-15T08:47:17Z

interfaces/builtin/common_test.go

+	c.Assert(spec.GetSuppressPtraceTrace(), Equals, true)
+
+	// setting both, only uses is set
+	iface = &commonInterface{


See above, not sure we need this special case.

mvo5 · 2018-10-15T08:48:05Z

interfaces/builtin/common.go

@@ -86,6 +89,11 @@ func (iface *commonInterface) BeforePrepareSlot(slot *snap.SlotInfo) error {
 }

 func (iface *commonInterface) AppArmorConnectedPlug(spec *apparmor.Specification, plug *interfaces.ConnectedPlug, slot *interfaces.ConnectedSlot) error {
+	if iface.usesPtraceTrace {
+		spec.UsesPtraceTrace()
+	} else if iface.suppressPtraceTrace {


I'm not sure why we need this? It seems like the apparmor/backend.go is doing the right thing if both are set. Not sure what this extra special case buys us. Or am I missing something?

I did it this way because there is no point setting uses with suppress since uses always trumps suppress and figured the common interface should reflect best practices. Happy to change if you prefer.

Should the interface complain that both are set? I think this is orthogonal to the problem at hand, just about internal API usability. I think I agree with mvo, it's probably better as a "bunt" pusher of options than extra logic.

I can reduce this in the follow-up PR.

jdstrand · 2018-10-15T20:08:11Z

@mvo5 and @zyga - I updated the PR and responded to feedback. I think the is ready for another review.

zyga · 2018-10-16T18:03:26Z

I think this is fine now but let me look quickly again.

zyga

LGTM, some comments for follow-up

zyga · 2018-10-16T18:07:34Z

interfaces/apparmor/backend.go

@@ -553,6 +553,17 @@ func addContent(securityTag string, snapInfo *snap.Info, opts interfaces.Confine
 					tagSnippets += snippet
 				}
 			}
+
+			if !ignoreSnippets {


This is fine, it is done after we've seen all the snippets for a given snap.

zyga · 2018-10-16T18:09:44Z

interfaces/apparmor/backend_test.go

+			suppress: true,
+			expected: false,
+		},
+		// classic, never suppress


zyga · 2018-10-16T18:11:51Z

interfaces/builtin/common.go

@@ -86,6 +89,11 @@ func (iface *commonInterface) BeforePrepareSlot(slot *snap.SlotInfo) error {
 }

 func (iface *commonInterface) AppArmorConnectedPlug(spec *apparmor.Specification, plug *interfaces.ConnectedPlug, slot *interfaces.ConnectedSlot) error {
+	if iface.usesPtraceTrace {
+		spec.UsesPtraceTrace()
+	} else if iface.suppressPtraceTrace {


Should the interface complain that both are set? I think this is orthogonal to the problem at hand, just about internal API usability. I think I agree with mvo, it's probably better as a "bunt" pusher of options than extra logic.

I can reduce this in the follow-up PR.

jdstrand · 2018-10-16T18:21:55Z

Thanks for the reviews and merge! :)

jdstrand force-pushed the conditionally-add-explicit-ptrace-deny-rules branch from bfb0723 to c751355 Compare October 13, 2018 03:49

jdstrand mentioned this pull request Oct 13, 2018

interfaces/many: updates to support k8s worker nodes #5981

Merged

update spread tests for interfaces that use SuppressPtraceTrace()

b08e890

jdstrand mentioned this pull request Oct 14, 2018

interfaces/many: use 'unsafe' with docker-support change_profile rules #5982

Merged

mvo5 added this to the 2.36 milestone Oct 15, 2018

zyga approved these changes Oct 15, 2018

View reviewed changes

mvo5 approved these changes Oct 15, 2018

View reviewed changes

Jamie Strandboge added 2 commits October 15, 2018 19:27

suppress can happen in jail mode too. Thanks to zyga

aecfe3a

use Go-style Setters and Getters. Thanks zyga and mvo

40b488d

zyga approved these changes Oct 16, 2018

View reviewed changes

zyga merged commit fa139d0 into snapcore:master Oct 16, 2018

jdstrand deleted the conditionally-add-explicit-ptrace-deny-rules branch December 4, 2018 14:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

interfaces/apparmor: conditionally add explicit deny rules for ptrace #5980

interfaces/apparmor: conditionally add explicit deny rules for ptrace #5980

jdstrand commented Oct 12, 2018 •

edited by zyga

chipaca commented Oct 12, 2018

zyga left a comment •

edited

zyga Oct 15, 2018

jdstrand Oct 15, 2018

zyga Oct 15, 2018

jdstrand Oct 15, 2018

zyga Oct 15, 2018

mvo5 Oct 15, 2018

jdstrand Oct 15, 2018

jdstrand Oct 15, 2018

jdstrand Oct 15, 2018

zyga Oct 16, 2018

jdstrand Oct 16, 2018

zyga Oct 15, 2018

zyga Oct 15, 2018

jdstrand Oct 15, 2018

zyga Oct 15, 2018

mvo5 left a comment

mvo5 Oct 15, 2018

mvo5 Oct 15, 2018

mvo5 Oct 15, 2018

jdstrand Oct 15, 2018

zyga Oct 16, 2018

jdstrand commented Oct 15, 2018

zyga commented Oct 16, 2018

zyga left a comment

zyga Oct 16, 2018

zyga Oct 16, 2018

zyga Oct 16, 2018

jdstrand commented Oct 16, 2018

interfaces/apparmor: conditionally add explicit deny rules for ptrace #5980

interfaces/apparmor: conditionally add explicit deny rules for ptrace #5980

Conversation

jdstrand commented Oct 12, 2018 • edited by zyga

chipaca commented Oct 12, 2018

zyga left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvo5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdstrand commented Oct 15, 2018

zyga commented Oct 16, 2018

zyga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdstrand commented Oct 16, 2018

jdstrand commented Oct 12, 2018 •

edited by zyga

zyga left a comment •

edited