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

Add Capsicum support #735

Merged
merged 26 commits into from Sep 10, 2017

Conversation

Projects
None yet
6 participants
@trasz
Contributor

trasz commented Jul 30, 2017

Add Capsicum (http://man.freebsd.org/capsicum) sandboxing support. When enabled ("/capsicum enter"), it greatly minimizes the consequences of a potential security hole. It's FreeBSD-specific, but might serve as a guideline to implement sandboxing for other systems. It's also rather minimally intrusive, so shouldn't cause any maintainership burden in the future.

One known bug is that TLS doesn't work. This might be addressed in the future; I believe the problem is that we'd need so somehow preload the CA certs.

trasz added some commits Jul 25, 2017

Make autotools detect Capsicum.
Signed-off-by: Edward Tomasz Napierala <trasz@FreeBSD.org>
Implement /cap_enter.
Signed-off-by: Edward Tomasz Napierala <trasz@FreeBSD.org>
Add capability mode error/success messages.
Signed-off-by: Edward Tomasz Napierala <trasz@FreeBSD.org>
Rename to "/capability enter" and "/capability status".
Signed-off-by: Edward Tomasz Napierala <trasz@FreeBSD.org>
Config file support for "capsicum" parameter.
Signed-off-by: Edward Tomasz Napierala <trasz@FreeBSD.org>
Reorder functions.
Signed-off-by: Edward Tomasz Napierala <trasz@FreeBSD.org>
Make DNS work in capability mode.
Signed-off-by: Edward Tomasz Napierala <trasz@FreeBSD.org>
Working /log and /rawlog.
Signed-off-by: Edward Tomasz Napierala <trasz@FreeBSD.org>
Prevent the user from calling "/capsicum enter" twice.
Signed-off-by: Edward Tomasz Napierala <trasz@FreeBSD.org>
Fix warnings.
Signed-off-by: Edward Tomasz Napierala <trasz@FreeBSD.org>
Working autolog.
Signed-off-by: Edward Tomasz Napierala <trasz@FreeBSD.org>
Restrict port range available in capability mode.
Signed-off-by: Edward Tomasz Napierala <trasz@FreeBSD.org>
Add wrappers to reduce #ifdefs.
Signed-off-by: Edward Tomasz Napierala <trasz@FreeBSD.org>
Fix build without Capsicum.
Signed-off-by: Edward Tomasz Napierala <trasz@FreeBSD.org>
Cosmetics.
Signed-off-by: Edward Tomasz Napierala <trasz@FreeBSD.org>
Fix trailing slash handling for capsicum_irclogs_path.
This is mostly an anti-footshooting measure, but still.

Signed-off-by: Edward Tomasz Napierala <trasz@FreeBSD.org>
Hook up capsicum.h and fe-capsicum.h to autotools.
This hopefully fixes Travis build.

Signed-off-by: Edward Tomasz Napierala <trasz@FreeBSD.org>
Update copyrights.
Signed-off-by: Edward Tomasz Napierala <trasz@FreeBSD.org>
@ahf

This comment has been minimized.

Member

ahf commented Jul 31, 2017

Exciting project!

I have a couple of questions before I walk over the code to review it:

  1. Having an explicit command to enter the sandbox will lead to some confusion. Would it be possible to add a layer of indirection where we start the irssi binary with a sandbox option where we enable the sandbox before we start doing anything with the network? The only provider of this sandbox layer would be Capsicum right now.
  2. I remember Capsicum having a support daemon (casperd?) for accessing the operating systems' random number generator - could that be related to TLS not working? We need to address the TLS issue before we merge.

Thanks for doing this work. I will be walking over the code line-by-line before the end of the week.

@trasz

This comment has been minimized.

Contributor

trasz commented Jul 31, 2017

  1. The /capsicum enter command is mostly for experimentation. It might come in handy, but the "proper" way is to use the "capsicum = on" setting, which causes it to do what you suggest: enter the sandbox before connecting to the servers.

  2. The TLS connection seems to be negotiated ok, which (I hope) means OpenSSL was able to get some randomness. The problem is afterwards, with certificate validation.

Thanks :-)

trasz added some commits Aug 1, 2017

Clean up includes a bit.
Signed-off-by: Edward Tomasz Napierala <trasz@FreeBSD.org>
Change the way we load default CA certificates so it works with Capsi…
…cum.

Signed-off-by: Edward Tomasz Napierala <trasz@FreeBSD.org>
Attempt to fix build by adding the forgotten header.
Signed-off-by: Edward Tomasz Napierala <trasz@FreeBSD.org>
@ailin-nemui

This comment has been minimized.

Contributor

ailin-nemui commented Aug 7, 2017

I think it would be better to #ifdef each capsicum_ function call individually rather than doing some macro redirection in capsicum.h

@trasz

This comment has been minimized.

Contributor

trasz commented Aug 8, 2017

So basically like with 12e2c46 reverted?

@ailin-nemui

This comment has been minimized.

Contributor

ailin-nemui commented Aug 8, 2017

yes but if you prefer you can make a hybrid:
#if CAPSICUM
capsicum_open_wrapper()
#else
open()
#endif
it could ease readability a bit

Add back some ifdefs.
Signed-off-by: Edward Tomasz Napierala <trasz@FreeBSD.org>
@ailin-nemui

This comment has been minimized.

Contributor

ailin-nemui commented Aug 9, 2017

lgtm, any objections @dequis @LemonBoy?

@dequis

This comment has been minimized.

Member

dequis commented Aug 9, 2017

Uh, not sure I agree about that last change improving readability...

@ailin-nemui

This comment has been minimized.

Contributor

ailin-nemui commented Aug 9, 2017

I want to be able to run unifdef on it to kill capsicum, and not being stuck in wrappers. this way is the most clean I can think of lest of making a generic VTable based wrapper class (for the still to be finished seccomp implementation)

@h1z1

This comment has been minimized.

h1z1 commented Aug 10, 2017

There are so many "sandbox" proposals now I'm curious why this would be required at all? Should it not be up to external projects to maintain their integrations? I don't use FreeBSD, I do however use firejail in linux for example.

success = X509_STORE_set_default_paths(store);
if (success == 0) {
g_error("Could not load default certificates");

This comment has been minimized.

@dequis

dequis Aug 10, 2017

Member

This was copied from a line that had g_warning() originally.

g_error() is fatal and results in abort(). Probably not the wisest thing to do if there's no system-wide cert store.

I'd also remove the return FALSE below to let it set ssl_inited.

(Hard-failing on X509_STORE_new errors seems fine, it's mostly malloc failures)

@trasz

This comment has been minimized.

Contributor

trasz commented Aug 10, 2017

There are many sandbox proposals on Linux; other systems, however, usually already have their own. And for FreeBSD, the official, native mechanism is Capsicum.

Don't error out on failure to load default certificate store.
This restores the previous behaviour.

Signed-off-by: Edward Tomasz Napierala <trasz@FreeBSD.org>
@ailin-nemui

This comment has been minimized.

Contributor

ailin-nemui commented Aug 10, 2017

ahf would furthermore request that the /capsicum enter is removed. maybe a command line switch could be used, as was originally proposed for linux in https://github.com/irssi/irssi/pull/342/files#diff-b2e1bd931ec6784cb91bfad7c8e679adR305

@trasz

This comment has been minimized.

Contributor

trasz commented Aug 10, 2017

Hm, not a big deal, but why? It might come in handy if you want to do something before you enter the sandbox.

@h1z1

This comment has been minimized.

h1z1 commented Aug 10, 2017

Even in the case of Linux, it's not portable or really scalable. There are other means to achieve the same thing without resorting to code requiring hacks. Quite frankly applications should not be dictating system configuration.

@trasz

This comment has been minimized.

Contributor

trasz commented Aug 10, 2017

None of those mechanisms are portable - and will probably never be. It's not a hack, however, because this is not system configuration: capability mode is something between the process and the operating system, not something for the system administrator or package maintainers to care about. The idea is briefly explained at https://en.wikipedia.org/wiki/Capability-based_security, although I'm not sure how good that explanation is.

In other words - yeah, it it was some SELinux "let's restrict access to random bunch of stuff and hope it somehow helps security" mechanism, then it would be a rather ugly hack and shouldn't be put into the software itself. But it's an entirely different approach.

@h1z1

This comment has been minimized.

h1z1 commented Aug 10, 2017

I don't follow, sorry. They are portable in that they don't all require code changes. Firejail, SELinux, sandbox (utility in RH), all for example work without modification of any kind nor upstream support. Contrast with cancerous things like Docker, "snap apps", etc.. It's actually better the application doesn't know nor care about system level details.

I'm aware of what caps are, and also find it ironic if it's as you say.. secure and not to be worried about.. why dropping capabilities is needed. Think you'll find it's the complete opposite and yes, does little for real security.

@trasz

This comment has been minimized.

Contributor

trasz commented Aug 10, 2017

Just to make sure: you are aware this has absolutely nothing to do with old POSIX.1e draft capabilities, as implemented in Linux, apart from the name? We are not "dropping capabilities" here.

I think we're drifting a bit off-topic, though. Making use of Capsicum - which won't help Linux, unless Google gets their port upstreamed, but which is the sandboxing mechanism on FreeBSD - requires some code changes. I don't think this patch will cause any maintainership problems for you - after all, it's a few hundred lines of code, vast majority in a single file. Maintaining it outside, however, would require a manual action each time you release, which is additional work.

@h1z1

This comment has been minimized.

h1z1 commented Aug 11, 2017

Just to make sure: you are aware this has absolutely nothing to do with old POSIX.1e draft capabilities, as implemented in Linux, apart from the name? We are not "dropping capabilities" here.

I do indeed. It was in response to your assertion that capabilities are not something I should worry about:

not something for the system administrator or package maintainers to care about.

I absolutely DO worry about what permissions applications have.

I think we're drifting a bit off-topic, though. Making use of Capsicum - which won't help Linux, unless Google gets their port upstreamed, but which is the sandboxing mechanism on FreeBSD - requires some code changes. I don't think this patch will cause any maintainership problems for you - after all, it's a few hundred lines of code, vast majority in a single file.

It is indeed a mess. My point was more to how other technologies like it will be integrated if this is the path irssi takes. There's no real reason why the linker couldn't be used for example. I mean looking at the changes to core//network.c alone has me baffled how that would be accepted.

Maintaining it outside, however, would require a manual action each time you release, which is additional work.

With the way this is implemented, I agree. Either way I suppose, forked and backed out locally. Cheers.

@ailin-nemui

This comment has been minimized.

Contributor

ailin-nemui commented Aug 18, 2017

I would like to see some more investigation into the topic of custom ca certificates (/server -tls_cafile / -tls_capath)

what would you want to do before /capsicum enter?

@trasz

This comment has been minimized.

Contributor

trasz commented Aug 25, 2017

I'm not sure what's the best approach for the custom certificates. The problem is that they might be located anywhere, so opening them is exactly what we want the sandboxing system to prevent from happening.

As for "/capsicum enter" - literally anything. Run scripts, for example. Or... load the custom certificates :-)

@ailin-nemui

This comment has been minimized.

Contributor

ailin-nemui commented Aug 26, 2017

@ahf please let @trasz know what he needs to do before this can be merged

@trasz could you include a capsicum.txt in the docs folder explaining roughly what it does (and what the limitations) and how to use?

@LemonBoy

This comment has been minimized.

Member

LemonBoy commented Aug 26, 2017

It is indeed a mess. My point was more to how other technologies like it will be integrated if this is the path irssi takes.

I agree that having a minimal amount of scaffolding to abstract over the capsicum-specific details would be a much better idea in the long run if we ever add support for seccomp or whatever the sandboxing library du jour is.

Run scripts, for example. Or... load the custom certificates :-)

The certificates are loaded in irssi_ssl_get_iochannel which I guess is too late to load the certificates. The tls_cert may be loaded by replacing the fopen with a call to the capsicum-wrapped open() (and letting irssi access the content inside ~/.irssi, from what I gather that's forbidden atm).
Also, is capsicum_irclogs_path necessary? Can't you just use autolog_path?

Add docs/capsicum.txt.
Signed-off-by: Edward Tomasz Napierala <trasz@FreeBSD.org>
@trasz

This comment has been minimized.

Contributor

trasz commented Aug 31, 2017

The problem with the "scaffolding" is that at this point we don't have support for any other sandboxing mechanism in Irssi, and so it would be an extrapolation from a single data point. From my experience what you usually end up in cases like that is an abstraction layer that needs to be reworked anyway.

Regarding the certificates: I generally agree, but there are few more things to keep in mind. First: I'm not sure if I want to allow access to ~/.irssi/, but yeah, if it's the only way to fix custom certificates - why not. Another way would be to add another configurable directory path (like capsicum_certificates_path). Yet another would be to just make the symbiont load the certificates - making sure it won't load anything that doesn't parse as a certificate - and somehow "forward them" to the sandboxed process. Still - that's one of the things I'd rather leave for now, especially given that (from what I understand) there's some work to make it possible to use GnuTLS instead of OpenSSL.

As for capsicum_irclogs_path - it's because I'm not sure how to extract the directory alone from the autolog_path. Is it even possible in all cases?

@LemonBoy

This comment has been minimized.

Member

LemonBoy commented Aug 31, 2017

I'm not sure if I want to allow access to ~/.irssi/

Why not? If there's a folder irssi should have control on is its own :)

Another way would be to add another configurable directory path

We should avoid adding even more switches and toggles.

there's some work to make it possible to use GnuTLS instead of OpenSSL

Lies, damn lies and TLS implementations. There's an old patch floating around the internet, but that's it.

As for capsicum_irclogs_path - it's because I'm not sure how to extract the directory alone from the autolog_path. Is it even possible in all cases?

The autolog_path can be customized using the expandos but I think it should be possible to obtain the "base" component and use that as mkdirat/openat base.

@trasz

This comment has been minimized.

Contributor

trasz commented Sep 1, 2017

The problem with ~/.irssi/ is that it would allow the sandboxed process to modify ~/.irssi/config. And this could be used to disable the sandbox next time Irssi gets restarted - or just run something at startup.

Regarding the autolog_path - well, I suppose I could just parse eg /home/trasz/irclogs/$tag/$0.log to get /home/trasz/irclogs; I'm not sure, however, it would correctly handle all the cases. What if someone puts $whetever above their home directory, and the path ends up being above (closer to the filesystem root) their home directory? This would allow the sandboxed process to access all that, rendering the sandbox pretty useless. Even worse, it wouldn't be immediately obvious to the user that this configuration is dangerous.

Thanks for clarifying the situation with GnuTLS; one less thing to worry about :-)

Fix /back in Capsicum capability mode.
Signed-off-by: Edward Tomasz Napierala <trasz@FreeBSD.org>

@ailin-nemui ailin-nemui merged commit 24ad801 into irssi:master Sep 10, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment