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

Don't disable SSL certificate validation #15

Merged
merged 1 commit into from
Feb 15, 2018

Conversation

jasonk
Copy link
Contributor

@jasonk jasonk commented Feb 13, 2018

From a security standpoint disabling SSL certificate verification by default is a terrible idea. Not providing any way for the user to turn it back on is even worse.

@brianleroux
Copy link
Owner

Some feedback: it wasn't an idea. It is a deliberate tradeoff for working locally (self signed certs).

I get what you're trying to say however and super open to making this configurable for folks that want to opt into the extra checks. Think changing this default now would count as a breaking change otherwise. Thoughts?

Further context. Our usage of this library is mostly with Slack and I am opting into the risk that I can trust their endpoints haven't been hijacked. Maybe that's naive!

@jasonk
Copy link
Contributor Author

jasonk commented Feb 14, 2018

My usage of this is also with Slack, and the risk is very real. By disabling this you are not just implicitly trusting Slack's endpoints, you are also implicitly trusting every router, switch or wireless access points between your client and their server and every DNS server that responds to your clients requests to resolve their API server.

Basically, anyone using your Slack library is at risk of Man-in-the-middle attacks. It's also worth noting that the reason I discovered this issue is that I had an application using your Slack library for most of it's operations, but using a regular REST client for making calls to their SCIM user-provisioning API. On Feb 8th Slack replaced their SSL certificates with new certs signed by a different CA, which broke our REST client, and we were confused as to why it only broke the REST portion, which is when I discovered that at some point in the past we had updated the library to a version that no longer honored SSL certificates.

It's ironic that this was the event that led to this discovery, because the reason that Slack changed CA's was to protect their users from Man-in-the-middle attacks. They previously had certificates signed by GeoTrust, which was one of several CA's operated by Symantec. On several occasions Symantec's CA's issued bogus certificates that did not comply with the CA/Browser Forum Requirements, which led to the eventual decision for most major browsers to stop trusting any certificate which chained up to a Symantec root, and for Symantec to sell their CA business to DigiCert.

So, protecting users from these kinds of attacks is actually a very big deal. As a developer I completely understand not wanting to make a breaking change, but this is a big deal, and I would strongly recommend that you change the default to not disable validation, and publish a security advisory warning your users that this vulnerability exists.

@jasonk
Copy link
Contributor Author

jasonk commented Feb 14, 2018

Also, as a heads up, because of the environment I'm using this in I'm required to report security vulnerabilities, so there are pending CVE's for both tiny-json-http and slack, so it's probably in your best interest to get ahead of the problem.

It's also worth noting that this is enough of a risk that even though there are only 11 main CVE Vulnerability Types, "Missing SSL certificate validation" is number 8 on that list.

@brianleroux
Copy link
Owner

Cool I appreciate your enthusiasm and urgency. I do not share your sense of concern that this is a gaping exploit or even remotely realistic threat scenario. The recent certs change by many demonstrates that. Regardless let the record show the new default is in place.

Some more feedback. A slightly more responsible disclosure is probably best left out of a public issue tracker! A private message next time would be appreciated. Congrats on the CVE tho. 😏

Thanks again: I do appreciate the heads up!

@brianleroux brianleroux merged commit 3c1e36d into brianleroux:master Feb 15, 2018
@jasonk
Copy link
Contributor Author

jasonk commented Feb 15, 2018

Yeah, sorry about making it public, I realized after it was posted that it probably should have been quieter..

@brianleroux
Copy link
Owner

No worries. I'd bet that the odds of getting hit by lightening twice today are better than a clandestine operative infiltrating data centers and swapping certs. 😜

@jasonk
Copy link
Contributor Author

jasonk commented Mar 9, 2018

This is a much bigger risk than you believe. If someone wanted to intercept the traffic from ALL Slack users, that would be the only time they would have to break into a datacenter and replace certificates on the servers. If someone were after a specific group or individual (such as, for example, someone who wanted to get the Slack token being used by the enterprise management app I working on when I discovered this vulnerability, which would have given them admin-level access to all of our enterprise Slack teams) they wouldn't have to break into any datacenters at all. In fact, the "clandestine operator" wouldn't have to do anything more strenuous than sitting in the same coffee shop as me while I was testing my app with a warm beverage. The possibilities get even scarier if the person using this code were using it for something like a Slack bot that inhabited a channel where dissidents were discussing their plans. The government they were protesting would have no problem getting in the middle of those communications.

@brianleroux
Copy link
Owner

Cool story! Moral of the story: use a VPN when using strange networks. Also: pls no more stories; the issue is closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants