interfaces/apparmor: allow reading from ecryptfs #2837

zyga · Feb 13, 2017

This patch changes the base apparmor template to allow reading from
ecryptfs. This seems to be required when a snap with a hook lives on a
$HOME directory that is on a crypted filesystem. When that snap is in
try mode the actual files will be encrypted and will end up being
resolved as a set of encrypted files under the /home directory. A
similar patch exists in snap-confine's apparmor profile for the very
same reason.

Fixes: https://bugs.launchpad.net/snapd/+bug/1637596
Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com

mvo5 · Feb 13, 2017

mvo5 approved these changes Feb 13, 2017

View changes

\o/ - but needs a review from @jdstrand as I'm not sure about the security implications (if any).

jdstrand · Feb 13, 2017

jdstrand reviewed Feb 13, 2017

View changes

Looking at the bug, it has:

audit: type=1400 audit(1477673355.022:268): apparmor="DENIED" operation="open" profile="snap.snapctl-hooks.hook.configure" name="/home/.ecryptfs/kyrofa/.Private/ECRYPTFS_FNEK_ENCRYPTED.FBaOD17IzZ9EsEQj239cjqBho2j-3yXXfzRq3GXkhDoVeoqkJlBFyivegA--/ECRYPTFS_FNEK_ENCRYPTED.FBaOD17IzZ9EsEQj239cjqBho2j-7yXXtzRh82OVjVmaBO0JsdHlqK7iEk--/ECRYPTFS_FNEK_ENCRYPTED.FBaOD17IzZ9EsEQj239cjqBho2j-3yXXfzRq3GXkhDoVeoqkJlBFyivegA--/ECRYPTFS_FNEK_ENCRYPTED.FBaOD17IzZ9EsEQj239cjqBho2j-7yXXtzRhhrBZUIEB0brHV1omVyCU.k--/ECRYPTFS_FNEK_ENCRYPTED.FBaOD17IzZ9EsEQj239cjqBho2j-3yXXfzRq3GXkhDoVeoqkJlBFyivegA--/ECRYPTFS_FNEK_ENCRYPTED.FBaOD17IzZ9EsEQj239cjqBho2j-7yXRhhrBA.pAGc4OcPwbPu1IZnkngk--/ECRYPTFS_FNEK_ENCRYPTED.FBaOD17IzZ9EsEQj239cjqBho2j-3yXXfzRq3GXkhDoVeoqkJlBFyivegA.Wak--/ECRYPTFS_FNEK_ENCRYPTED.FBaOD17IzZ9EsEQj239cjqBho2j-7yXXtzRhU.DKB3vJS4NnysPO9.RZ1U--/ECRYPTFS_FNEK_ENCRYPTED.FBaOD17IzZ9EsEQj239cjqBho2j-3yXXtzRhCckA-2z2BEUruD7rA-mb0U--/ECRYPTFS_FNEK_ENCRYPTED.FBaOD17IzZ9EsEQj239cjqBho2j-7yXXtzRhx3A

This output appears truncated, but this path is important: /home/.ecryptfs/kyrofa/.Private/ECRYPTFS_FNEK_ENCRYPTED.FBaOD17IzZ9EsEQj239cjqBho2j-3yXXfzRq3GXkhDoVeoqkJlBFyivegA--/ECRYPTFS. /etc/apparmor.d/abstractions/base has a rule that matches this path:

  # new-style encrypted $HOME
  owner @{HOMEDIRS}/.ecryptfs/*/.Private/** mrixwlk,

Since you said that this is the configure hook, we know that the configure hook runs as root, we see that the rules added to this PR effectively only remove 'owner', and we see that the root process is trying to access encrypted files in /home/.ecryptfs/kyrofa/.Private/.... I'm not sure of the cause, but it seems like the snap in the bug is trying to have a 'daemon' access files in the user's directory. We don't allow non-owner match in the default policy or the home interface now, and we shouldn't allow that in the ecryptfs policy.

@kyrofa - I expect this configure hook will fail on non-encrypted HOME systems. Does it? If not, can you strace the process and see what file it is accessing?

-1 as is without more information.

@jdstrand the snap in question is contained in the snapd codebase. You can see the configure hook doesn't do anything other than run snapctl a few times. Note also that this works fine if the snap is installed-- the problems come when the snap is being tried. I assume that's where the access to my $HOME is coming from (since the snap actually resides there).

@jdstrand to answer your question: this works fine on a non-encrypted $HOME. However, things have changed a little since that bug was logged: since the configure hook now runs upon initial install, I get these errors when running snap try instead of snap set. straceing just straces the snap client, where we'd actually want to strace snapd itself. I'm not sure how to do that.

jdstrand · Feb 13, 2017

Adding 'Request changes' so this doesn't accidentally get committed.

kyrofa · Feb 15, 2017

Adding 'Request changes' so this doesn't accidentally get committed.

I don't think that worked the way you hoped 😉 .

niemeyer · Feb 16, 2017

niemeyer requested changes Feb 16, 2017

View changes

Requesting changes per @jdstrand note above

jdstrand · Feb 16, 2017

A simple reproducer is to create a user with encrypted home (sudo adduser --encrypt-home foo) then install a snap with 'snap try' and run sudo snap run --shell your.cmd. Then you see:

Feb 16 08:47:04 sec-xenial-amd64 kernel: [  424.347885] audit: type=1400 audit(1487256424.804:29): apparmor="DENIED" operation="capable" profile="snap.strict.sh" pid=9869 comm="snap-exec" capability=1  capname="dac_override"
Feb 16 08:47:04 sec-xenial-amd64 kernel: [  424.347890] audit: type=1400 audit(1487256424.804:30): apparmor="DENIED" operation="capable" profile="snap.strict.sh" pid=9869 comm="snap-exec" capability=2  capname="dac_read_search"
Feb 16 08:53:40 sec-xenial-amd64 kernel: [  819.711510] audit: type=1400 audit(1487256820.174:33): apparmor="DENIED" operation="capable" profile="snap.strict.sh" pid=10151 comm="snap-exec" capability=1  capname="dac_override"
Feb 16 09:00:45 sec-xenial-amd64 kernel: [ 1245.372000] audit: type=1400 audit(1487257245.841:37): apparmor="DENIED" operation="open" profile="snap.strict.sh" name="/home/.ecryptfs/foo/.Private/ECRYPTFS_FNEK_ENCRYPTED.FWZVitsLJyTlPkSbDZAu3WSsNMMuO.6YHGr-QpTPgIL4Jvwe265BklmrAk--/ECRYPTFS_FNEK_ENCRYPTED.FWZVitsLJyTlPkSbDZAu3WSsNMMuO.6YHGr-nKUseO7FOiXyipko.p2oVk--/ECRYPTFS_FNEK_ENCRYPTED.FWZVitsLJyTlPkSbDZAu3WSsNMMuO.6YHGr-N81FsjwOsxgTIOE4u-FKbU--/ECRYPTFS_FNEK_ENCRYPTED.FWZVitsLJyTlPkSbDZAu3WSsNMMuO.6YHGr-MB6124cb92gwPFQ9O0.whk--" pid=10297 comm="snap-exec" requested_mask="wr" denied_mask="wr" fsuid=0 ouid=1001

Rather than updating the policy for all snaps to weaken the policy by removing 'owner' matching for ecryptfs files and adding capabilities, I suggest conditionally adding the following policy if both running under snap try and the directory for snap try is under ecryptfs. Then you can do:

# Workaround policy added by 'snap try' to support ecryptfs and
# /var/lib/snapd/snaps/<name>_xN.snap pointing inside user's directory
capability dac_override,
capability dac_read_search,
# encrypted ~/.Private and old-style encrypted $HOME
@{HOME}/.Private/** mrixwlk,
# new-style encrypted $HOME
@{HOMEDIRS}/.ecryptfs/*/.Private/** mrixwlk,

We want the 'if running under ecryptfs' check because these rules aren't needed with normal 'snap try' and adding dac_override and dac_read_search changes the policy in a way that diverges from proper strict mode.

Note snap try has two other issues when snap try is used under ecryptfs that are not easily solved:

Remembering that the decrypted home is only mounted when the user logs in, if a user with encrypted home uses 'snap try', all commands are unavailable to everyone until that user logs in. This breaks daemons on boot and cli commands by all users
if a user with encrypted home uses 'snap try', when this user is logged in, that snap's commands are not available to non-root users on the system, because with encrypted home, that user's home directory is set to 0700 instead of the default 0755

Point '2' is also a problem on multiuser systems where the 'snap try'ing user has a more restrictive home directory than the default on Ubuntu. This would affect other distros or sites who have stricter defaults for their home directories than Ubuntu.

jdstrand · Feb 16, 2017

jdstrand requested changes Feb 16, 2017

View changes

Please adjust snapd to conditionally add the policy as mentioned in my previous comment instead.

I think it is probably sufficient to document the limitations of 'snap try' with multiuser, so documenting that in the code you add and in documentation would be helpful.

kyrofa · Mar 20, 2017

@zyga what is blocking this?

zyga · May 4, 2017

This is no longer blocked. I implemented the suggestion from @jdstrand and will follow up with spread tests.

jdstrand · May 4, 2017

jdstrand requested changes May 4, 2017

View changes

Please see inline comments. The direction is mostly correct in that the snippets are only being added if in try mode and with ecryptfs present. Please fix the ecryptfs detection though. Thanks!

'daemons, hooks, and snap commands run under sudo'

Done

''if ecryptfs is mounted anywhere'

Corrected

"once that information becomes available"? At the time of 'snap try' invocation we know the user and can therefore determine if we should add the snippets. We just need to store that off somewhere so that future refreshes can check on whether or not to add these snippets. This is most correct but also makes the feature robust in the face of snap refresh happening when no users are logged in. (Not to mention, ecryptfs might be used on the system for something other than encrypted home and this would unconditionally add the weakened policy to the snap when it doesn't need to).

Right, it's all there but we don't have it in snapd, it's thrown away the moment the task is complete. I will work wit @chipaca to fix this so that we can reliably detect the right condition.

// With non-existent mountinfo we say nothing is encrypted.

Done

Please use this:

# Workaround policy added by 'snap try' to support ecryptfs and
# /var/lib/snapd/snaps/<name>_xN.snap pointing inside user's directory
capability dac_override,
capability dac_read_search,
# encrypted ~/.Private and old-style encrypted $HOME
@{HOME}/.Private/ r,
@{HOME}/.Private/** mrixwlk,
# new-style encrypted $HOME
@{HOMEDIRS}/.ecryptfs/*/.Private/ r,
@{HOMEDIRS}/.ecryptfs/*/.Private/** mrixwlk,

Ooops, thanks! Corrected

zyga · May 8, 2017

So this is still slightly blocked on how we handle the sequence state data and patches. I'll mark it as such for now.

chipaca · Jul 10, 2017

What's the state of this PR?

zyga · Jul 13, 2017

This PR needs a 2nd deep jump into the sequence state so that we can store extra data there. I think that we don't have any time to do this (now). I can close the PR

chipaca · Aug 25, 2017

Closing until time is found to work on this.

zyga requested a review from jdstrand Feb 13, 2017

zyga added the Blocked label Feb 22, 2017

zyga removed the Blocked label May 4, 2017

zyga added the Blocked label May 8, 2017

chipaca added the Decaying label Jul 10, 2017

chipaca closed this Aug 25, 2017

+        +				// Add a special internal snippet for snaps that are in try
+        +				// mode on an encrypted home directory. This snippet provides
+        +				// access to the encrypted files for users other than the owner
+        +				// and thus allows daemons and hooks to run correctly.

snapcore/snapd

interfaces/apparmor: allow reading from ecryptfs #2837

zyga commented Feb 13, 2017

zyga requested a review from jdstrand Feb 13, 2017

mvo5 approved these changes Feb 13, 2017

View changes

jdstrand reviewed Feb 13, 2017

View changes

jdstrand commented Feb 13, 2017

kyrofa commented Feb 15, 2017 •

Edited 1 time

kyrofa
Feb 15, 2017

niemeyer requested changes Feb 16, 2017

View changes

jdstrand commented Feb 16, 2017 •

Edited 1 time

jdstrand
Feb 16, 2017

jdstrand requested changes Feb 16, 2017

View changes

zyga added the Blocked label Feb 22, 2017

kyrofa commented Mar 20, 2017

zyga added some commits May 4, 2017

zyga removed the Blocked label May 4, 2017

zyga commented May 4, 2017

jdstrand requested changes May 4, 2017

View changes

zyga added some commits May 4, 2017

zyga commented May 8, 2017

zyga added the Blocked label May 8, 2017

chipaca commented Jul 10, 2017

chipaca added the Decaying label Jul 10, 2017

zyga commented Jul 13, 2017

chipaca commented Aug 25, 2017

chipaca closed this Aug 25, 2017

snapcore/snapd

Join GitHub today

interfaces/apparmor: allow reading from ecryptfs #2837

Conversation

zyga commented Feb 13, 2017

All checks have passed

zyga requested a review from jdstrand Feb 13, 2017

mvo5 approved these changes Feb 13, 2017 View changes

jdstrand reviewed Feb 13, 2017 View changes

jdstrand Feb 13, 2017 • edited Edited 1 time jdstrand Feb 13, 2017

kyrofa Feb 13, 2017 • edited Edited 1 time kyrofa Feb 13, 2017

jdstrand commented Feb 13, 2017

kyrofa commented Feb 15, 2017 • edited Edited 1 time kyrofa Feb 15, 2017

niemeyer requested changes Feb 16, 2017 View changes

jdstrand commented Feb 16, 2017 • edited Edited 1 time jdstrand Feb 16, 2017

jdstrand requested changes Feb 16, 2017 View changes

zyga added the Blocked label Feb 22, 2017

kyrofa commented Mar 20, 2017

zyga added some commits May 4, 2017

All checks have failed

zyga removed the Blocked label May 4, 2017

zyga commented May 4, 2017

jdstrand requested changes May 4, 2017 View changes

zyga added some commits May 4, 2017

All checks have failed

zyga commented May 8, 2017

zyga added the Blocked label May 8, 2017

All checks have passed

chipaca commented Jul 10, 2017

chipaca added the Decaying label Jul 10, 2017

zyga commented Jul 13, 2017

chipaca commented Aug 25, 2017

chipaca closed this Aug 25, 2017

mvo5 approved these changes Feb 13, 2017

View changes

jdstrand reviewed Feb 13, 2017

View changes

jdstrand Feb 13, 2017 •

Edited 1 time

jdstrand
Feb 13, 2017

kyrofa Feb 13, 2017 •

Edited 1 time

kyrofa
Feb 13, 2017

kyrofa commented Feb 15, 2017 •

Edited 1 time

kyrofa
Feb 15, 2017

niemeyer requested changes Feb 16, 2017

View changes

jdstrand commented Feb 16, 2017 •

Edited 1 time

jdstrand
Feb 16, 2017

jdstrand requested changes Feb 16, 2017

View changes

jdstrand requested changes May 4, 2017

View changes