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

Security issue: HTTPS server trust is never verified. #168

Closed
aef opened this issue May 5, 2014 · 6 comments
Closed

Security issue: HTTPS server trust is never verified. #168

aef opened this issue May 5, 2014 · 6 comments
Labels

Comments

@aef
Copy link

aef commented May 5, 2014

Gem in a Box does never verify the validity of the certificate returned by HTTPS servers it communicates to, nor does it allow to enable this through simple configuration.

This is a severe security issue. If you do not verify if the communication partner is the one you really want to talk to, it could literally be anyone. Encryption doesn't save your messages from being read by strangers if you do not make sure you aren't talking to a stranger in the first place. This means security is effectively disabled in Gem in a Box's current state.

I do not understand at all why verification has been explicitly disabled in your code: https://github.com/geminabox/geminabox/blob/master/lib/geminabox/http_adapter/http_client_adapter.rb#L22 . Please correct this broken behavior.

Should you not intend to fix this you should at least warn people about this: The sentence "Defining your own adapter also allows you to configure Geminabox to use the local systems SSL certificates." from your documentation does a bad job at telling users that security is basically disabled completely if you do not write your own adapter. Personally I did not realize what this actually means before I reviewed your code. You should also consider rejecting "https://" URLs completely with a warning because if people explicitly use HTTPS they do this with an expectation of authenticity and protection from eavesdropping, which is not true in this case.

@reggieb
Copy link
Member

reggieb commented May 5, 2014

Yes, I agree. It's time this issue was addressed. I think we need to leave the default behaviour as is, but make it very easy to enable SSL in a way that is easy to alter for a range of environments, and alter the documentation to emphasis that configuring this option is the recommended solution.

@tomlea
Copy link
Member

tomlea commented May 5, 2014

I'm happier with secure by default, and you can poke your own holes in it.

How does https://github.com/geminabox/geminabox/blob/ssl-certs/lib/geminabox/http_adapter/http_client_adapter.rb feel to you two?

I've not had chance to run it, beyond rake, but it should result in the system trusted SSL certificates being used. It seems like the happiest way of dealing with httpclient's usage of its own private (3 years out of date) certificate bundle.

It does make SSLTest#test_method fail. So I'm working on updating the cert.

@tomlea
Copy link
Member

tomlea commented May 5, 2014

With the new cert I just pushed, I added the localhost.crt to my system keychain, and ran the following to pull it back into the openssl CA trust root:

cert_file="$( openssl version -d | awk -F'"' '{print $2}' )/cert.pem"
mkdir -p "${cert_file%/*}"
security find-certificate -a -p /Library/Keychains/System.keychain > "$cert_file"
security find-certificate -a -p /System/Library/Keychains/SystemRootCertificates.keychain >> "$cert_file"

Now my tests pass, and we're secure by default.

Anyone who needs to talk to another server just needs to add the specific certificate for that server to the system CA bundle on their machine.

@reggieb
Copy link
Member

reggieb commented May 5, 2014

That looks good. It should be easy for people to roll their own adapter if that doesn't suit a particular environment.

@aef
Copy link
Author

aef commented May 5, 2014

@reggieb I'm very glad to see you acknowledge that change is needed on this. Thanks.

@cwninja I would be happy about an "secure by default" approach. Although I hope that the #set_default_path method is using OpenSSL's default trust store (/etc/ssl/certs on Debian based systems) and not the cacert.p7s file bundled with the httpclient gem, because I couldn't find any information about the policy used for deciding whom to trust.

@tomlea
Copy link
Member

tomlea commented May 5, 2014

@aef yarp, from experimentation, #set_default_path is consuming the system default. I've set the travis CI build to stuff the key into the system cacerts path, confirmed fail without it confirmed pass with it. So I'm happy.

Also looking into giving better error messages when running the tests without the cert set up, and on cert failures when fetching upstream gems.

@tomlea tomlea added the Stale label Feb 15, 2016
@tomlea tomlea closed this as completed Feb 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants