brianleroux / tiny-json-http

:anchor: Minimalist HTTP client for JSON payloads.
172 stars 18 forks source link

Don't disable SSL certificate validation #15

Closed jasonk closed 6 years ago

jasonk commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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!

jasonk commented 6 years ago

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

brianleroux commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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