apparmor: allow to read /proc/self/map_files

[sergio-costas]

Accesing this folder is needed for mangohud. Since the info there is basically the same than in /proc/self/maps, but it's easier to access because the program doesn't have to parse it, it should be safe.

[alexmurray]

Hmm unfortunately there is no way to restrict this to just processes of the snap itself - as such, even if you were to also add the owner keyword as is done for the proc/pid/maps entry above, a snap would still be able to read the contents of this directory for other snaps running as the same user.

As such it would make more sense for this to be added to the system-observe interface I think.

[sergio-costas]

Hmm unfortunately there is no way to restrict this to just processes of the snap itself - as such, even if you were to also add the owner keyword as is done for the proc/pid/maps entry above, a snap would still be able to read the contents of this directory for other snaps running as the same user.

@alexmurray Mmm... Are you sure? I mean: we are restricting it to the PID of the running process, not to the user UID... It's exactly the same than the previous line with the "maps" file (in fact, both have the same information)...

In fact, I did a

snap run --shell firefox

and inside there I can see the contents of the "maps" file and "map_files" folder for the firefox PID, but not for the mattermost PID...

[alexmurray]

AppArmor does not (yet) support restricting to just the PID of the process via the @{pid} variable - this instead is an AppArmor regex to match all possible process-ids. So this would not be restricted by AppArmor. FWIW within a snap shell of a simple snap that only plugs network and home I can see the /proc/PID/maps file and the /proc/PID/map_files dir of the mattermost-desktop snap but I can't list the contents of this dir.

As such I still think this is not appropriate for the base template and instead should be in the system-observe

[sergio-costas]

@alexmurray Did you try to do a "cat /proc/XXXX/maps"? You can read only the file for your PID, but can't read the contents for other processes' PIDs... and it contains the same info than map_files folder, that's what I mean.

Anyway, if you insist in that it should be in system-observe, I can move it: we need it for steam, and it uses that interface too.

[sergio-costas]

@alexmurray My point is that, according to https://www.kernel.org/doc/html/latest/filesystems/proc.html , map_files was added to avoid having to parse the contents of the "maps" file, so if a process can read /proc/XXXX/maps, it should also be able to list the contents of /proc/XXXX/map_files. This is what this patch intends to do for containerized apps...

[alexmurray]

I agree that if a process can read /proc/$PID/maps then it should be able to also read the contents of the /proc/$PID/map_files/ - however what this PR is proposing is not that - the base template does not allow reading of /proc/$PID/maps - only locking of the file:

  owner @{PROC}/@{pid}/maps k,   # k means lock
  owner @{PROC}/@{pid}/map_files/ r,  # but r means read

so this is not equivalent.

And also now that I look at system-observe I see the following comment:

# allow reading of smaps_rollup, which is a summary of the memory use of a process,
# but not smaps which contains a detailed mappings breakdown like
# /proc/self/maps, which we do not allow access to for other processes

which makes me think that this is not even appropriate for system-observe - instead perhaps process-control (or in this case if this is just for the steam snap, why not put it in steam-support)?

[sergio-costas]

@alexmurray Mmm... odd... Then, why I can access /proc/XXXX/maps with the off-the-shelf snapd?

[alexmurray]

@alexmurray Mmm... odd... Then, why I can access /proc/XXXX/maps with the off-the-shelf snapd?

That is a very good question - @jrjohansen do you have any ideas? I can't see anything in the generated apparmor profile for a snap that would allow this but it is definitely possible - and when it is denied I see a ptrace denial in the journal log

[amurray:~] 130 $ hello-world.sh 
Launching a shell inside the default app confinement. Navigate to your
app-specific directories with:

  $ cd $SNAP
  $ cd $SNAP_DATA
  $ cd $SNAP_USER_DATA

bash-4.3$ cat /proc/self/maps | head -n1
00400000-0040c000 r-xp 00000000 07:1c 13                                 /bin/cat
bash-4.3$ ps
    PID TTY          TIME CMD
 338566 ?        00:00:00 bash
 450000 ?        00:00:00 sh
 450024 ?        00:00:00 bash
 450029 ?        00:00:00 ps
bash-4.3$ head -n1 /proc/450024/maps
00400000-004f4000 r-xp 00000000 07:1c 2                                  /bin/bash

• now try a process that is outside of the snap itself

bash-4.3$ head -n1 /proc/449616/maps
head: cannot open '/proc/449616/maps' for reading: Permission denied

And in the journal log we see a ptrace denial:

Feb 09 23:07:43 graphene kernel: audit: type=1400 audit(1675946263.929:1297): apparmor="DENIED" operation="ptrace" class="ptrace" profile="snap.hello-world.sh" pid=450181 comm="head" requested_mask="read" denied_mask="read" peer="unconfined"

Yet even for the original reads of /proc/self/maps etc there is nothing in the apparmor profile for the snap which would allow that from what I can see (see attached).

snap.hello-world.sh.txt

[jrjohansen]

Its because abstractions/base is granting access with the rule

@{PROC}/@{pid}/{maps,auxv,status} r,

and currently apparmor is using a place holder regex for @{pid} instead of doing an actual pid check within the pattern (which is a wip feature).

Realistically /proc/self/maps should not be allowed by abstractions base and that should require a different abstraction.

[jrjohansen]

I can add that map_files won't get added to abstraction/base and maps should be pulled in the next major release

[sergio-costas]

@jrjohansen It makes sense, yes... So, where should them be added (I can send patches)? "system-observer"? "process-control"? A new interface? (I don't think it should go in "steam-support", since it's not really for steam, but for a FPS indicator: mangohud, and it can be useful for other programs).

[alexmurray]

I can add that map_files won't get added to abstraction/base and maps should be pulled in the next major release

Hmmm I am wary of breaking stuff that was using and expecting access to maps, but on the other hand this is exactly the kind of info needed by a malicious process if it wants to read out secrets from some victim process, so I think it makes sense to disallow this for snaps going forward (I realise that this is protected in the kernel by ptrace, ie a malicious process still needs the ability to ptrace the victim process to be able to read its maps etc but I think we should be as defensive as possible.

@sergio-costas so to try and resolve your original issue - do you know why mangohud wants to be able to read this?

[alexmurray]

@jrjohansen hmm although looking at the base abstraction it has the following comment:

  # glibc's *printf protections read the maps file
  @{PROC}/@{pid}/{maps,auxv,status} r,

if this is removed from future AppArmor releases will this break these printf() protections? Should this perhaps at least be restricted by owner?

[jrjohansen]

@jrjohansen hmm although looking at the base abstraction it has the following comment:

  # glibc's *printf protections read the maps file
  @{PROC}/@{pid}/{maps,auxv,status} r,

if this is removed from future AppArmor releases will this break these printf() protections? Should this perhaps at least be restricted by owner?

@alexmurray so being removed is a gradual process, and exactly what we can get away without breaking everything is a good question. It is likely that how this is going to happen atm is the above rule is replaced with an abstraction that is effectively the same with owner on it. But the reality is we don't want this on by default for every task even if it breaks glibc's printf protections because we want to be able to express profiles that don't have it.

[sergio-costas]

@alexmurray It is used in two places of the code: one to detect vkbasalt (a Vulkan post-processing layer) and gamemode

and in other place, to check if an specific library has been loaded:

This last one is used to detect if wine3d, libtogl, libtogl_client or libvulkan are being loaded. It seems that it's to avoid some issues in some specific libraries.

[alexmurray]

Ok so thinking about this a bit more, since we already allow /proc/PID/maps in the base abstraction for AppArmor and this has been available for a long time in the base template for snaps, I think it is fine to add this to the base template for snaps using the owner conditional to try and limit the impact. Given the kernel also protects access to this with ptrace this also limits the exposure. However it would be good to add a comment just above this that explains how the base abstraction in AppArmor already allows read of /proc/pid/maps and so this is just an extension of that.

[sergio-costas]

@alexmurray Done. Please, check it.

[alexmurray]

LGTM - thanks for working through this @sergio-costas 🙏

Oops - I just realised that we also need to add access to all the files under the directory too - so this should instead be:

owner @{PROC}/@{pid}/map_files/{,*} r,

Doh - on second thought, no I don't think we do need this - please leave it as you already have it.

[sergio-costas]

Oops - I just realised that we also need to add access to all the files under the directory too - so this should instead be:

Are you sure? I mean: the info is really the directory list, not the content of the files. Just check the contents of the corresponding maps file...

I'll do a test to ensure that read access is not needed to see the destination of the soft link, but that's the only thing that I think that maybe would require read access...

(in fact, mangohud works great just with the current patch; it doesn't need read access to the files inside)

[sergio-costas]

Ops... just read now your edited comment :-D

[sergio-costas]

Confirmed: with the current patch it is possible to read the destination file for each link, which is all that is needed. No read access for the children is needed.

[alexmurray]

LGTM

[Meulengracht]

lgtm

[pedronis]

lgtm

[pedronis]

comment tweak suggestion