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

Changeable seccomp error action #3301

Conversation

topimiettinen
Copy link
Collaborator

@topimiettinen topimiettinen commented Mar 27, 2020

Let user specify the action when seccomp filters trigger:

  • 'kill' (default): kill the process as before
  • errno name like EPERM/ENOSYS: return errno and let the process continue.

Not killing the process weakens Firejail slightly when trying to
contain intrusion, but it may also allow tighter filters if the
alternative is to always allow a system call. It's still possible to
use errno return per syscall.

@topimiettinen
Copy link
Collaborator Author

I based this on 32-bit filter commit to avoid rebasing.

@Vincent43
Copy link
Collaborator

Why keep kill as default? I think we want default to work with as many apps as possible otherwise cases like mesa depending on new blocked syscall will keep breaking apps.

@topimiettinen
Copy link
Collaborator Author

For compatibility, then each app profile can opt-in to enable it or keep the default if the profile just works.

@topimiettinen
Copy link
Collaborator Author

There should be a way to override this per syscall, like --seccomp-error-action=EPERM --seccomp.drop=kcmp:kill.

@Vincent43
Copy link
Collaborator

How moving to EPERM/ENOSYS can decrease compatibility? I think it can only increase it. The point is not having to whack a mole with fixing particular profiles one by one which often takes weeks or months until fixes land in distros. Apps and their ecosystem constantly change and we don't control what syscall get called. Default kill results in profiles that just worked yesterday are broken tomorrow. Kill should be user explicit override, not default.

@topimiettinen
Copy link
Collaborator Author

With compatibility, I mean that the behaviour does not change until each profile is changed. Right now, all profiles should work with no change. Someone who cares for a specific app should check if the new option makes sense (probably does), enable it but preferably also check if the filters can be tightened since the app may respond intelligently to getting ENOSYS/EPERM from system call filters which previously were not there because of kill action. I don't like the idea of enabling this globally for all apps, then in the worst case nobody would make the checks for tightening the filters.

@Vincent43
Copy link
Collaborator

Vincent43 commented Mar 28, 2020

Right now many profiles are broken and change is needed ASAP, ideally once for all with this PR. People are disabling seccomp completely because broken security is worse than no security.

I don't like the idea of enabling this globally for all apps, then in the worst case nobody would make the checks for tightening the filters.

With default kill nobody would tightening the filters neither because it's impossible to do. So in worst case you end up with same security (but working apps), in best case security will improve as filters will be tightened. ENOSYS/EPERM is really no brainier here, everyone else learned that already and it's time for firejail to fix itself too.

@topimiettinen
Copy link
Collaborator Author

How would it be impossible to tighten the filters? Just add `seccomp-error-action EPERM´ to the profile.

@Vincent43
Copy link
Collaborator

Yes, it would be possible this way, yet it needs additional work about which you said nobody will do. I don't understand why we can't fix everything here.

@topimiettinen topimiettinen force-pushed the changeable-seccomp-error-action branch from 8d5b034 to f3bb0c8 Compare March 28, 2020 15:07
@topimiettinen
Copy link
Collaborator Author

Updated with syscall:kill syntax to override the default.

We don't know how much there's willingness do the changes and the checks. Perhaps other developers should also comment which would be their preference?

@glitsj16
Copy link
Collaborator

glitsj16 commented Mar 28, 2020

I don't consider myself a developer technically, my programming skills are way to limited for that alas. As a contributor I probably have above-average time to spend on firejail (profile maintenance) due to all kinds of personal circumstances. So I for one would be willing to make an extra effort to keep our profiles in good working condition, if that's what it takes. Please note that I don't fully understand all the implications of what this PR is about, but I wanted to chime in to express a growing concern that firejail profiles in general are getting harder to maintain.

The technically very sound and much needed MR about fine-grained D-Bus control is another case in point in this context. Not that I don't appreciate all the work being done here, quite the opposite. Profile complexity will keep rising, there's no way around that. But it wouldn't hurt to have a discussion on what we as a community can do to keep it manageable.

Bottomline: I tend to prefer implementations that don't touch (individual) profiles, or just as little as possible otherwise.

@topimiettinen topimiettinen force-pushed the changeable-seccomp-error-action branch from f3bb0c8 to d6da859 Compare March 29, 2020 18:24
@topimiettinen
Copy link
Collaborator Author

All right, I changed the default action to EPERM.

@topimiettinen
Copy link
Collaborator Author

Some thoughts about the filters. Let's say we have three sets of system calls for an application X:

  • A: always need to be allowed
  • B: not really needed, but the app or the libraries can try to use the system call
  • C: never needed

Previously the kill error action has forced the filters to allow both A and B because denying B would kill the application. With default seccomp-error-action of EPERM it's possible to deny B.

This could be improved so that while set B returns EPERM as the new default, it's easy to use kill method for set C (or parts of it like the most suspicious system calls). The syntax could be something like seccomp.drop=@obsolete:kill.

@FOSSONLY
Copy link

It should not be possible to change this, and certainly not reduce security globally. And it's not the profiles that are broken, but the programs that are the problem if they go against security. Finally, "kill (process tree)" is the Linux standard for Seccomp, and for very good reasons. Compatibility is also clearly secondary to security. Not the security mechanisms have to become more compatible, but the programs have to obey the rules. It can't make sense to give in all the time, just so that certain programs that don't like to be restricted work properly.

@Vincent43
Copy link
Collaborator

It should not be possible to change this, and certainly not reduce security globally

This change will only increase security globally as more syscalls can be blocked now. Default seccomp policy could be also changed from blacklist to whitelist as new syscalls will be gracefully denied but this needs some more work.

And it's not the profiles that are broken, but the programs that are the problem if they go against security

Firejail can't fix any programs, it can only refuse to support it however if program profile exist in firejail repo then it should be supported.

Finally, "kill (process tree)" is the Linux standard for Seccomp

It's not. Neither of snap, flatpak, systemd, or docker does it. Here's comment from Lennart Poettering which among other things admits that killing process with seccomp was a mistake.

Compatibility is also clearly secondary to security.

Yes, broken apps are secure. Also useless 😄 .

Not the security mechanisms have to become more compatible, but the programs have to obey the rules.

How would you force them to obey the rules? Magic? Programs do what was written in their code.

It can't make sense to give in all the time, just so that certain programs that don't like to be restricted work properly.

When app crash on startup because it gets killed by seccomp policy then what you would do? Stick to kill and allow problematic syscall or keep blocking that syscall and use EPERM? What do you think is more secure?

@matu3ba
Copy link
Contributor

matu3ba commented Mar 30, 2020

Default seccomp policy could be also changed from blacklist to whitelist as new syscalls will be gracefully denied but this needs some more work.

Could you explain a possible strategy on changing to whitelists? Removing code to simplify is always a good thing, but the problem is the compatibility (which needs alot testing under different distros).
To me it looks like some way to to keep 2 parallel installations to test the distro update by users would be needed. Then the program maintainers or firejail devs could be notified to fix the problem.
Do you know any simple enough tools for that with large enough compatibility?

Firejail can't fix any programs, it can only refuse to support it however if program profile exist in firejail repo then it should be supported.

You definition of support is vaguely. Maybe the definition of support regarding security should be specified somewhere like in the README. If you say that is the problem, then keeping track of severity would be the first step. Do you have numbers/a list? Do they use common functionality?

Finally, "kill (process tree)" is the Linux standard for Seccomp

It's not. Neither of snap, flatpak, systemd, or docker does it. Here's comment from Lennart Poettering which among other things admits that killing process with seccomp was a mistake.

True, but the questions is about the project goal(intention) and default behavior and how simple/complex the solution would be.

Compatibility is also clearly secondary to security.

Yes, broken apps are secure. Also useless smile .

It depends on what the project goal is and if you want to take that compromise. However this should be specified.

When app crash on startup because it gets killed by seccomp policy then what you would do? Stick to kill and allow problematic syscall or keep blocking that syscall and use EPERM? What do you think is more secure?

Please be constructive and suggest an alternative behavior with its complexity/simplicity tradeoffs from a coding standpoint.

Sorry for being picky your comment in special, but probably you could at best weight the tradeoffs of the argument against another at best.

@Vincent43
Copy link
Collaborator

Vincent43 commented Mar 30, 2020

Could you explain a possible strategy on changing to whitelists?

Take a current blacklist and reverse it - instead of listing blocked syscalls, list allowed syscalls which are ones not in blacklist. The advantage of this is that any new syscalls added won't be allowed automatically. This is what docker does.

You definition of support is vaguely. Maybe the definition of support regarding security should be specified somewhere like in the README. If you say that is the problem, then keeping track of severity would be the first step. Do you have numbers/a list? Do they use common functionality?

I think it's safe to assume that if firejail ships profile for specific app then that app should work and not be killed by seccomp policy. There are a lot of issues reported regarding seccomp being too aggressive, latest one is kcmp syscall used by recent mesa which affects many apps.

True, but the questions is about the project goal(intention) and default behavior and how simple/complex the solution would be.

Firejail seccomp policy has same goal as all examples I mentioned - enhance security by blocking potentially dangerous syscalls. I believe we don't want to argue here if making app unworkable enhances its security. The complexity doesn't change.

Please be constructive and suggest an alternative behavior with its complexity/simplicity tradeoffs from a coding standpoint.

I literally provided two practical alternatives: one when seccomp violation kills offending processes (current behavior) and one when seccomp policy just blocks specific syscall (what this PR does) which user can switch back.

@topimiettinen
Copy link
Collaborator Author

If someone does not like the new default, it's still possible to get the old behaviour with --seccomp-error-action=kill. This can also be done per profile basis or locally. Though when the filters get tightened so that previously allowed system calls return EPERM, this will not be so simple, also the filters need to be tuned so the processes are not killed.

Maybe some filters could be annotated with tags or something that specify whether it's OK to use either kill or error, or error/allow only (syscall:?ERROR_ALLOW_NOKILL:EPERM or something crazy like that). That would not help the goal to keep the job of profile maintainers relatively sane.

@Vincent43
Copy link
Collaborator

@topimiettinen could you rebase this to resolve conflicts?

@netblue30 @Fred-Barclay @smitsohu @reinerh could any (or all) of you review this if there are any objections?

src/firejail/usage.c Outdated Show resolved Hide resolved
@reinerh
Copy link
Collaborator

reinerh commented Apr 4, 2020

I don't have objections. I think it could help with maintainability of profiles.
But I would have an additional request.
Could you make the default action also configurable via /etc/firejail/firejail.config?
This would make it much easier for users to switch back to the old behaviour, if they prefer to kill the processes instead.

(It would also be nice to have tests for the new behavior. And I would also expect that some older tests are now failing?)

@rusty-snake
Copy link
Collaborator

Could you make the default action also configurable via /etc/firejail/firejail.config?
This would make it much easier for users to switch back to the old behaviour, if they prefer to kill the processes instead.

IMHO globals.local with exceptions in .local would be easier. However a seccomp-force-kill yes (like force-nonewprivs yes) could be interesting.

(It would also be nice to have tests for the new behavior. And I would also expect that some older tests are now failing?)

firejail --audit

@Fred-Barclay
Copy link
Collaborator

I like it! I'm with @reinerh, it would be great to configure with /etc/firejail/firejail.config (I slightly prefer this to globals.inc but either are good).

But this is needed for sure. 👍

@topimiettinen topimiettinen force-pushed the changeable-seccomp-error-action branch from d6da859 to d761c6e Compare April 4, 2020 19:59
@topimiettinen
Copy link
Collaborator Author

Added also /etc/firejail/firejail.config option.

Let user specify the action when seccomp filters trigger:
- errno name like EPERM (default) or ENOSYS: return errno and let the process continue.
- 'kill': kill the process as previous versions

The default action is EPERM, but killing can still be specified with
syscall:kill syntax or globally with seccomp-error-action=kill. The
action can be also overridden /etc/firejail/firejail.config file.

Not killing the process weakens Firejail slightly when trying to
contain intrusion, but it may also allow tighter filters if the
only alternative is to allow a system call.
@topimiettinen topimiettinen force-pushed the changeable-seccomp-error-action branch from d761c6e to 48112d8 Compare April 6, 2020 16:29
@topimiettinen topimiettinen merged commit 3f27e84 into netblue30:master Apr 6, 2020
@topimiettinen topimiettinen deleted the changeable-seccomp-error-action branch April 6, 2020 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants