[$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.

[perpetual-hydrofoil]

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.

[perpetual-hydrofoil]

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.

[perpetual-hydrofoil]

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.

[perpetual-hydrofoil]

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 )

[perpetual-hydrofoil]

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 )

[perpetual-hydrofoil]

Some additional context here:
Python - https://bugs.python.org/issue23857
Ruby - ruby/openssl#8 (comment)
Go - golang/go@fca335e

[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.

[perpetual-hydrofoil]

@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.

[perpetual-hydrofoil]

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

#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.

[perpetual-hydrofoil]

@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.

[perpetual-hydrofoil]

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.

[perpetual-hydrofoil]

@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.

[perpetual-hydrofoil]

Perhaps #782 (comment) should be in distros? Or should net not depend on distros?

[ghost]

@jamiesonbecker post a bounty using bountysource :)

[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/

[ringabout]

import std/httpclient

var client = newHttpClient()
echo client.getContent("https://nim-lang.org")

On win10 compiled with -d:ssl, it crashed with latest devel(commits after Nim 1.4.4)

Error: execution of an external program failed

---

(without .pem) gives
Error: unhandled exception: No SSL/TLS CA certificates found. [IOError]

[Araq]

@xflywind works for me on Windows and on Ubuntu. Which OS do you use?

[ringabout]

I use win10

nim -v
Nim Compiler Version 1.5.1 [Windows: amd64]
Compiled at 2021-02-16

Is it related to my Internet?

I follow the instructions from forum
https://forum.nim-lang.org/t/7551

[Araq]

Put the cacert.pem next to your .exe please and retry.

[dom96]

We can close this now, right?

[Araq]

Waiting for @xflywind's feedback.

[ringabout]

It works.

[timotheecour]

confirmed it also work on:

• osx
• windows 10 VM via parallels on osx, with the cacert.pem next to binary

• How aobout we offer an option to automatically curl certs from https://curl.se/ca/cacert.pem, cache it in nimcache (maybe with expiration date), and copy cacert.pem to outDir, which would make life easier? eg: nim r -d:ssl --sslCerts main
  (--sslCerts could later get options, if needed)

[alaviss]

How aobout we offer an option to automatically curl certs from https://curl.se/ca/cacert.pem, cache it in nimcache (maybe with expiration date), and copy cacert.pem to outDir, which would make life easier? eg: nim r -d:ssl --sslCerts main

No, instead we should implement auto-import for Windows' root certificate store. cacert.pem IMO is just a temporary fix.

[Araq]

No, instead we should implement auto-import for Windows' root certificate store

We considered that too and it's not without its problems:

• Program behaviour then depends on the Windows version.
• Where to cache the generated .pem file is not easy.
• When to update the cache is not easy.

[timotheecour]

instead of innovating here, can we follow what python does?
refs:

• https://docs.python.org/3/library/ssl.html#ssl.SSLContext.load_default_certs

On Windows it loads CA certs from the CA and ROOT system stores. ...

• https://docs.python.org/3/library/ssl.html#ssl.enum_certificates
• https://stackoverflow.com/questions/39356413/how-to-add-a-custom-ca-root-certificate-to-the-ca-store-used-by-pip-in-windows

for eg, on my windows VM (via osx parallels), I can use:

python -c "import ssl; print(ssl.get_default_verify_paths())"

[Araq]

I don't try to "innovate", I try to support SSL on Windows with minimal changes to the codebase as it's a bugfix.

[alaviss]

Program behaviour then depends on the Windows version.

Realistically we only have to support Windows 7 (EOL-ed) to Windows 10 (which most people should be on). There aren't that many differences between them in terms of certificate store APIs.

[Araq]

I'm not talking about API differences, I'm talking about the fact we don't know what is stored in the certificate store. This might lead to differences in behavior, for example, your web crawler can stop working after a Windows update. Having said that, I have no idea how often the entries in the store actually change.

[FedericoCeratto]

The root certificates are meant to receive updates over time. This is usually used to support new CAs, but it becomes critical if an existing CA is compromised and all clients need to drop it quickly. In such case, "breaking" application is the desired behavior.

[Araq]

Yet the breaking would be Windows specific. More importantly, it's still unclear how to cache the generated .pem file so the current solution is acceptable.

[alaviss]

Yet the breaking would be Windows specific.

Not really, if such a global drop happens, it's very likely that all OS (and applications on such OSes) will be impacted (and we don't provide a cacert.pem for anything non-Windows anyway).

[FedericoCeratto]

For example the debian ca-certificates changelog shows removals of obsoleted CAs but also blocking of untrusted ones.

The refreshing of CRLs is a decades-old caching problem. Perhaps doing an automated refresh every N days could be acceptable for Nim on Windows, if N is user-configurable.