EM-HTTP-Request is not secure
koenrh opened this issue · 3 comments
This HTTP client does not seem to perform any validation on the certificate (and chain) presented by the peer, which is bad. It means it does not provide any guarantees with respect to authenticity and confidentiality of TLS connections.
After reading the EventMachine documentation, I understand the problem is that EventMachine leaves the actual validation of certificates up to the client. Which means that this client should have implemented the ssl_verify_peer method.
It is up to user defined code to perform a check on the certificates. The return value from this callback is used to accept or deny the peer. A return value that is not nil or false triggers acceptance. If the peer is not accepted, the connection will be subsequently closed.
I don't know too much about the implementation of either EventMachine or this client, but I don't think the implementation of certificate validation is something that should be left up to the client. This is something that should, in my opinion, be implemented in EventMachine.
That said, developers and maintainers do have a responsibility of warning developers that, at this time, using this client (in production) is dangerous. Developers rightfully and reasonably expect HTTP clients that advertise HTTPS support to properly verify certificates.
I was thinking about:
- At the very least the documentation and README should reflect that this client does not perform any validation of the certificate.
- Possibly release a new version in which HTTPS support is disabled, and the change log explains that validation is absent. For the time being, allow developers to continue using this client with HTTPS support using some 'not secure' flag.
- Reach out to the community, and update vulnerability databases (which means we need to request an identifier). Especially considering how many projects/libraries (still) depend on 'em-http-request'.
At the very least the documentation and README should reflect that this client does not perform any validation of the certificate.
We use EM's default, which you can override by passing in your own options — e.g. see https://github.com/igrigorik/em-http-request/wiki/Issuing-Requests#available-connection--request-parameters and EM docs. Overall, +1 to providing more detailed guidance here. Would you be up for writing a wiki page on best practices, which can link to from the readme?
Possibly release a new version in which HTTPS support is disabled..
That would be a breaking change for a lot of clients. Unfortunately not something we can do in practice. Raising warnings, maybe.
Reach out to the community, and update vulnerability databases (which means we need to request an identifier). Especially considering how many projects/libraries (still) depend on 'em-http-request'.
The actual behavior is up to the upstream application that relies on em-http — see my comment on overriding EM defaults. As such, it's worth updating the docs and possibly adding some warnings, but a blanket CVE is not applicable here.
Here is how Faraday monkey patches em-http to verify ssl certs: https://github.com/lostisland/faraday/blob/a712938071bb0997410b5de4a4f2517d30fc4d3d/lib/faraday/adapter/em_http_ssl_patch.rb
Since both projects are MIT license, it would be nice if the Faraday patch could be added to em-http. I think most people expect an HTTP library to handle SSL cert verification. It is too bad that EM decided on an insecure default.
TLS verification (from Faraday) is now merged into em-http-request.