netblue30 / firejail

Linux namespaces and seccomp-bpf sandbox
https://firejail.wordpress.com
GNU General Public License v2.0
5.77k stars 565 forks source link

firejail should never fail to find its helper binaries for arbitrary install paths #2148

Closed crass closed 6 years ago

crass commented 6 years ago

I found this annoying as I wanted to install a development version in a sub-directory of $HOME. But when I run firefox with firejail using the development binary and profile, I get the following debug lines:

sbox run: /home/user/development.forked/firejail.git/installed/lib/firejail/fseccomp drop /run/firejail/mnt/seccomp /run/firejail/mnt/seccomp.postexec @clock,@cpu-emulation,@debug,@module,@obsolete,@raw-io,@reboot,@resources,@swap,acct,add_key,bpf,fanotify_init,io_cancel,io_destroy,io_getevents,io_setup,io_submit,ioprio_set,kcmp,keyctl,mount,name_to_handle_at,nfsservctl,ni_syscall,open_by_handle_at,personality,pivot_root,process_vm_readv,ptrace,remap_file_pages,request_key,setdomainname,sethostname,syslog,umount,umount2,userfaultfd,vhangup,vmsplice allow-debuggers (null) 
Dropping all capabilities
Drop privileges: pid 7, uid 1000, gid 1000, nogroups 1
No supplementary groups
execvp: No such file or directory
Error: failed to run /home/user/development.forked/firejail.git/installed/lib/firejail/fseccomp
Error: proc 26027 cannot sync with peer: unexpected EOF

If I add --whitelist=/home/user/development.forked/firejail.git/installed/lib/firejail, things work as expected.

Also, if firejail is installed under some mount directory (ie /media/extra_storage/local-install), a similar error happens when firejail is run with the --disable-mnt option. A similar problem would likely happen if $HOME is on a different mount point under /media and symlinked in /home. However, here the problem is not resolved with an added --whitelist option. The issue appears to be that by the time firejail attempts to whitelist the firejail lib install directory, /media has already been blacklist and thus set to deny users read access.

Firejail should succeed in both these cases (and in all cases where it has been properly installed). The first issue is fairly straight forward to resolve, but the second one is trickier.

Fred-Barclay commented 6 years ago

Hi @crass I haven't been able to think much about your second case yet, but on the first one: what you're seeing sounds exactly like what I'd expect (and want) firejail to do. :smile: My suspicion is that allowing firejail to automatically allow access to any non-whitelisted directories, simply because it thinks it's installed there, is opening up the potential for abuse.

Perhaps the others will have a different perspective and be able to correct me here, though.

(Also my brain is telling me that having a SETUID binary in your /home may not be the best idea, but I don't have a specific example why off the top of my head.)

crass commented 6 years ago

Hey @Fred-Barclay, well I should hope that if you installed firejail to some location that you'd expect it to actually work securely. Remember, firejail can see both inside and outside the container, so why should it ever have a problem finding the right executables for itself? It has been installed by someone, and that someone clearly wants a functioning firejail where they installed it... otherwise why install it? If firejail were a single binary, as opposed to having multiple helpers, this wouldn't be a problem... yet you would still expect a failure in that scenario? I was going to say, "I'm not saying that there is anything problematic with the current helper executables model". But on further thought, I'm not seeing a disadvantage to a single executable and at least one clear advantage (yes a single executable is not exactly the UNIX way).

Also, I never suggested "allowing firejail to automatically allow access to any non-whitelisted directories". What I'm suggesting is to specifically allow access to the helper executables that firejail has installed. Let's look at two different scenarios and their security implications.

  1. Install firejail into $HOME using an unprivileged user. All files will be owned by the user and the setuid bit will be set on the firejail executable. Are there security problems here? I'd say not. Why not? Well, what the setuid bit being set on an executable means that that executable may request to change to the user that owns the executable. In this case, its the same user. The firejail process won't be able to do anything interesting because it will only ever be able to run as the unprivileged user. So, it doesn't really make sense to install as a regular user, and that's not my scenario.

  2. Install firejail into $HOME using root. Here it gets interesting. All files will be owned by root and the setuid bit will be set on the firejail binary. Keep in mind that the executables have been installed in LIBDIR and that LIBDIR was created by make install and thus is root owned with root owned files inside. So even though the parent directory of LIBDIR may be user owned, how is it that a malicious piece of code running as the unprivileged user will be able to leverage this to escalate privileges or even mess with the execution of firejail itself? Since LIBDIR is root owned, the user can not delete the helper programs in there. The user will not be able to modify those same helper programs either because the binaries are also root owned and not writable by unprivileged users (if it weren't it would be a security issue even if firejail was installed in the default location). And the user will not be able to change the ownership of the files either. The same logic applies to the firejail binary in bin/. Ok, so far so good. Now what about the setuid bit of the binary? Well that will allow firejail to gain root privileges, but since the binary can't be modified by an unprivileged user, it will just be the normal firejail running... nothing special.

QED

Hopefully that'll put you more at easy :stuck_out_tongue_closed_eyes:

reinerh commented 6 years ago

Install firejail into $HOME using an unprivileged user. All files will be owned by the user and the setuid bit will be set on the firejail executable. Are there security problems here? I'd say not. Why not? Well, what the setuid bit being set on an executable means that that executable may request to change to the user that owns the executable. In this case, its the same user. The firejail process won't be able to do anything interesting because it will only ever be able to run as the unprivileged user. So, it doesn't really make sense to install as a regular user, and that's not my scenario.

This has security implications on multi-user systems. By default home directories are world-readable, so when you install a suid-binary (for your user) in your home, other users can run stuff as your user.

reinerh commented 6 years ago

Install firejail into $HOME using root. Here it gets interesting. All files will be owned by root and the setuid bit will be set on the firejail binary. Keep in mind that the executables have been installed in LIBDIR and that LIBDIR was created by make install and thus is root owned with root owned files inside. So even though the parent directory of LIBDIR may be user owned, how is it that a malicious piece of code running as the unprivileged user will be able to leverage this to escalate privileges or even mess with the execution of firejail itself? Since LIBDIR is root owned, the user can not delete the helper programs in there. The user will not be able to modify those same helper programs either because the binaries are also root owned and not writable by unprivileged users (if it weren't it would be a security issue even if firejail was installed in the default location). And the user will not be able to change the ownership of the files either. The same logic applies to the firejail binary in bin/. Ok, so far so good. Now what about the setuid bit of the binary? Well that will allow firejail to gain root privileges, but since the binary can't be modified by an unprivileged user, it will just be the normal firejail running... nothing special.

I think that also has security problems. As a user you could rename LIBDIR and substitute it with your own malicious replacement.

Vincent43 commented 6 years ago

Since LIBDIR is root owned, the user can not delete the helper programs in there. The user will not be able to modify those same helper programs either because the binaries are also root owned and not writable by unprivileged users

Unfortunately, everything what you wrote is wrong, for example see: https://ervinb.github.io/2017/08/16/casually-removing-root-files/ . Please learn how things work before presenting your own ideas here.

reinerh commented 6 years ago

@Vincent43 If a (non-empty) directory is root-owned it cannot be removed (even in your home directory).

Edit: but as I mentioned, it can be renamed.

Vincent43 commented 6 years ago

Did you read what I linked? Did you checked this?

$ sudo mkdir root
$ touch root/aaa
touch: cannot touch 'root/aaa': Permission denied
$ rm -rf root
$ touch root/aaa
touch: cannot touch 'root/aaa': No such file or directory
reinerh commented 6 years ago

Did you read what I wrote? Non-empty root-owned directory.

root # mkdir /home/reiner/foo
root # touch /home/reiner/foo/bar
root # logout
reiner $ rm -f foo/bar 
rm: cannot remove 'foo/bar': Permission denied
reiner $ rm -rf foo/
rm: cannot remove 'foo/bar': Permission denied
crass commented 6 years ago

Thanks @reinerh. Its is annoying to have someone correct me about something so simple and then they be wrong about it... Please, I've been using linux for over 20 years...

Anyway, @reinerh, you are correct about moving the directory and honestly I hadn't thought of it. I think this reinforces my idea that we should have it bundled as a single binary. I think that would solve these head aches without the potential security issues.

smitsohu commented 6 years ago

I think this reinforces my idea that we should have it bundled as a single binary. I think that would solve these head aches without the potential security issues.

Firejail was split into several binaries to actually increase the security. Though many of the things we are doing now (dropping privs, internal sandboxes) could also be done within a single binary, execve is a powerful boundary. Putting everything in a single binary is something we cannot do, it looks like a step backwards.

Vincent43 commented 6 years ago

@reinerh You're right in this specific example, sorry.

@crass What's the difference between removing LIBDIR completely and moving it to .null. It's the same effect.

Attacker may put any code they want in place of firejail binary, like:

~/LIBDIR/firejail
#!/bin/sh
rm -rf ~/*

which you will happily execute. There is no need for setuid, root and anything else.

crass commented 6 years ago

@smitsohu, why do you think I'm saying to get rid of execve? I'm thinking of the code path for the single binary situation be virtually identical to the current code path. The main difference will be that there will some stub logic at the start of main that forks+execs different code based on argv[0]. Do you have any reason to believe that this would be less secure? I'm not totally convinced that it is just as secure btw. My main concern is that the single binary situation would necessarily be like the current situation but with all the helpers having the setuid bit set. So that seems dangerous, but I think if we have the first line of code be to setuid and that should nullify the effect.

@Vincent43 you can say that about any piece of code that you execute via an unprivileged user. Do you have scripts that you've written that you use? Do you check every line before you use them each time to make sure you're not running malicious code? The obvious answer is no. What we need to be talking about are specific threat models. I think the specific threat model we should consider is the one where the attacker is in control of the unprivileged user inside the sandbox. If the attack gets outside of the sandbox, then firejail was worthless and can be disregarded for that scenario. And in this case the attacker can do as you say, but so what? I don't ever run firejail from _inside__ the sandbox and it won't matter for the sandbox itself because its already setup.

But anyway, I wasn't actually suggesting that the solution be something so naive as just whitelisting LIBDIR. I was thinking more along the lines of having a special mountpoint in the sandbox for putting the LIBDIR and potentially other files. The absolute path to the directory would be static regardless of where firejail was actually installed to. And the contents of the directory would not be accessible by the unprivileged user.

I want to reiterate that the threat model doesn't involve attackers outside the sandbox, that's out of scope for firejail.

crass commented 6 years ago

@smitsohu

Putting everything in a single binary is something we cannot do, it looks like a step backwards.

From an engineering perspective, that is a horrible perspective and from a security perspective down right dangerous. I hope I'm misinterpreting that. It does not matter what things "look" like. Do we have a political constituency that we need to be concerned with their opinion? This isn't Vietnam, where we think we can't go backwards because we'll "lose credibility" and we must find a way to retreat without "losing face". And it wasn't even the it wasn't right.

Obviously, "putting everything in a single binary" is something that you can do, its a question of will you. And that question should not be answered in any particular way by appearances or where you came from. From an engineering and security perspective, it would be answered by how it compares to where you are right now and perhaps where you want to go. Are the proposed changes more or less secure? Do the proposed changes allow for a greater degree of flexibility, simplification of the code base, or actually solve a problem? [/rant]

Anyway, not wanting to attack you, just the idea expressed. :smiley:

smitsohu commented 6 years ago

The main difference will be that there will some stub logic at the start of main that forks+execs different code based on argv[0]

Ok, now I understand what you mean :wink: It might work this way.

My main concern is that the single binary situation would necessarily be like the current situation but with all the helpers having the setuid bit set.

If the very first thing they do is to get rid of the privileges, it should be fine. But this is a significant change, @netblue30 will have to add his comment anyway.

chiraag-nataraj commented 6 years ago

his isn't Vietnam, where we think we can't go backwards because we'll "lose credibility" and we must find a way to retreat without "losing face".

...how exactly did we get from helper binaries to Vietnam?! There needs to be a Godwin's Law for this kind of thing.

SkewedZeppelin commented 6 years ago

But this is a significant change

@smitsohu You said it earlier, there is little reason to go backwards. No need to waste time refractoring for no real gain.

Projects that end up in refractoring hell die a slow death.

crass commented 6 years ago

I'm not convinced there is much refactoring. I might take a stab at it. I'm thinking that actually these could be built side-by-side. Just produce a firejail.fat binary or something.

And btw everything goes back to Nam... just watch the Big Lebowski :stuck_out_tongue_closed_eyes:

Vincent43 commented 6 years ago

@crass

you can say that about any piece of code that you execute via an unprivileged user. Do you have scripts that you've written that you use? Do you check every line before you use them each time to make sure you're not running malicious code? The obvious answer is no

No, because those scripts aren't writable by unprivileged users. It's known security principle to not have files writable and executable at the same time which rules out running some random binaries from $HOME.

Anyway, that was you who wrote:

Since LIBDIR is root owned, the user can not delete the helper programs in there. The user will not be able to modify those same helper programs either because the binaries are also root owned and not writable by unprivileged users (if it weren't it would be a security issue even if firejail was installed in the default location). And the user will not be able to change the ownership of the files either.

which suggest that was in threat model. After prove that above is untrue, it's suddenly not in threat model. It seems the threat model is a moving target.

crass commented 6 years ago

@Vincent43

No, because those scripts aren't writable by unprivileged users. It's known security principle to not have files writable and executable at the same time which rules out running some random binaries from $HOME.

Right, I misspoke, I meant to say "you can say that about any piece of code that is owned and executed by your unprivileged user." Obviously, not every piece of code executed by an unprivileged user is untrustworthy, root owned ones installed by your package manager tend to have a higher degree of trust associated with them.

which suggest that was in threat model. After prove that above is untrue, it's suddenly not in threat model. It seems the threat model is a moving target.

You are correct, the threat model has changed to what it actually should be. Originally I was trying to prove something stricter, but it was unnecessary. Though had it been correct, it would have been sufficient. If you believe that the threat model as explicitly described is insufficient in some way, I'm all ears.

Vincent43 commented 6 years ago

Ok, that's settled then.

I'm skeptical about single binary approach. Main concern against firejail is the size of its setuid binary (about ten times bigger than bubblewrap). Moving code out of it was seen as improvement. There should be some significant security advantages to do it other than hope to be not less secure.

Fred-Barclay commented 6 years ago

Ok, apologies in advance if I mix folks up :smile:

Also, I never suggested "allowing firejail to automatically allow access to any non-whitelisted directories". What I'm suggesting is to specifically allow access to the helper executables that firejail has installed.

Right. What I mean is, allowing firejail to access a directory that it ordinarily would not have access to (because it's not whitelisted in the scenario you described in your first post), if it thinks it is installed inside that folder, sounds to me like a bad idea. It's not inconceivable that one could purposely trick firejail into thinking it's installed somewhere it's not, allowing access to folders that shouldn't be visible inside the sandbox.

I know this is outside your threat model, but we do need to consider attackers outside the sandbox as well.

I did misunderstand your OP and thought you meant installing to your $HOME as your user, not as root. That said, I'm still of the opinion that this isn't a good idea, even when installing as root.

I'm also skeptical of the single binary approach. What you describe might work, but I see it as introducing more complexity for only marginal benefit. The less code I see with the setuid bit, the happier I am! :grin: I'd rather have the current approach then have to drop privs for all the little guys that are currently split out into separate binaries.

To be completely honest, I see where you're coming from. There are definite benefits to your side, and you're right, users should have the expectation of firejail working. But I think this is an edge case that would cause too much refactoring and introduce additional complexity that we don't really need.

By all means, correct when/where I'm wrong.

Cheers!

crass commented 6 years ago

@Vincent43 I think the hope that you speak of would be for those who don't read an understand the code. Just because the helpers are not setuid, doesn't mean there can't be a bug in firejail such that it doesn't drop privs when exec'ing one of the helpers. You have to trust that currently the code doesn't allow that. Until you've verified that it doesn't, you're living with a false sense of security. You should actually think of the helper programs as just bits of code. That code could either be in the firejail binary or outside of it, it doesn't really matter, because either way its conditionally executed by the setuid firejail. So the setuid "contaminates" that code potentially. What does matter, rightly pointed out by smitsohu is how that code is executed (eg. in a separate process or not). I'm suggesting putting all the code in one binary, but I am not suggesting it all runs in the same process. What I'm suggesting is that the single binary scenario will effectively look like the set of firejail processes we have today, just that the code for those processes is packed into one file as opposed to many. Here's another way of looking at it, what if I packed all the files from a build with the current code base into a zipfile, tacked that onto a stub exec header that when run would self-extract into a tmpfs location and run everything from there. So we have this single executable binary that when run changes into a situation that we have to today. Are we less secure under that scenario? What I get from you is that you're reacting from fear because you don't understand exactly what's being proposed. Lack of understanding on your part doesn't make it any less secure.

My vision is something that should be quite easy to prove is semantically equivalent to the current situation. And that this will be obvious from looking at relevant sections of a very small portion of the code.

And no, it absolutely doesn't need to have "significant security advantages" that's not the purpose of this issue. Do you make that requirement of all enhancements? Obviously it should never make firejail less secure, but that also goes for any code change.

@Fred-Barclay My current use case and this issue arises from me installing firejail at a location that I absolutely do not recommend nor condone purely for development purposes. This problem also arises if you install to /media and then turn on certain profile features. I was annoyed that I had to debug why it didn't just work. It seemed reasonable that firejail should always just work (in a secure manner) regardless of how it is installed. If its not going to work when you install in $HOME, then why not have a warning or disallow installing there? As opposed to doing something that appears successful, but is actually broken. So my idea was to change firejail to get rid of this limitation. You are correct that this is a very uncommon case.

I get the keep setuid code as small as possible, and I wholeheartedly agree. But let me make a distinction, what we need be worried about is the code that actually executes under setuid, not necessarily all bytes in a file with setuid perms. Lets take sshd from openssh as a prime example. It was recognized that it had a lot of code that didn't need to be run as root, but that some of it did. So they implemented the privilege separation, which is essentially what firejail is doing with the helper programs. However, how many other executables does sshd use to implement this privilege separation? None. And its not because these guys are security n00bs, Theo de Raadt is legendary. I think people are a little confused on the issue of what poses a security threat.

As for an outside the jail attacker threat model. Please tell me specifically what the threat model is you're thinking about. I'm not saying you're wrong, but we need to be explicit if developers are actually concerned about security. There is one out of sandbox threat model that worries me about firejail currently, and that is local privilege escalation via some a code injection bug in profile parsing. There's a lot of code (still) that is running as root that doesn't need to be. We should focus on having the bare minimum of code run as root.

As for the single binary proposal, the proof is in the pudding. I never had expectations of anyone but me even taking a stab at implementing it. If or when I do, then we'll have something to evaluate. I'm happy to hear specific concerns, but so far I've only hear vague references to security and complexity without much substance (as far as I can tell) to back it up.

I do appreciate your engagement in this though. Thanks!

Vincent43 commented 6 years ago

My vision is something that should be quite easy to prove is semantically equivalent to the current situation

This is my concern. You are proposing an U-turn in firejail development only to maintain the status quo in the end. The only benefit you present is to being easier to run firejail from exotic dirs like /media or /home but 99% users don't care about it as they install firejail from distro repos which is always installed somewhere in /bin.

You have to sell us your idea by providing evidence that it improves security/maintainability, reduce complexity/codebase or whatever - just something that will benefit more than one person, something we should care about.