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

apparmor: fix and improve the service #93457

Merged
merged 2 commits into from Sep 27, 2020
Merged

apparmor: fix and improve the service #93457

merged 2 commits into from Sep 27, 2020

Conversation

ju1m
Copy link
Contributor

@ju1m ju1m commented Jul 19, 2020

This is an attempt to improve the integration of AppArmor into NixOS.
It's still a code which needs testing, but it is ready for review.

Motivation for this change

The current security.apparmor service has several problems :

  • ExecStop is missing the included paths used in ExecStart therefore most profiles using #include are not removed and when a profile changes this leaves the old version loaded into the kernel, causing potential troubles.
  • /etc/apparmor.d is not populated as expected by aa-* tools.
  • Profiles distributed by AppArmor's upstream (eg. <abstractions/base>) use the FHS (Filesystem Hierarchy Standard) but the FHS does not match paths in the Nix store.
  • Any enabling, disabling or change to an AppArmor's profile causes security.apparmor to be restarted and thus also all other services which have Requires=apparmor.service.
  • All profile are loaded in enforce mode, there is no way to select between enforce and complain confinement modes for each profile.
  • Caching of compiled profiles cannot be enabled.
Things done
  • Fix reloading of apparmor.service, and enable reloadIfChanged (see comment next to this setting for the why and how).
  • Use alias when it makes sense to use the FHS in order to reuse AppArmor's upstream profiles.
  • Remove option profiles in favor of option policies allowing to disable individual profiles, or disable their enforcing (aka. complain mode).
  • Add option includes to be able to fix or modify included profiles (like <abstractions/base>).
  • Fix profile of ping.
  • Add option enableCache to enable caching of compiled profiles (note that it works with transmission's profile but not ping's profile, I don't know why).
  • Add binary aa-teardown, and use it in ExecStartPre and ExecStop.
  • Add binary aa-remove-unknown, and use it in ExecReload.
  • Add option killUnconfinedConfinables to kill processes which are not confined but have a profile.
  • Configure apparmor_parser through /etc/apparmor/parser.conf.
  • Configure aa-logprof through /etc/apparmor/logprof.conf.
  • Adapt virtualisation.lxc, virtualisation.lxd and services.torrent.transmission.
  • Make aa-enforce and ˋaa-complainˋ work?
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@doronbehar
Copy link
Contributor

Looks like wonderful work @ju1m :) I can't judge or review because I don't use apparmor. Don't forget to add release notes once all reviews are done.

Copy link
Contributor

@bqv bqv left a comment

Choose a reason for hiding this comment

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

This works, and is a substantial improvement. I suggest merge once below is solved

nixos/modules/security/apparmor/profiles.nix Outdated Show resolved Hide resolved
${etcRule {path="python3.6"; trail="/**";}}
${etcRule {path="python3.7"; trail="/**";}}
${etcRule {path="python3.8"; trail="/**";}}
${etcRule {path="python3.9"; trail="/**";}}

This comment was marked as resolved.

Copy link
Contributor Author

@ju1m ju1m Aug 6, 2020

Choose a reason for hiding this comment

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

I would say that this profile falls under the introductive comment saying that I generally don't know the rules which are relevant, and likely that those current rules are incorrect or useless. Hence I would not add a comment to say what to do except investigate on how /etc/pythonX.Y is generated on NixOS, if it ever is. Those present rules match a configuration where /etc/pythonX.Y is a symlink, like it is for instance the case with /etc/systemd/system -> /etc/static/systemd/system, but maybe NixOS/Python people would generate it otherwise, eg. environment.etc."pythonX.Y/foo".text = "bar". Maybe we should simply remove those rules and let correct ones be added by a later PR once the /etc/pythonX.Y setup has been elucidated.

This comment was marked as resolved.

Copy link
Member

Choose a reason for hiding this comment

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

No we don't use /etc/pythonX.Y. What kind of system-wide Python configuration could one expect?

Copy link
Member

Choose a reason for hiding this comment

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

I don't know why this is needed, but at least remove all old Python versions.

This comment was marked as resolved.

nixos/modules/virtualisation/lxd.nix Outdated Show resolved Hide resolved
pkgs/os-specific/linux/apparmor/default.nix Show resolved Hide resolved
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/sandboxing-all-programs-by-default/7792/6

@ju1m ju1m force-pushed the apparmor branch 3 times, most recently from 43363d6 to ecad566 Compare August 7, 2020 03:13
@ofborg ofborg bot requested a review from joachifm August 8, 2020 06:11
@8573
Copy link
Contributor

8573 commented Aug 8, 2020

@ju1m, I see it looks like the email address with which you're committing doesn't appear to be linked to your GitHub account.

# (because AppArmor can do that without stopping the processes already confined).
# - Removing from the kernel any profile whose name is not
# one of the names within the content of the profiles in cfg.policies.
# - Killing the processes which are unconfined but now have a profile loaded
Copy link
Member

Choose a reason for hiding this comment

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

How does this work? It seems a bit scary to me to kill processes.

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems scarier to me to have processes around actively circumventing the profiles, without that being obvious to the user

Copy link
Contributor

@8573 8573 Aug 9, 2020

Choose a reason for hiding this comment

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

I suppose killing the unconfined confinable processes by default poses a certain risk of causing people who casually enable AppArmor (without looking at all of the module's options) and get their processes killed unexpectedly to forsake AppArmor with an unnecessarily poor opinion of it. As a compromise, I propose that killUnconfinedConfinables be disabled by default but the main security.apparmor.enable option's documentation recommend enabling it.

Copy link
Member

Choose a reason for hiding this comment

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

Also asking upstream: @stevebeattie @cboltz This procedure seems a bit unorthodox, what is your recommendation here?

Copy link

Choose a reason for hiding this comment

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

It's indeed a bit unorthodox ;-)

I'm not familiar with NixOS, which makes recommending something a bit difficult. Please read the following lines with a grain of salt.

My recommendation not to use aa-teardown (and therefore not unloading any profiles) in my other comment is probably the most important part, because it will avoid that processes become accidently unconfined if someone runs systemctl restart apparmor.

That will however still leave the case of newly added profiles for already running processes, where killing/restarting the process is the only way to get them confined. Killing them by default is a secure way, but I'm not sure if the users will like it ;-)

Copy link
Member

Choose a reason for hiding this comment

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

I think the biggest difference from a apparmor perspective between NixOS and other Linux distributions here is that new versions executables will get a new path in the nix store, while common Linux distributions will update executables in place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@8573 I've set killUnconfinedConfinables to true by default because it's how to get the running system to match the expectations declared in the NixOS configuration, and somehow "a secure way" as @cboltz put it. I've added warnings in the relase notes and the enable description. Also, AFAIK, when enabling AppArmor the first time, the system must be rebooted anyway to enable AppArmor in the kernel, and AFAICS the other main case that could trigger kills is in the rare situation when an AppArmor profile will be introduced/enabled for the first time into Nixpkgs.

I've also mentionned that killUnconfinedConfinables will unfortunately won't even work if we give names to the profiles in addition to their attachment paths because then /sys/kernel/security/apparmor/profiles will not expose the attachment paths and thus aa-status will not report processes from these paths as unconfined, and AFAICS it the same problem for attachment paths using regex, but because aa-status lacks support for these.

@cboltz Thanks for mentionning aa-remove-unknown, I've reinvented the wheel not knowing it existed ^^. Concerning my use of ˋaa-teardownˋ in ExecStop= I thing it's more faithful to what stopping means and is not problematic here because I'm using NixOS' reloadIfChanged= feature so that reconfiguring AppArmor shall only run ExecReload=. I'm also using ˋaa-teardownˋ in ExecStart= as a precaution because I want to be sure that only what is declared in the NixOS configuration is actually loaded into the kernel, I've had bad surprises in the previous AppArmor service with profiles lingering in the kernel and still active in addition to the profiles I wanted (in that case it happenned because apparmor_parser --remove had failed because of a missing included file, now this precise case should no longer happen but since the ExecStart= is normally only run at boot and only ExecReload= after, I prefer to be sure and remove all profiles upfront). So as I see it, a NixOS user should never have to run systemd start/stop/restart apparmor, and nixos-rebuild shall only run systemd reload apparmor.

@bqv
Copy link
Contributor

bqv commented Aug 26, 2020

Gosh, this has blown up

@ju1m
Copy link
Contributor Author

ju1m commented Sep 4, 2020

Updated to revert #96034 in favor of this PR. Also, having a parserConfig does not seem needed, since one can directly put extra config lines in environment.etc."apparmor/parser.conf".text.

@bqv
Copy link
Contributor

bqv commented Sep 27, 2020

Is there any reason this isn't being looked at or merged? It's fairly trivial again

@7c6f434c
Copy link
Member

So apparently it works somehow for @bqv and @joachifm has approved some intermediate version… I guess we are not getting any better reports here…

@7c6f434c 7c6f434c merged commit 31a4e2e into NixOS:master Sep 27, 2020
@vcunat
Copy link
Member

vcunat commented Oct 7, 2020

This was reverted in 420f89c due to very hard evaluation problems (see cross-linked info). I expect it's a task for someone of you to prepare a fixed version of this and file a new PR.

@ju1m
Copy link
Contributor Author

ju1m commented Oct 7, 2020

@vcunat sorry for the troubles, I'm trying to understand what's going wrong.

#99236 (comment) reports the following error related to AppArmor, which I can't make sense of:

a 'aarch64-linux' with features {} is required to build '/nix/store/dhwnmrcdsz42d7sb9j2hb19z4j66wnql-apparmor-parser-2.13.4.drv', but I am a 'x86_64-linux' with features {benchmark, big-parallel, kvm, nixos-test}"

But #99236 (comment) reports an other error, which makes more sense to me:

attempted to realize '/nix/store/4shyj8ckj7f6lxy5jvk0d9spx99dhgsi-apparmor-utils-2.13.4.drv' during evaluation but 'allow-import-from-derivation' is false"

So the error appears to be located into the apparmor-parser derivation, which is changed by this PR only to fix paths in upstream's parser/rc.apparmor.functions:
https://github.com/ju1m/nixpkgs/blob/fb6d63f3fdd95a5468d43a0693c8ca7c1894363f/pkgs/os-specific/linux/apparmor/default.nix#L222-L224

I'm not aware of this PR requiring allow-import-from-derivation, maybe someone would see something obviously wrong in the way I patch apparmor-parser? Like using $out or ${./fix-rc.apparmor.functions.sh}?

I'm trying to learn to use hydra to reproduce the problem myself in order to bisect it further down using:

nix-shell -p hydra-unstable --run 'NIX_PATH= hydra-eval-jobs -I . nixos/release-combined.nix --option allow-import-from-derivation false --verbose --show-trace'

I've never done it, I don't have a clear view of what it's trying to do, and I'm expecting many hours of buildtime considering the low power of my computer, or more if I have to restart it for whatever reason.

@vcunat
Copy link
Member

vcunat commented Oct 7, 2020

At a quick glance, usage of apparmorRulesFromClosure looks like import-from-derivation.

@vcunat
Copy link
Member

vcunat commented Oct 7, 2020

To clarify, import-from-derivation is disallowed on Hydra.nixos.org but normally that should just kill only the jobs requiring it and not the whole evaluation (I see at least one report for another job in successful eval). So, the biggest problem in this PR might be something else than IFD.

@bqv
Copy link
Contributor

bqv commented Oct 7, 2020

@vcunat is that to say you're not actually sure what part of this breaks hydra so comprehensively, but you're sure some part of it does? Because I can't see anything overly complicated here

@samueldr

This comment has been minimized.

@samueldr
Copy link
Member

samueldr commented Oct 8, 2020

strace doesn't seem to point to anything particular here. Sorry for the false hope in my now hidden previous comment.

@vcunat
Copy link
Member

vcunat commented Oct 8, 2020

Correct, I do not know (for sure at least). Still, import-from-derivation is better avoided in the official nixpkgs repo (not sure if there's some rule on that) and it's intentionally disabled on Hydra.nixos.org.

@ju1m
Copy link
Contributor Author

ju1m commented Oct 8, 2020

When trying to reproduce on fb6d63f, I get:

$ nix-shell -p hydra-unstable --run 'NIX_PATH= time hydra-eval-jobs -I . nixos/release-combined.nix --option allow-import-from-derivation false --verbose'
[…]
[json.exception.type_error.302] type must be string, but is null

And when removing the call to closureInfo I get:

$ nix-shell -p hydra-unstable --run 'NIX_PATH= time hydra-eval-jobs -I . nixos/release-combined.nix --option allow-import-from-derivation false --verbose'
[…]
error: --- InvalidPath ------------------------------- hydra-eval-jobs
path '/nix/store/vycirqyzd6vf8v4g1bhp4jbx99wn0y6k-nixos-channel-20.09pre56789.gfedcba.drv' is not a valid store path
Command exited with non-zero status 1
19.19user 36.82system 4:13:52elapsed 0%CPU (0avgtext+0avgdata 91472maxresident)k
1186inputs+0outputs (251major+53123minor)pagefaults 0swaps

No clue of what I can do to get more feedback on what's going wrong.

@ju1m
Copy link
Contributor Author

ju1m commented Oct 8, 2020

Correct, I do not know (for sure at least). Still, import-from-derivation is better avoided in the official nixpkgs repo (not sure if there's some rule on that) and it's intentionally disabled on Hydra.nixos.org.

@vcunat, using closureInfo is an important helper to write and maintain AppArmor profiles, I don't understand why it should not be used in the official Nixpkgs?

AFAICS closureInfo is already used, but only in NixOS:

closureInfo = pkgs.closureInfo { inherit rootPaths; };

But in the AppArmor case it makes more sense to call closureInfo when building the packages (hence in Nixpkgs) because:

  1. this is where dependencies are selected (using build parameters and such) and easily filterable at the program level instead of the package level. For instance, dependencies of ping are not necessarily the same than tftpd's, tough they share the same iputils package. So, if one would do like systemd-confinement.nix and derive rules restraining ping based upon its iputils package, one would get a profile more permissive than necessary. In that case it may not be an important security concern, but in other packages it may.
  2. AppArmor is path-based, hence applies to program that may me called manually (eg. ping) not just to systemd services.

@vcunat
Copy link
Member

vcunat commented Oct 8, 2020

I don't think closureInfo should be a problem by itself. It seems likely that on some place it's (unintentionally? transitively) required during evaluation already, but I haven't really looked.

EDIT: I suspect you can test with --option allow-import-from-derivation false even without cannons like hydra-eval-jobs.

@ju1m ju1m mentioned this pull request Oct 19, 2020
18 tasks
@Mic92 Mic92 mentioned this pull request Dec 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet