Security vulnerability: missing SSL hostname validation

[igrigorik]

GitHub Security Lab (GHSL) Vulnerability Report: GHSL-2020-094

Summary

Missing hostname validation allows an attacker to perform a man in the middle attack against users of the library.

Product

em-http-request

Tested Version

1.1.5

Missing SSL/TLS certificate hostname validation

em-http-request uses the library eventmachine in an insecure way that allows an attacker to perform a man in the middle attack against users of the library.

Impact

An attacker can assume the identity of a trusted server and introduce malicious data in an otherwise trusted place.

Remediation

Implement hostname validation.

Resources

To trigger the vulnerability, a simple TLS enabled listening daemon is sufficient as described in the following snippets.

# Add a fake DNS entry to /etc/hosts.
$ echo "127.0.0.1 test.coinbase.com" | sudo tee -a /etc/hosts

# Create a certificate.
$ openssl req -x509 -newkey rsa:2048 -keyout key.pem -out cert.pem -days 365 -nodes

# Listen on port 443 with TLS enabled.
$ openssl s_server -key key.pem -cert cert.pem -accept 443
Using auto DH parameters
Using default temp ECDH parameters
ACCEPT
-----BEGIN SSL SESSION PARAMETERS-----
MGoCAQECAgMDBALAMAQABDDtsipRTslOpunNYATFLBo/Urf61flD0RYbyS0cpgwt
cH5uPqX4CKfCYg196MsV+HehBgIEXrqnkKIEAgIcIKQGBAQBAAAAphMEEXRlc3Qu
Y29pbmJhc2UuY29t
-----END SSL SESSION PARAMETERS-----
Shared ciphers:ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-GCM-SHA384:DHE-RSA-AES256-GCM-SHA384:ECDHE-ECDSA-CHACHA20-POLY1305:ECDHE-RSA-CHACHA20-POLY1305:DHE-RSA-CHACHA20-POLY1305:ECDHE-ECDSA-AES256-SHA384:ECDHE-RSA-AES256-SHA384:DHE-RSA-AES256-SHA256:DHE-RSA-CAMELLIA256-SHA256:ECDHE-ECDSA-AES256-SHA:ECDHE-RSA-AES256-SHA:DHE-RSA-AES256-SHA:DHE-RSA-CAMELLIA256-SHA:AES256-GCM-SHA384:AES256-SHA256:CAMELLIA256-SHA256:AES256-SHA:CAMELLIA256-SHA:ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-GCM-SHA256:DHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES128-SHA256:ECDHE-RSA-AES128-SHA256:DHE-RSA-AES128-SHA256:DHE-RSA-CAMELLIA128-SHA256:ECDHE-ECDSA-AES128-SHA:ECDHE-RSA-AES128-SHA:DHE-RSA-AES128-SHA:DHE-RSA-CAMELLIA128-SHA:AES128-GCM-SHA256:AES128-SHA256:CAMELLIA128-SHA256:AES128-SHA:CAMELLIA128-SHA
CIPHER is ECDHE-RSA-AES256-GCM-SHA384
Secure Renegotiation IS supported
GET / HTTP/1.1
Connection: close
Host: test.coinbase.com
User-Agent: EventMachine HttpClient
Accept-Encoding: gzip, compressed

Create a sample client with the following contents:

require 'rubygems'
require 'eventmachine'
require 'em-http'

urls = ARGV
if urls.size < 1
  puts "Usage: #{$0} <url> <url> <...>"
  exit
end

pending = urls.size

EM.run do
  urls.each do |url|
    http = EM::HttpRequest.new(url).get
    http.callback {
      puts "#{url}\n#{http.response_header.status} - #{http.response.length} bytes\n"
      puts http.response

      pending -= 1
      EM.stop if pending < 1
    }
    http.errback {
      puts "#{url}\n" + http.error

      pending -= 1
      EM.stop if pending < 1
    }
  end
end

Run the example client to see a connection being performed in the listening daemon initialized in the previous steps.

$ ruby em-http-request-client.rb "https://test.coinbase.com"

References

CWE-297: Improper Validation of Certificate with Host Mismatch

GitHub Security Advisories

We recommend you create a private GitHub Security Advisory for these findings. This also allows you to invite the GHSL team to collaborate and further discuss these findings in private before they are published.

Credit

This issue was discovered and reported by GHSL team member @agustingianni (Agustin Gianni).

[igrigorik]

• Looks like this is an issue with a number of EM-powered libraries, e.g. Security vulnerability: missing SSL hostname validation ConradIrwin/em-imap#25
• Faraday includes an em-http patch that implements validation: lostisland/faraday@63cf47c

Ideally, this should be addressed upstream in eventmachine core.. @sodabrew any thoughts on that? As an interim solution we could merge Faraday implementation into em-http. @agustingianni could you confirm that faraday implementation addresses the issue?

[agustingianni]

CVE-2020-13482 has been assigned to this issue.

Ilya, let me know when you are ready for me to test the Faraday changes and I will give it a sping.

[sodabrew]

I'd be glad to upstream that, thanks for calling it out!

[igrigorik]

@sodabrew great, I think that would be the best path forward and help resolve and prevent this same issue across a number of different libraries and services using EM.

@agustingianni in theory you should be able to test with..

require 'rubygems'
require 'eventmachine'
require 'em-http'

## copy local instance from https://github.com/lostisland/faraday/commit/63cf47c95b573539f047c729bd9ad67560bc83ff#diff-2ffbfc9e78f3db69aad38b56f7decad1
require 'em_http_ssl_patch.rb'  


urls = ARGV
if urls.size < 1
  puts "Usage: #{$0} <url> <url> <...>"
  exit
end

pending = urls.size

EM.run do
  urls.each do |url|
    http = EM::HttpRequest.new(url).get
    http.callback {
      puts "#{url}\n#{http.response_header.status} - #{http.response.length} bytes\n"
      puts http.response

      pending -= 1
      EM.stop if pending < 1
    }
    http.errback {
      puts "#{url}\n" + http.error

      pending -= 1
      EM.stop if pending < 1
    }
  end
end

[agustingianni]

Fantastic, I will give it a try and will get back at you. Thank you.

[agustingianni]

So I made some changes to actually trigger the validation code, basically I added ssl: {verify_peer: true} to the EM::HttpRequest constructor. The patch seems to work fine, I have tested it both with a fake certificate that matches the host and with a valid certificate (emited for another domain) but with the DNS pointing to the target host. Both tests were successfull.

I would advice to change the default value to verify_peer: true but I understand that this may break some clients of the library. If thats the case, updating the documentation reflecting this important detail is a good compromise I think.

Thank you all for addressing this issue, I think this will help em-imap too to solve the issue. I will try to contact the author and see what we can do.

This is the client I used for testing:

require 'rubygems'
require 'eventmachine'
require 'em-http'

## copy local instance from https://github.com/lostisland/faraday/commit/63cf47c95b573539f047c729bd9ad67560bc83ff#diff-2ffbfc9e78f3db69aad38b56f7decad1
require './em_http_ssl_patch.rb'  

urls = ARGV
if urls.size < 1
  puts "Usage: #{$0} <url> <url> <...>"
  exit
end

pending = urls.size

EM.run do
  urls.each do |url|
    http = EM::HttpRequest.new(url, ssl: {verify_peer: true}).get
    http.callback {
      puts "#{url}\n#{http.response_header.status} - #{http.response.length} bytes\n"
      puts http.response

      pending -= 1
      EM.stop if pending < 1
    }
    http.errback {
      puts "#{url}\n" + http.error

      pending -= 1
      EM.stop if pending < 1
    }
  end
end

[igrigorik]

@agustingianni PR live that should, I believe, address the issue. I added an explicit warning that will get logged to STDERR if verify_peer is not set to true. Can you do another sanity check and confirm that it's working as intended, before I merge?

@sodabrew alternatively, would it make sense to merge this or similar logic into EM core?

[agustingianni]

Hello, yes of course. I will test it now and report back.

[agustingianni]

LGTM! Thank you for fixing this!

[igrigorik]

1.1.6 should be live on rubygems now, with this merged.

Thanks again for the report and your help!