Nim: [$150] httpclient SSL doesn't verify certificate chain

[zielmicha]

For example

import httpclient
getContent("https://www.pcwebshop.co.uk/").echo

works, but certificate of pcwebshop is self-signed (at last at the time of bug report).

[flaviut]

Here is a patch to fix this, as well as to disable insecure versions of SSL:

diff --git c/lib/pure/httpclient.nim w/lib/pure/httpclient.nim
index 2b16177..0dd868c 100644
--- c/lib/pure/httpclient.nim
+++ w/lib/pure/httpclient.nim
@@ -275,7 +275,7 @@ when not defined(ssl):
   type SSLContext = ref object
   let defaultSSLContext: SSLContext = nil
 else:
-  let defaultSSLContext = newContext(verifyMode = CVerifyNone)
+  let defaultSSLContext = newContext()

 proc newProxy*(url: string, auth = ""): Proxy =
   ## Constructs a new ``TProxy`` object.
diff --git c/lib/pure/net.nim w/lib/pure/net.nim
index 8afc6c5..27fef9b 100644
--- c/lib/pure/net.nim
+++ w/lib/pure/net.nim
@@ -167,7 +167,7 @@ when defined(ssl):
       if SSL_CTX_check_private_key(ctx) != 1:
         raiseSSLError("Verification of private key file failed.")

-  proc newContext*(protVersion = protSSLv23, verifyMode = CVerifyPeer,
+  proc newContext*(protVersion = protTLSv1, verifyMode = CVerifyPeer,
                    certFile = "", keyFile = ""): SSLContext =
     ## Creates an SSL context.
     ## 
diff --git c/lib/pure/smtp.nim w/lib/pure/smtp.nim
index 26f0c95..001b59a 100644
--- c/lib/pure/smtp.nim
+++ w/lib/pure/smtp.nim
@@ -82,7 +82,7 @@ when not defined(ssl):
   type PSSLContext = ref object
   let defaultSSLContext: PSSLContext = nil
 else:
-  let defaultSSLContext = newContext(verifyMode = CVerifyNone)
+  let defaultSSLContext = newContext()

 proc connect*(address: string, port = Port(25), 
               ssl = false, debug = false,
diff --git c/lib/pure/sockets.nim w/lib/pure/sockets.nim
index 99cdc00..32e38cb 100644
--- c/lib/pure/sockets.nim
+++ w/lib/pure/sockets.nim
@@ -284,8 +284,8 @@ when defined(ssl):
       if SSL_CTX_check_private_key(ctx) != 1:
         raiseSslError("Verification of private key file failed.")

-  proc newContext*(protVersion = protSSLv23, verifyMode = CVerifyPeer,
-                   certFile = "", keyFile = ""): SSLContext =
+  proc newContext*(protVersion = protTLSv1, verifyMode = CVerifyPeer,
+                   certFile = "", keyFile = "", certificateBundlePath: string): SSLContext =
     ## Creates an SSL context.
     ## 
     ## Protocol version specifies the protocol to use. SSLv2, SSLv3, TLSv1 are 
@@ -325,8 +325,12 @@ when defined(ssl):
     if newCTX == nil:
       raiseSslError()

+    # return code doesn't signify failure, ignore it
     discard newCTX.SSLCTXSetMode(SSL_MODE_AUTO_RETRY)
+
     newCTX.loadCertificates(certFile, keyFile)
+    if not newCTX.SSL_CTX_load_verify_locations("/home/user/tmp/ca-bundle.crt", nil):
+      raiseSslError("Unable to load certificate bundle")
     return SSLContext(newCTX)

   proc wrapSocket*(ctx: SSLContext, socket: Socket) =

The main issue now is determining how to distribute the certificate bundle.

There is no practical way to obtain it from the OS, so the best source is the converted Mozilla certificate bundle, which can be found at this url.

Any suggestions on how that should be done?

[Araq]

I think we should have a copy of this in $nim/dist and document it really well. We should support "copyFile" at compile-time then it can be copied over to where the exe will be put. Then it can be loaded via os.getAppDir() / "ca-bundle.crt". Maybe a bit too much magic though.

[dom96]

That's way too much magic. Please research how it's done in other programming languages.

[Araq]

Ok, here is how to do it: newContext only does SSL_CTX_load_verify_locations when certificateBundlePath is not "", so we delegate this problem to the programmer. httpclient then also gets some proc to overwrite the default SSL context. So people who think this sing and dance has anything to do with realworld security can deal with it but people like me who want stuff that works out of the box can happily ignore it.

[dom96]

That's fine. You already have the ability to specify the SSL context in httpclient I think.

[endragor]

How about adding insecure: bool parameter for newContext? Not validating certificates kills major point of SSL, so I don't think many people would be comfortable having that as the default. You could introduce -d:insecureSsl switch that affects the insecure value for default SSL context, setting it to true. In the languages/frameworks/tools I've seen, the default is secure, and you need to explicitly confirm you are fine with SSL checks not being performed.

[jamieson99]

Any MITM that is able to passively listen on unencrypted connections could almost as easily impersonate the signed server and just proxy if certificates are not verified by default. Python just went through this process as well for 2.6.x and switched to default:secure.

That definitely seems like the right way to go, as long as it's easy to disable checks. Is there currently any way to easily disable checks (ie curl --insecure, or forcibly enable them for that matter) or do you have to define a custom SSL context?

[dom96]

You have to define a custom SSL context currently. I'm happy to accept PRs to fix this, but I do like @Araq's proposed solution so if that could be implemented it would be ideal.

[jamieson99]

What about providing a compile-time warning for cert list, and then a compile-time option? (Are they available in libre/openssl when compiled in). It looks like Debian/Ubuntu/Alpine/etc use /etc/ssl/certs and /etc/ssl/cert.pem, and RHEL/CentOS/(suse?) use /etc/pki/; perhaps this belongs in distros instead for cross-platform run-times.

[jamieson99]

Looking at net.newContext, SSLVerifyPeer should probably be OR'ed with SSL_VERIFY_FAIL_IF_NO_PEER_CERT to prevent anonymous ciphers. This is ignored in client mode though.

[jamieson99]

Both verify peer and hostname check (in https) should be enabled by default. Virtually every other major language has gone in this direction, even when it meant breaking changes. In particular, Rust, Go, Python, and Ruby have all become secure by default. For example, see: https://nvd.nist.gov/vuln/detail/CVE-2015-1828

1. verify peer on the TLS connection itself (net)
2. verify fail if no peer cert (for anon ciphers in server mode, it's just ignored if client) (net)
3. verify hostname in HTTPS (and other protocols as appropriate, such as IMAPS, POPS, etc.) (httpclient, etc, but possibly calling a lower-level proc in net)

All of this should be easy to disable, and should probably check against the system certificates, which is how most languages do it. (Python fallback: "If all three are None, this function can choose to trust the system’s default CA certificates instead." https://docs.python.org/3/library/ssl.html )

[jamieson99]

In the list below, none of the listed locations are user-modifiable (that would be a risk). Of course, this is not maintaining a list of certificates or the nightmare of checking CRLs etc, but simply a list of where the OS's own list is. Grabbing this at runtime seems to make more sense than a compile flag (since you might want the same binary to run on both RHEL and Ubuntu). (If anyone knows where/if these are on Windows, that'd be great too.)

cert_paths = [
    # Debian, Ubuntu, Arch, SuSE
    # NetBSD (security/mozilla-rootcerts)
    "/etc/ssl/certs/",
    # Debian, Ubuntu, Arch: maintained by update-ca-certificates
    "/etc/ssl/certs/ca-certificates.crt",
    # Red Hat 5+, Fedora, Centos
    "/etc/pki/tls/certs/ca-bundle.crt",
    # Red Hat 4
    "/usr/share/ssl/certs/ca-bundle.crt",
    # FreeBSD (security/ca-root-nss package)
    "/usr/local/share/certs/ca-root-nss.crt",
    # FreeBSD (deprecated security/ca-root package, removed 2008)
    "/usr/local/share/certs/ca-root.crt",
    # FreeBSD (optional symlink)
    # OpenBSD
    "/etc/ssl/cert.pem",
    # Mac OS X
    "/System/Library/OpenSSL/certs/cert.pem",
]

(source: https://bugs.python.org/msg192601 )

[jamieson99]

Some additional context here:
Python - https://bugs.python.org/issue23857
Ruby - https://github.com/ruby/openssl/issues/8#issuecomment-226552969
Go - https://github.com/golang/go/commit/fca335e91a915b6aae536936a7694c4a2a007a60

[FedericoCeratto]

It's also good to be able to disable certificate verification at runtime (in order to run functional tests and have testbeds), and print out a big warning when doing so.

Not having this encourages developers to disable verification in their code and a "verify=false" can easily slip into release builds.

[jamieson99]

@FedericoCeratto agreed. A user can write an if statement that checks a user setting somewhere (config file, etc) for that. I don't know about the big warning, since it could affect stderr that the user might be controlling in a specific format, but it seems logical.

[jamieson99]

These are high priority bugs; in fact, they're actually language vulnerabilities in the stdlib, like the CVE linked to above for Ruby.

https://github.com/nim-lang/Nim/issues/784 perhaps could be merged into this one, or vice-versa, since they'll probably be fixed at the same time.

[FedericoCeratto]

@jamiesonbecker I suggest having a "verify: bool" flag on newHttpClient for fine-grained control and an environment variable that disables SSL verification globally. Only the latter is meant for testbeds and prints out the warning.

[jamieson99]

@FedericoCeratto Agreed with the verify: bool flag. Not sure about the env variable, but sounds reasonable to me if it wouldn't be set by accident. Something like TLS_VERIFY_CERTS=0 TLS_VERIFY_CN=0 ./myapp.

[jamieson99]

BOUNTIES

I'd like to pledge bounties for this work (no matter who does it) in PayPal, Stripe, Amazon or Ebay gift cards, etc. As a general rule, everywhere that a check can be done, it will be done by default, but checks should be easy to disable at the highest level possible (ie httpclient verify=false should make changes to ssl contexts etc). I anticipate that these patches would affect httpclient.nim and net.nim, and possibly openssl.nim.

1. $50 - ensure CA certificate chains are verified all the way through net (default:verify), but also ensure that they can be easily disabled using a convenient method argument when creating the socket etc.

2. $50 - ensure CA hostnames (ie Common Name) are verified in httpclient (by default) while also ensuring that verify: false on netHttpClient (etc) aborts these checks. CN checks also should be done by default in imaps, ftps, etc similar to how other languages do it. You should not have to create a custom SSL context just to turn on (or off) hostname verification - it should be passed through.

3. $50 - bonus for completing 1 and 2 by 31 Oct.

If you think you grok this and might dig into this a bit, please mention it in this thread, and if no one takes up the challenge, I'll start digging into it next week.

[jamieson99]

@Araq or @dom96 could you put bounty in the title? unless you're going for it ;) @flaviut I think your patch is a great start.

[jamieson99]

Perhaps https://github.com/nim-lang/Nim/issues/782#issuecomment-338093614 should be in distros? Or should net not depend on distros?

[Yardanico]

@jamiesonbecker post a bounty using bountysource :)

[jamieson99]

@Yardanico unfortunately BountySource has been suspended from Github logins with an API issue:

https://github.com/bountysource/core/issues/1147

[Yardanico]

@jamiesonbecker https://github.com/bountysource/core/issues/1147#issuecomment-337741285 ?

[dom96]

Just as an FYI: BountySource takes a 10% cut so using PayPal might be better.

[jamieson99]

Bounty is active! Ping me (first at firstlastname.com) if you're working on it.

[FedericoCeratto]

Work in progress... A high-level overview on how other languages/libraries handle badssl.com tests: https://github.com/FedericoCeratto/ssl-comparison

[jamieson99]

Awesome, thx Federico! Also, extended deadline until Nov 7 to win the part 3 bonus above. Keep in mind that there are really multiple places where verification should occur: at the TLS/ssl level for cert checks, at the https/imaps/etc level for CN or subject checks (and don't forget SNI certs!), etc. This will hopefully not result in a huge amount of extra code, but it might be be all over the place. Good luck and looking forward to seeing this come together!

[jamieson99]

@FedericoCeratto nailed it!

He's asked me to donate the bounty ($150) directly to the project, so I'll get that taken care of tomorrow morning.

EDIT: donation made a few days ago from paypal, can someone on the project team verify receipt?

Federico, you rock!

[me7]

Reading through this because I'm just watching @dom96 live stream on nim programming.
Seem like issue already solve. Should we close it?

[vegai]

No, https://github.com/nim-lang/Nim/pull/6664 is still open.

[narimiran]

Fixed by @FedericoCeratto in #13697.

[FedericoCeratto]

To allow a smoother adoption curve, certificate verification is not enabled by default in httpclient.
It's also not supported on Windows due to an old OpenSSL version.

[narimiran]

After some consultation with @FedericoCeratto, we'll keep this one open for now.

[timotheecour]

here's an updated test case that works exactly as intended (at least on osx, did not try windows):
nim r -d:ssl main # works
nim r main # works

import httpclient
import net

var client = newHttpClient()
discard client.getContent("http://google.com/")
when defined ssl:
  discard client.getContent("https://google.com/")
  doAssertRaises(SslError): discard client.getContent("https://www.pcwebshop.co.uk/")
else:
  doAssertRaises(HttpRequestError): discard client.getContent("https://google.com/")
  doAssertRaises(HttpRequestError): discard client.getContent("https://www.pcwebshop.co.uk/")

D20200424T153756

[aguspiza]

newContext parameters caDir and caFile were reversed when calling SSL_CTX_load_verify_locations upto 1.2.4 ( see #14815 ) fixed in devel

[viralpoetry]

Hi,
just to note that there are Google https endpoints that can be used when testing valid, expired & revoked certificates issued by GlobalSign:

https://good.gsr2demo.pki.goog/
https://expired.gsr2demo.pki.goog/
https://revoked.gsr2demo.pki.goog/