[PACKAGING] Provides a dedicated AppArmor profile

HorlogeSkynet commented 2 years ago

:warning: TODO :

[x] dh_apparmor for Debian (see here) ?
[x] /etc/apparmor.d/usr.bin.archey4 marked as a configuration file ?
[ ] What about profile reload on Arch Linux ? (EDIT : I'm currently waiting for Eli Schwartz's inputs on this)
[X] dig opens UDP sockets (under its own profile) : remove network related permissions
[x] Warning from /etc/apparmor.d (/etc/apparmor.d/usr.bin.archey4 line 7): /sbin/apparmor_parser: Profile abi not supported, falling back to system abi.

Description

This patch provides a first AppArmor profile candidate to be included in Archey GNU/Linux distribution packages.

Reason and / or context

See https://apparmor.net/.

How has this been tested ?

Debian 11.

Types of changes :

Bug fix (non-breaking change which fixes an issue)
Typo / style fix (non-breaking change which improves readability)
[X] New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist :

[IF NEEDED] I have updated the README.md file accordingly ;
[IF NEEDED] I have updated the test cases (which pass) accordingly ;
[IF BREAKING] This pull request targets next Archey version branch ;
[X] My changes looks good ;
[X] I agree that my code may be modified in the future ;
My code follows the code style of this project (PEP8).

roddhjav commented 1 year ago

Hi, only seeing your mastodon message now.

Here are my comments on your profile:

As it is a single profile for your project, do not use variable in profile definition. That will break most aa-* tools. (Yes, I do it in apparmor.d... do what I say, not what I do...)
Some program should be confined within the archey4 profile. So with rix, not with PUx: ls, free, uptime, getent
/{,usr/}sbin -> /{,usr/}{s,}bin/
If dig is in PUx mode. You should not need network inet stream (unless you get a no-new-priv error)

Regarding the abi/3.0. This is not supported in Debian, but you can safely remove it on Debian. It is mostly here for future proofing as with they will be a abi/4.0 with AppArmor 4.x that may add features.

HorlogeSkynet commented 1 year ago

Hey @ingrinder ! Could you take a look to this PR ? Even if you don't actually have AppArmor on your system, I'd love hearing one more comment on the restrictions and/or the packaging.

Bye, thanks again :wave:

ingrinder commented 1 year ago

I'm a touch busy at the moment, but I can certainly take a proper look in a few weeks (unless you want to push this soon?) Just let me know.

HorlogeSkynet commented 1 year ago

It's okay to postpone this a bit, we are ready for a new release 😉

HorlogeSkynet commented 8 months ago

Hi @HorlogeSkynet!

Finally I have taken a look over this PR, for now just manually reading through the code. I have left a few inline comments, some other thoughts:
* We need to add `Distributions` file access permissions - e.g. the `/etc/*release` files (and maybe anything `distro` accesses?)

* Also need to add the screenshot tools in `Screenshot`. Is there an established way in AppArmor to get permission to write to a file to somewhere sensible, e.g. a screenshots folder in the home directory (or at least create one in cwd?) or does it have to be hard-coded like `@{HOME}/specific/path`?

* We should probably add a `README.md` notice for the `Custom` entries for anyone running AA instructing them to make an additional profile for their command.
I'll test it out for real soon on my system - when I get around to finally rebooting 😜

See you!

Thanks for this complete review Michael !

You must be right (I don't know how it works without these on my machine, then :thinking:)
Yes, you're absolutely right, that's a real omission. I've added some !
Indeed, added too !

I've not tested everything (yet), keep me posted, bye :pray:

HorlogeSkynet commented 8 months ago

It's me again !

So about the remaining points :

It appears AppArmor dereferences symbolic links when they are accessed, so specifying symlink executable paths do not work (for instance on my machine /usr/bin/import resolves to /usr/bin/import-im6.q16, thus /{,usr/}bin/import PUx does not allow its execution). Also see upstream issue ;
I've removed the rule to allow "write access to CWD" for screenshot as spawned processes run under their own profile, or unconfined (so CWD access would be allowed) ;
I've learnt that AppArmor is not installed by default on Arch Linux, hence the "missing" packaging hacks to deal with profile reload. I'll address this directly and manually in AUR package sources.

Bye, many thanks :wave:

HorlogeSkynet commented 5 months ago

Hey @ingrinder, do you think we could/should include this in the next release ? 🙂

ingrinder commented 5 months ago

@HorlogeSkynet sure! I think there might be a couple of additions to make first.

Here's what I noticed from aa-logprof now I have AA set up on my system:

/usr/bin read access -- unsure where this one is coming from, but I haven't noticed anything broken without it in enforce mode...
Networking stuff -- I think this is because I have the WAN_IP DNS config option turned off, so read access to these are required for urllib:
- /etc/ssl/openssl.conf and/or /etc/ssl/openssl.cnf (probably use abstractions/openssl)
- /etc/nsswitch.conf
- /etc/host.conf
- /run/systemd/resolve/stub-resolv.conf (from the symlink at /etc/resolv.conf).
- /etc/hosts

Did you find a solution to the symlinks e.g. with import on your system? I'm not sure how you could account for them aside from adding all the known locations manually!

HorlogeSkynet commented 5 months ago

@HorlogeSkynet sure! I think there might be a couple of additions to make first.

Thanks again for this feedback !

Here's what I noticed from aa-logprof now I have AA set up on my system:
1. `/usr/bin` read access -- unsure where this one is coming from, but I haven't noticed anything broken without it in enforce mode...

Indeed, maybe an oversight of the tool itself ?

2. Networking stuff -- I think this is because I have the WAN_IP DNS config option turned off, so read access to these are required for `urllib`:

   * `/etc/ssl/openssl.conf` and/or `/etc/ssl/openssl.cnf` (probably use `abstractions/openssl`)
   * `/etc/nsswitch.conf`
   * `/etc/host.conf`
   * `/run/systemd/resolve/stub-resolv.conf` (from the symlink at `/etc/resolv.conf`).
   * `/etc/hosts`

You're absolutely right, 'just (rebased and) added openssl and nameservices abstractions.

Did you find a solution to the symlinks e.g. with import on your system? I'm not sure how you could account for them aside from adding all the known locations manually!

None ! Upstream issue is still opened, and it doesn't appear there is an easy way to deal with it. A "dirty" way though would be to prepare an IMPORT_SYMLINK placeholder and dynamically resolve it during install (in after_install for instance)... Maybe a cleaner approach would be to specify a wildcard for "known" import symlink targets :upside_down_face:

Bye :wave:

ingrinder commented 5 months ago

@HorlogeSkynet

You're absolutely right, 'just (rebased and) added openssl and nameservices abstractions.

Is it called nameservices on Debian? Here on Arch it appears to be just nameservice :-)

Other than that those changes seem to make it work great for me!

HorlogeSkynet commented 5 months ago

Is it called nameservices on Debian? Here on Arch it appears to be just nameservice :-)

Nice catch, sorry for the typo.

I've just force-pushed again to fix this issue and opted for a relatively simple wildcard for import (regular and high resolution) targets.

Bye, thanks again :wave:

HorlogeSkynet commented 5 months ago

You were right about /usr/bin/ opened for reading ! I wasn't able to completely figure it out but it looks related to Python internals. I've added the corresponding permission to profile and edited changelog ; Merging here ! Thanks again :wave:

HorlogeSkynet / archey4