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

CVE-2019-5736 #2401

Closed
SkewedZeppelin opened this issue Feb 13, 2019 · 13 comments
Closed

CVE-2019-5736 #2401

SkewedZeppelin opened this issue Feb 13, 2019 · 13 comments
Labels
bug Something isn't working information_old (Deprecated; use "doc-todo" or "needinfo" instead) Information was/is required security Security issue

Comments

@SkewedZeppelin
Copy link
Collaborator

Is firejail vulnerable to this? It seems to be a common implementation issue when using privileged namespaces.

runc, docker, k8s, lxc, and flatpak have all had patches issued for it.

See

@smitsohu
Copy link
Collaborator

smitsohu commented Feb 13, 2019

This blog post has some additional details and seems to indicate that Firejail is not vulnerable, the key part being

... the kernel does not allow modifications to running binaries

Firejail has everything in one binary and in general outlives the sandboxed process. In other words, Firejail's situation is more similar to LXC than to runc, with the added advantage that we don't have an equivalent to the lxc-attach binary - it's the same Firejail binary all over, including for --join.

Only killing a master process with a signal other than SIGTERM or SIGINT (e.g. pkill -9 firejail) might potentially invalidate this assumption, but this is just a guess, and it is impossible to do from inside the sandbox due to the pid namespace.

In any case --noroot is expected to defend against this.

@smitsohu
Copy link
Collaborator

After playing with publicly available exploit code, I can confirm there is a problem if following conditions are met:

  1. The sandbox must be running exploit code.
  2. The sandbox must be running as root.
  3. The sandbox parent is killed instantly by an unhandled signal, i.e. something different from SIGTERM (kill <pid>) or SIGINT (ctrl+c). This cannot be done from inside the sandbox (because of the pid namespace), and also it cannot be done from the outside without root privileges. As only root him/herself is able to kill the sandbox in this way, this kind of attack is not relevant with regards to Firejail's SUID property.

Because this is a sandbox escape, the obligatory read-only mount and private-bin are no mitigations. I couldn't win the race for writing the payload, but it was possible to truncate the Firejail binary to length zero.

So for the time being, if your sandboxes are root and you need to hard kill a sandbox (kill -9 <pid>), preferably send the signal to the second firejail process rather than the first one (check firejail --tree).

@netblue30 ?

@glitsj16
Copy link
Collaborator

So for the time being, if your sandboxes are root and you need to hard kill a sandbox (kill -9 ), preferably send the signal to the second firejail process rather than the first one (check firejail --tree).

@smitsohu Is this what happens currently when using firejail --shutdown=foo? Or in other words, is the --shutdown=name/pid construct affected by this in any way? Just trying to understand the code, especially https://github.com/netblue30/firejail/blob/master/src/firejail/shutdown.c

@SkewedZeppelin
Copy link
Collaborator Author

@glitsj16 it sends a SIGTERM and if after 10 seconds it hasn't closed it sends a SIGKILL.
I am unsure which process it goes to.

@glitsj16
Copy link
Collaborator

glitsj16 commented Feb 15, 2019

@SkewedZeppelin I noticed the SIGTERM/SIGKILL routine, thanks for explaining. If I read shutdown.c correctly, the SIGKILL goes to the child first. But I'm at best a beginner with C. And I was unsure too if that corresponds with what smithsohu refers to as first/second firejail process. Currently I don't have any firejail sandbox running as root, so I shouldn't have to worry about this. Sorry for the noise here, it's reassuring to see CVE's are getting proper attention/discussion in firejail!

@smitsohu
Copy link
Collaborator

smitsohu commented Feb 15, 2019

@glitsj16 yes, --shutdown always kills the second firejail process/child first:

--shutdown accepts pids and sandbox names. If a sandbox name was passed, firejail will translate it to the pid of the first process/firejail parent, but a user can pass the pid of the second process/firejail child as well....
If pid is the parent, the child will be force-killed first (though the timespan between the two kills is very short). If pid is the child, it is the target of the second force-kill, and the parent terminates regularly.

Anyway, firejail --shutdown=sth itself is another running instance of the firejail binary that outlives the sandbox that is being shut down, so I guess this should be safe.

@smitsohu
Copy link
Collaborator

so I guess this should be safe.

And that was wrong, --shutdown also had this problem (now fixed in shutdown.c)

@glitsj16
Copy link
Collaborator

👍

smitsohu added a commit that referenced this issue Feb 18, 2019
cf. #2401
increase head start to a full second
@chiraag-nataraj chiraag-nataraj added bug Something isn't working information_old (Deprecated; use "doc-todo" or "needinfo" instead) Information was/is required in testing A bugfix that is being tested labels Feb 18, 2019
@t4777sd
Copy link

t4777sd commented Apr 29, 2019

In any case --noroot is expected to defend against this.

For clarification, did profiles with noroot protect against this even if the parent process was killed before the child?

@smitsohu
Copy link
Collaborator

smitsohu commented Apr 30, 2019

For clarification, did profiles with noroot protect against this even if the parent process was killed before the child?

I would say yes, noroot defends in any case. The attacker being root is a necessary precondition, otherwise there is no write permission on the Firejail binary.

Edit: while the above is true in most scenarios, some capabilities (like setuid, dac_override) allow to work around noroot. So if a way exists to somehow acquire one of these capabilities, at least one option out of nonewprivs/seccomp*/protocol/memory-deny-write-execute/caps.drop=all would be necessary in addition to noroot. doesn't apply, as the new user namespace has no capabilities in the parent namespace

@reinerh
Copy link
Collaborator

reinerh commented May 31, 2019

This issue got CVE-2019-12499 assigned.

@reinerh reinerh added the security Security issue label May 31, 2019
@reinerh
Copy link
Collaborator

reinerh commented May 31, 2019

@netblue30 Can you please update the CVE status page? https://firejail.wordpress.com/download-2/cve-status/

@netblue30
Copy link
Owner

LTS version also released, CVE status page updated - https://firejail.wordpress.com/download-2/cve-status/

@Vincent43 Vincent43 removed the in testing A bugfix that is being tested label Jun 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working information_old (Deprecated; use "doc-todo" or "needinfo" instead) Information was/is required security Security issue
Projects
None yet
Development

No branches or pull requests

8 participants