many: fix behavior of deny rules created as a result of a prompt reply #14369
Conversation
github-actions
bot
added
the
Needs Documentation -auto-
|
I've reproduced on a clean lxd VM prepared via |
|
Figured out the problem: whenever we receive a message from the kernel, we're getting the requested permission in both the I think this should not be happening, my understanding was that the allow field would only contain permissions explicitly allowed (not and an application somehow made a magic syscall which requested rwx at the same time, my understanding is we'd get a request which looks like this (please excuse the hand waving): {
path: "/path/to/foo",
allow: r,
deny: w | x,
}But the reality is that most requests from the kernel are for only a single permission, and the Here are some logs. The "received prompt request from kernel" messages are from the listener, and the "received from kernel requests channel" is the interfaces requests manager after it sees a new request from the listener (perhaps that log should instead say "received from listener requests channel") |
|
So the question is: is it intentional that the messages from the kernel have the (same) requested permissions in both the |
|
I think why this may be happening is that the operation is matched by both an allow rule and a prompt rule (which would have formerly been an allow rule, without prompt prefixes) in the AppArmor profile. But since deny has priority over allow, we get a prompt. But technically, there is still an allow rule, so we get the permission in both the allow and deny fields. |
olivercalder
added
the
Needs Samuele review
|
The previous commit should address the bug. I'll add some more robust tests to make sure the response message construction does exactly what we intend with respect to allowed and denied permissions. |
|
I'll also explore replacing the |
|
Is this a change from the kernel side that is breaking existing code in snapd or a bug introduced on the snapd as part of the recent merges? This was definitely working previously. |
olivercalder
force-pushed
the
prompting-fix-reply-deny-rule
branch
3 times, most recently
from
876e0b0
to
37eaded
Compare
github-actions
bot
removed
the
Needs Documentation -auto-
Before the recent merges, snapd (prompting branch) was being very simplistic about allowing and denying permissions: if the user denied any permission (via reply or rule), send a message to the kernel denying all originally-requested permissions, and if the user allowed all permissions via rules or via an explicit allow reply, send a message to the kernel allowing all originally-requested permissions. This approach was fine, but that meant that if there was an unrecognized file permission, snapd could implicitly allow it without the user being prompted for it. So we needed to only allow permissions which were explicitly allowed by snapd. This was addressed in to #14196, which was itself based on the earlier requestprompts PR #13981. The idea was that if a request was allowed, we should send back the following permissions as approved: But this bug didn't start manifesting in tests until we adjusted the handling of partially allowed requests as part of the interfaces requests manager PR #14316. If a new request has some permissions which are allowed by existing rules and at least one permission which is denied by existing rules, then we immediately send back a reply. That PR changed the behavior so that instead of sending back a denial of every permission (even the ones which were allowed), we instead send back an "allow" response containing only the permissions which were explicitly allowed, with the idea that the listener sorts out the permissions to the kernel so that only the allowed permissions are allowed and the rest are denied. Furthermore, we were relying on the kernel auto-denying any permissions which were not included in the allow or deny fields: https://github.com/canonical/snapd/pull/14316/files#diff-7a19d15563de3674607eeb9469c2abf6ec8ee481d1aef78a712fb7db86fec0c2R448 (some unfortunate foreshadowing). But the problem was that all the permissions were included in the |
|
|
||
| // If the same permission appears in both the allow and deny fields, | ||
| // treat it as initially denied. | ||
| apparmorAllowed := msg.Allow & ^msg.Deny |
There was a problem hiding this comment.
I think golang has directly a &^ operator as well
| resp.Allow = apparmorAllowed | (explicitlyAllowed & msg.Deny) | ||
| // Deny permissions which were initially denied and not explicitly allowed | ||
| // by the user. | ||
| resp.Deny = msg.Deny & ^explicitlyAllowed |
| if !allow { | ||
| // Remaining permissions were denied, so allow permissions which were | ||
| // previously allowed by prompt rules | ||
| allowedPerms = make([]string, 0, len(constraints.originalPermissions)-len(constraints.remainingPermissions)) |
There was a problem hiding this comment.
not for here but we should take a note to find another name for remainingPermissions because it doesn't clearly convey what they are, they are the permission we are prompting about (because no rules gave them already)
| // replyChan is a channel for writing the response. | ||
| replyChan chan *Response | ||
| // replyChan is a channel for sending the explicitly allowed permissions. | ||
| replyChan chan any |
There was a problem hiding this comment.
it makes me slightly uneasy to erase all type and pass around any here, as if we're plugging a leaky interface and then make it harder to follow the code. Since we already expose types from notify package, we may as well introduce a generic notify.Permissions interface with a single method such that any doesn't match anymore:
// in apparmor/notiry
type Permissions interface {
Kind() string // eg. returns "file"
String() string // eg. returns "file:rwxm"
}
In this case we'd have:
func AbstractPermissionsToAppArmorPermissions(iface string, permissions []string) (notify.Permissions, error) {
...
}
and then the listener package can do type assertion to a concrete type. FWIW, I think this can be a followup
| } | ||
|
|
||
| // If the same permission appears in both the allow and deny fields, | ||
| // treat it as initially denied. |
There was a problem hiding this comment.
it may be worth expanding the comment a little bit to mention that this is unexpected but it's also what the kernel gave us
There was a problem hiding this comment.
I want to wait to chat with JJ before adding that comment, in case it actually is expected and it's only unexpected by me :) but yes I agree
Signed-off-by: Oliver Calder <[email protected]>
If an operation was matched by both `allow` and `prompt` rules in the apparmor profile, the message from the kernel may contain overlapping permissions in the allow and deny fields. It is important that any such permissions are not auto-allowed, and that we do what the user specifies, either via existing request rules or a direct prompt reply. Signed-off-by: Oliver Calder <[email protected]>
When a prompt is denied, either because of a direct reply or because a rule was added which denies one of its remaining permissions, the prompt may have some originally-requested permissions which were previously allowed due to previous rules. We want to reply to the kernel that those previously-allowed permissions were allowed, but the newly-denied permissions and any permissions which were not previously allowed by a rule should be denied. The old `listener.Response` type had a field for whether the request should be allowed or denied, as well as the permissions for which that decision should apply. Because of the cases described above, users of this type often needed to send an "allow" response with the previously- allowed permissions, even if the reply which triggered the response was actually a denial. Furthermore, in the deny case, the permissions included in the response were ignored, and all originally-requested permissions were denied. This is clearly confusing. A much simpler approach is to reply directly with the permission(s) which should be allowed. In strictly "deny" cases where no permissions were previously allowed, the allowed permission sent as the response should be the empty permission for the corresponding AppArmor permission type -- in the case of a file, this is `notify.FilePermission(0)`. In the more common cases where there may be some previously-allowed permissions, any such permissions can be sent as the allowed permission, and any permissions which are not included in the allowed permission are denied by the listener. Signed-off-by: Oliver Calder <[email protected]> s/a/n/listener: accept nil allowed permission as allowing no permissions Signed-off-by: Oliver Calder <[email protected]> s/a/n/listener: test response behavior with different allowed permission Signed-off-by: Oliver Calder <[email protected]> s/a/n/listener: use the &^ operator instead of & ^ separately Signed-off-by: Oliver Calder <[email protected]>
olivercalder
force-pushed
the
prompting-fix-reply-deny-rule
branch
from
56311cb
to
5a350f2
Compare
When an application tries to carry out an operation and AppArmor mediates the operation via prompting, it sends to snapd a struct with an
AllowandDenyfield, among others. TheAllow field contains the bits for the permissions which the application already possessed, while theDeny` field contains the bits for the permissions which the application currently lacks.The listener and related code was originally written with the assumption that a permission could not occur in both the
AllowandDenyfields in the same request, and more generally, that any permission which occurred in theAllowfield should be granted regardless of the prompt decision. The thought process was that this field was permissions which were matched by allow rules and were not in need of mediation.However, in testing, requests from the kernel usually, if not always, receive the same permissions in both the
AllowandDenyfields. This may be a bug, or it may be that the AppArmor profile happens to have both allow rules and prompt rules, with the prompt rules causing the permissions to be placed in theDenyfield and passed to snapd.Regardless, the solution to the immediate bug is relatively simple: if a permission occurs in both the
AllowandDenyfields, ignore the fact that it was originally allowed, and treat it just like any other permission in theDenyfield. When constructing the response message to send back to the kernel, the allowed permissions should be those from the requestAllowfield and not the requestDenyfield, along with any permissions fromDenywhich were explicitly allowed by the user. The denied permissions in the response should be any permissions from the messageDenyfield which were not explicitly allowed by the user. Thus, all permissions from the original request are included in the response, and we never allow previously denied permissions without explicit user consent.This new design relies on the idea that replies always contain the set of permissions which were explicitly granted by the user. However, the existing mechanism for sending a reply to the listener is via a
Responsestruct which has fields for outcome and permissions. Even when the user replies with a denial, we want to allow any permissions which were allowed by request rules prior to the eventual denial which resulted in the response to the kernel. Using an allow outcome in order to send the subset of allowed permissions even when the reply was a denial is confusing at best, and certainly superfluous, as the existing implementation ignored the replied permissions whenever the outcome was deny.A cleaner implementation of the response mechanism forgoes the outcome field entirely, and instead simply takes the set of permissions that the user explicitly allowed and passes that to the listener, which then sorts out denying any permissions which were not explicitly allowed. This requires more than the minimum set of changes to fix the bug, but should simplify and clarify the code and its handling of allowed and denied permissions.
This is tracked internally by https://warthogs.atlassian.net/browse/SNAPDENG-30187