Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PACKAGING] Provides a dedicated AppArmor profile #119

Merged
merged 1 commit into from
Apr 13, 2024

Conversation

HorlogeSkynet
Copy link
Owner

@HorlogeSkynet HorlogeSkynet commented Sep 23, 2022

⚠️ TODO :

  • dh_apparmor for Debian (see here) ?
  • /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)
  • dig opens UDP sockets (under its own profile) : remove network related permissions
  • 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)
  • 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 ;
  • My changes looks good ;
  • I agree that my code may be modified in the future ;
  • My code follows the code style of this project (PEP8).

@HorlogeSkynet HorlogeSkynet added the enhancement ⬆️ Implements a new feature, fixes or improves existing ones label Sep 23, 2022
@HorlogeSkynet HorlogeSkynet self-assigned this Sep 23, 2022
@HorlogeSkynet HorlogeSkynet force-pushed the packaging/apparmor_profile branch from 92720cf to 77775c0 Compare September 23, 2022 17:58
@HorlogeSkynet HorlogeSkynet marked this pull request as ready for review September 23, 2022 19:10
@HorlogeSkynet HorlogeSkynet force-pushed the packaging/apparmor_profile branch from 77775c0 to 90d62ac Compare September 25, 2022 13:08
@HorlogeSkynet HorlogeSkynet changed the title [PACKAGING] Provides a dedicated AppArmor profile [WIP] [PACKAGING] Provides a dedicated AppArmor profile Oct 8, 2022
@HorlogeSkynet HorlogeSkynet force-pushed the packaging/apparmor_profile branch from 90d62ac to ce010b8 Compare October 8, 2022 10:31
@HorlogeSkynet HorlogeSkynet marked this pull request as draft October 8, 2022 10:33
@HorlogeSkynet HorlogeSkynet force-pushed the packaging/apparmor_profile branch from ce010b8 to 42940ab Compare October 27, 2022 13:52
@roddhjav
Copy link
Contributor

roddhjav commented Nov 2, 2022

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 HorlogeSkynet force-pushed the packaging/apparmor_profile branch from 42940ab to 6746780 Compare November 5, 2022 10:44
@HorlogeSkynet HorlogeSkynet added this to the v4.14.2.0 milestone Jun 18, 2023
@HorlogeSkynet HorlogeSkynet force-pushed the packaging/apparmor_profile branch 3 times, most recently from b9eabb9 to 0c1a9a8 Compare July 26, 2023 17:40
@HorlogeSkynet HorlogeSkynet marked this pull request as ready for review July 26, 2023 17:41
@HorlogeSkynet
Copy link
Owner Author

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 👋

@HorlogeSkynet HorlogeSkynet force-pushed the packaging/apparmor_profile branch from 0c1a9a8 to fa81111 Compare July 26, 2023 17:51
@ingrinder
Copy link
Collaborator

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
Copy link
Owner Author

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

@HorlogeSkynet HorlogeSkynet changed the title [WIP] [PACKAGING] Provides a dedicated AppArmor profile [PACKAGING] Provides a dedicated AppArmor profile Aug 4, 2023
@HorlogeSkynet HorlogeSkynet added the security 🔒 A security matter is being treated label Aug 4, 2023
@HorlogeSkynet HorlogeSkynet removed this from the v4.14.2.0 milestone Aug 26, 2023
Copy link
Collaborator

@ingrinder ingrinder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

apparmor.profile Show resolved Hide resolved
apparmor.profile Show resolved Hide resolved
apparmor.profile Show resolved Hide resolved
apparmor.profile Show resolved Hide resolved
apparmor.profile Show resolved Hide resolved
apparmor.profile Outdated Show resolved Hide resolved
@HorlogeSkynet HorlogeSkynet force-pushed the packaging/apparmor_profile branch from fa81111 to e0075d1 Compare January 8, 2024 20:31
@HorlogeSkynet
Copy link
Owner Author

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 !

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

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

@HorlogeSkynet HorlogeSkynet added this to the v4.14.3.0 milestone Jan 8, 2024
@HorlogeSkynet HorlogeSkynet force-pushed the packaging/apparmor_profile branch from e0075d1 to 55c2c20 Compare January 24, 2024 18:23
@HorlogeSkynet
Copy link
Owner Author

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 👋

@HorlogeSkynet
Copy link
Owner Author

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

@HorlogeSkynet HorlogeSkynet modified the milestones: v4.14.3.0, v4.14.4.0 Apr 5, 2024
@ingrinder
Copy link
Collaborator

@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:

  1. /usr/bin read access -- unsure where this one is coming from, but I haven't noticed anything broken without it in enforce mode...
  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

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 HorlogeSkynet force-pushed the packaging/apparmor_profile branch from 55c2c20 to ae3142d Compare April 6, 2024 07:43
@HorlogeSkynet
Copy link
Owner Author

@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 🙃


Bye 👋

@HorlogeSkynet HorlogeSkynet force-pushed the packaging/apparmor_profile branch from ae3142d to ad45ce2 Compare April 6, 2024 07:46
@ingrinder
Copy link
Collaborator

@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 HorlogeSkynet force-pushed the packaging/apparmor_profile branch from ad45ce2 to 0a389b4 Compare April 12, 2024 21:12
@HorlogeSkynet
Copy link
Owner Author

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 👋

Copy link
Collaborator

@ingrinder ingrinder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then, this LGTM now, as long as it's working on your end! 🚀

@HorlogeSkynet HorlogeSkynet force-pushed the packaging/apparmor_profile branch 2 times, most recently from a845a12 to 7e0d0cd Compare April 13, 2024 17:17
@HorlogeSkynet
Copy link
Owner Author

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 👋

Co-Authored-By: Alexandre Pujol <alexandre@pujol.io>
Co-Authored-By: Michael Bromilow <developer@bromilow.uk>
@HorlogeSkynet HorlogeSkynet force-pushed the packaging/apparmor_profile branch from 7e0d0cd to 25854d7 Compare April 13, 2024 17:20
@HorlogeSkynet HorlogeSkynet merged commit d4a6b16 into master Apr 13, 2024
26 checks passed
@HorlogeSkynet HorlogeSkynet deleted the packaging/apparmor_profile branch April 13, 2024 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement ⬆️ Implements a new feature, fixes or improves existing ones security 🔒 A security matter is being treated
Projects
Development

Successfully merging this pull request may close these issues.

3 participants