Project

General

Profile

Feature #8007

Self-audit our AppArmor profiles

Added by intrigeri almost 5 years ago. Updated about 4 years ago.

Status:
Resolved
Priority:
Elevated
Assignee:
-
Category:
-
Target version:
Start date:
Due date:
% Done:

100%

Feature Branch:
bugfix/8007-AppArmor-hardening
Type of work:
Audit
Starter:
Affected tool:

Description

Let's audit the AppArmor profiles we ship. Once we've done that ourselves and fixed the worst problems, we can ask other people to audit them.


Related issues

Related to Tails - Feature #8004: Basic AppArmor support Resolved 10/05/2014
Related to Tails - Bug #9370: tor-browser wrapper script offers avenues for arbitrary code execution to e.g. an exploited Pidgin Resolved 05/11/2015
Related to Tails - Bug #9534: Tighten AppArmor policy Resolved 06/04/2015

Associated revisions

Revision bc491c9e (diff)
Added by intrigeri over 4 years ago

live-boot: don't mount tmpfs twice on /live/overlay.

With live-boot 3.0.1-1 in Tails 1.4~rc1, /live/overlay appears to be empty both
with ls and df. It feels wrong, because that's where the read-write branch of
our aufs mount lives, and there must be data in there.

We actually have two tmpfs mounted there, as live-boot mounts stuff twice on
/live/overlay:

- with `mount -t tmpfs tmpfs /live/overlay` (line 159 in `9990-overlay.sh`)
- with `mount -t tmpfs -o rw,noatime,mode=755 tmpfs /live/overlay` (same file,
line 296)

And then, shortly after, it tries to unmount one of those, leading to a `umount:
can't umount /live/overlay: Device or resource busy` message one can see in
`/var/log/live/boot.log`. My understanding is that the top-most mountpoint
umount sees is the one used for the root aufs).

Then, live-boot runs `mount -o move /live/overlay /root/lib/live/mount/overlay`
twice, which is expected given live-boot does that for every mount (since
commit d2b2a461 in live-boot Git).

With all this in mind, my understanding is that the first tmpfs that was mounted
is `mount -o move`'d last, so it hides the one that was mounted last, which is
actually the one that's used as the read-write branch of the root filesystem's
union mount.

This raises two practical problems:

1. One cannot inspect how much space is used, at a given time, in the read-write
branch of the root filesystem's union mount. Probably almost nobody cares,
expect developers who want to analyze performance issues / resource
consumption, and power users who want to check whether they have enough room
left e.g. to download a large file. Admittedly, if this problem was the only
one, I would not have bothered spending so much time in live-boot's source
code to go through this analysis. Sadly, this is not the case:

2. As far as AppArmor policy is concerned, in theory the more files are hidden,
the better. However:

  • if this (arguably buggy) live-boot behavior change under our feet in the
    future, then our AppArmor policy may suddenly be more open than we would
    like; therefore, it would be better to fix that bug ourselves now, so that
    we can analyze the consequences of exposing the read-write branch's
    content, and adjust our AppArmor policy accordingly at the same time;
  • I'm not 100% sure how AppArmor policy applies to such stacked mounts: e.g.
    if some file was left open in an underlying mount, its file handle might be
    used by some application to access other files (that we might otherwise
    believe are not accessible). This kind of things is pretty hard for me to
    reason about (I lack the corresponding low-level system skills). So, better
    expose these files, and then we'll be able to actually test and confirm
    what's the real-world effect of potentially letting applications access
    them (mediated by DAC and MAC, of course), and again, adjust DAC
    permissions and AppArmor policy as needed.

Now, about the actual change. Ideally we would "simply" invert the order of the
`mount -o move' operations. However, the two mounts have the same mountpoint,
and the only way we have to differentiate them is the options they were
mounted with. Given another programming language, I would just do it, but
with POSIX shell, oh well.

So, this patch simply comments out the first command that mounts a tmpfs on
/live/overlay. This is not applicable for upstream, because the large chunk of
code that's before this command and the other tmpfs mount on /live/overlay needs
such an active mount in use cases we don't care about, and even if it did not,
then this could change in the future -- and anyway, there must be a good reason
for mounting /live/overlay this early in the general case, right? :) But for us
it's OK, because the only operations before these two mount commands are about
persistence enabled at initramfs time, which we do not support and explicitly
disable on the kernel command-line.

Refs: #8007

History

#1 Updated by intrigeri almost 5 years ago

  • Description updated (diff)

#2 Updated by intrigeri almost 5 years ago

#3 Updated by intrigeri over 4 years ago

  • Description updated (diff)

#4 Updated by intrigeri over 4 years ago

  • Description updated (diff)

#5 Updated by intrigeri over 4 years ago

  • Description updated (diff)

#6 Updated by intrigeri over 4 years ago

As part of this review, I'll add comments wherever I feel it's needed in the patches we apply to AppArmor profiles.

#7 Updated by intrigeri over 4 years ago

  • Priority changed from Normal to Elevated
  • Target version changed from Tails_1.3 to Tails_1.3.2

Clearly, I won't make it in time for 1.3 => postponing, and thus raising priority. Meh.

#8 Updated by intrigeri over 4 years ago

  • Target version changed from Tails_1.3.2 to Tails_1.4

#9 Updated by intrigeri over 4 years ago

  • Description updated (diff)
  • Blueprint set to https://tails.boum.org/blueprint/audit_AppArmor_profiles/

(Moved useful bits from ticket desc. to the blueprint.)

#10 Updated by intrigeri over 4 years ago

  • Status changed from Confirmed to In Progress

#11 Updated by intrigeri over 4 years ago

  • Feature Branch set to bugfix/8007-AppArmor-hardening

#12 Updated by intrigeri over 4 years ago

  • % Done changed from 0 to 10

#13 Updated by intrigeri over 4 years ago

  • Description updated (diff)

#14 Updated by intrigeri over 4 years ago

  • Related to Bug #9370: tor-browser wrapper script offers avenues for arbitrary code execution to e.g. an exploited Pidgin added

#15 Updated by intrigeri over 4 years ago

  • Target version changed from Tails_1.4 to Tails_1.4.1

The first issue that was discovered through this work (#9370) was merged for 1.4. Postponing the rest again.

#16 Updated by intrigeri about 4 years ago

  • Related to Bug #9533: Tighten Evince AppArmor policy added

#17 Updated by intrigeri about 4 years ago

  • Related to deleted (Bug #9533: Tighten Evince AppArmor policy)

#18 Updated by intrigeri about 4 years ago

  • Related to Bug #9534: Tighten AppArmor policy added

#19 Updated by intrigeri about 4 years ago

  • Status changed from In Progress to Resolved
  • Assignee deleted (intrigeri)
  • % Done changed from 10 to 100

I'm now done with everything I wanted to check. Audit results are on the blueprint, and follow-ups go to #9534.

Also available in: Atom PDF