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

Clamav privatetmp #33228

Closed
wants to merge 2 commits into from
Closed

Clamav privatetmp #33228

wants to merge 2 commits into from

Conversation

Ekleog
Copy link
Member

@Ekleog Ekleog commented Dec 31, 2017

Motivation for this change

Allow to disable clamav running with a private /tmp. Turning this on fixes clamdscan not working for files in /tmp. Not turning PrivateTmp off altogether as I'd guess it has been added here for some reason.

I also replaced some mkIf x [y] with optional x y while I was at it.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@joachifm
Copy link
Contributor

Strictly speaking, you can already configure this easily via systemd.services.*.serviceConfig.PrivateTmp.

clamd will naturally be a juicy target, it seems more prudent to never let it see any of the fs, only consuming bytes over a pre-opened file descriptor & sending judgements about those bytes to another fd, or some other configuration where it runs completely "blind" (probably should run on a dedicated machine, even).

@joachifm joachifm requested a review from fpletz December 31, 2017 15:02
@joachifm
Copy link
Contributor

If the motivation is only to share files/whatnot with another daemon, you could try making them share namespace(s) or something. That seems stronger than exposing the system /tmp.

@Ekleog
Copy link
Member Author

Ekleog commented Dec 31, 2017

I'm assuming using systemd.services.*.serviceConfig.PrivateTmp is a bad idea, as it makes the configuration depend on internals of modules.

As for clamd being a juicy target, I completely agree with you. Had there been an actually strong protection of the filesystem, I wouldn't even have considered this. But protecting only /tmp means a whole lot of the filesystem is exposed, so I'm a bit doubtful this has a real use.

Then, your comment made me notice the JoinsNamespaceOf parameter, so I guess I can get clamsmtp work otherwise. That said, eg. clamdscan /tmp/foo won't work, which is surprising to beginners (TBH, it took me an hour with help from #clamav to understand why clamscan worked but not clamdscan, not counting the time I spent debugging before this: /tmp is a logical place to put test files), so I'm leaving this open so that if it's merged at least an option will be put forward in the documentation and people have a chance to more easily get what's going on. :)

@Ekleog
Copy link
Member Author

Ekleog commented Dec 31, 2017

Hmm, actually I just tried adding 7a711db to the daemon that runs clamdscan and run clamav with PrivateTmp enabled, and I'm hitting the same issue of clamsmtp not being able to pass files through clamdscan via /tmp, even after having manually restarted both services.

Did I miss something?

@joachifm
Copy link
Contributor

But protecting only /tmp means a whole lot of the filesystem is exposed, so I'm a bit doubtful this has a real use.

My comment was more to suggest going in the opposite direction of this PR, i.e. add more not fewer restrictions, I didn't intend to say that we should not merge this in the interim, if there's a need for it.

@Ekleog
Copy link
Member Author

Ekleog commented Dec 31, 2017

In this case I completely agree with you, but maybe patch clamdsmtp so that people know it has a rather peculiar way of working due to the hardening in place?

Actually, I guess that's about the same as most hardening, it's a trade-off between usability and security. I personally deem linux's DAC enough for security and believe that *ns bring in more risks than benefits, but that's just my opinion :)

@joachifm
Copy link
Contributor

believe that *ns bring in more risks than benefits

I think history proves that ns will never be enough and sometimes can be a liability. A skilled & motivated attacker will "simply" target the kernel if they have to, after all. Perhaps it is a mistake to rely on ns for anything beyond deployment convenience, but lots of people seem to think you can enhance security this way. I'm not qualified to say either way but am hopeful that it helps more than it hurts overall :)

@joachifm
Copy link
Contributor

joachifm commented Jan 6, 2018

I've integrated 4314be4 but closing this for now

@joachifm joachifm closed this Jan 6, 2018
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

4 participants