PaperCutSoftware / silver

SILVER - A cross-platform service/daemon wrapper with in-build auto-update, crash resilience and more.
MIT License
54 stars 13 forks source link

Vulnerability #4

Closed neuegram closed 8 years ago

neuegram commented 9 years ago

It has come to my understanding that PaperCut plans to use Silver in an upcoming release (~1 year). I was told that the idea behind Silver and the product it will be included in is to "host an agent service installed at customer sites that needs to operate reliably with zero on-site maintenance and configuration." Unfortunately, this is not easily done. Here is my take on the security of Silver. This basic, common mistake that most software makes is that it updates over HTTP (or binaries are distributed over HTTP). However, Silver takes that a step further by allowing arbitrary command injection. Read more here.

codedance commented 9 years ago

Thanks @neuegram . That’s the benefit of open source! Many eyes keep things secure. Thanks for taking the time to review the code and make some suggestions. As mentioned, we’re not using Silver in production just yet. We’re working on other aspects of our auto updating functions but did this code very early on in the project to proof-of-concept the flow, stability and viability across all the major platforms (Mac, Linux and Windows). We’ve also make this work open so others can benefit. In this respect it’s been great. We’ve had a few external contributions as well. Open source is great! Many eyes make great software.

Regarding the issue you raised. You have a few good points and discuss them below. I’d also welcome your input/suggestion on a few proposed changes.

I don’t think it’s a security issue per se, but rather an issue with “It’s too easy to shoot yourself in the foot”. The design intention is that the update URL should only ever point to a signed HTTPS server (i.e. the check URL should be in the form of https://myupdateserver.org/updatecheck ). Because the “author” (the person who supplied the first installed version) controls the update check URL, they can control this location - the trust chain starts with the first install and all upgrades following. Our assumptions/designs are:

I can however see your point of view. Where I think things fall down from a security point of view is that it fails the “secure-by-default rather than secure-by-configuration” test.

I’ve made the following code changes to close this (and of course will be included in the documentation when we finish this off :-):

This changes should make it impossible to accidentally use a non-secure configuration.

Future Ideas:

What are your thoughts on these changes?

Thanks for the contribution.

Cheers!

Chris

codedance commented 9 years ago

Code changes in 51e0281f61daac4badbb2c1f31ccd215852bbcef

codedance commented 9 years ago

@neuegram ping!

neuegram commented 9 years ago

@codedance Sorry I haven't had a chance to get back to you. The proposed changes look good. There's really no replacement for secure by design, but I fully understand that isn't always an option.

I like the idea of TLS. Go makes configuration pretty safe, although there's always some risk to be had. Self-signed certificates could mess this up, although pinning would resolve that.

As for signatures, SHA1 is deteriorating quickly. SHA256 or other variants might be slower than what you're looking for. For security and performance, check out BLAKE2b.

Lastly, I understand that the command execution might be necessary here, but there will always be a case against it. I have seen software that uses checksums, codesigning, TLS, etc. unable to avoid certain vulnerabilities linked to software updates, command execution, etc. Some risk is always unavoidable and I believe your suggestions should make a notable difference in improving security. Using a memory safe language like Go is in and of itself a large step towards security.

codedance commented 9 years ago

Hi @neuegram

Thanks for jumping on this.

The TSL implementation in the Go std lib will raise an error on a self-signed certificate, so this should be safe. Regarding Sha256: It's about as twice as slow as MD5, but I think still fast enough for this use-case. For example, the code at it stands can validate 500Mb of install in under 6 seconds on my medium spec laptop. I did a quick search. There is a Go implementation of Blake2. https://godoc.org/github.com/codahale/blake2 I'll check it out.

Thinking about command execution. One idea I just had is to maybe add an option to ignore the "commands". That way, if you're upgrade/installer does not need the feature you can at least turn it off. This way it will at least minimise the surface area - option out. Thanks for pushing on this and opening up the thinking.

I'll also create an issue to look at adding the following:

codedance commented 8 years ago

Added command line option called "unsafe" to turn off HTTPS requirement. Made it clear that this is for debug/testing purposes only. Closing this issue off.