SSL certificate verify GitHub action (#13697)

* Implement SSL/TLS certificate checking #782

* SSL: Add nimDisableCertificateValidation

Remove NIM_SSL_CERT_VALIDATION env var
tests/untestable/thttpclient_ssl.nim ran successfully on Linux with libssl 1.1.1d

* SSL: update integ test to skip flapping tests

* Revert .travis.yml change

* nimDisableCertificateValidation disable imports

Prevent loading symbols that are not defined on older SSL libs

* SSL: disable verification in net.nim

..when nimDisableCertificateValidation is set

* Update changelog

* Fix peername type

* Add define check for windows

* Disable test on windows

* Add exprimental GitHub action CI for SSL

* Test nimDisableCertificateValidation

.github/workflows/ci_ssl.yml

@@ -0,0 +1,91 @@
+name: Nim SSL CI
+on:
+  pull_request:
+    # Run only on changes on related files
+    paths:
+      - 'lib/pure/httpclient.nim'
+      - 'lib/pure/net.nim'
+      - 'lib/pure/ssl_certs.nim'
+      - 'lib/wrappers/openssl.nim'
+      - 'tests/stdlib/thttpclient_ssl*'
+      - 'tests/untestable/thttpclient_ssl*'
+
+jobs:
+  build:
+    strategy:
+      fail-fast: false
+      matrix:
+        os: [ubuntu-18.04, macos-10.15, windows-2019]
+        cpu: [amd64]
+    name: '${{ matrix.os }} (${{ matrix.cpu }})'
+    runs-on: ${{ matrix.os }}
+    steps:
+      - name: 'Checkout'
+        uses: actions/checkout@v2
+
+      - name: 'Checkout csources'
+        uses: actions/checkout@v2
+        with:
+          repository: nim-lang/csources
+          path: csources
+
+      - name: 'Install dependencies (Linux amd64)'
+        if: runner.os == 'Linux' && matrix.cpu == 'amd64'
+        run: |
+          sudo apt-fast update -qq
+          DEBIAN_FRONTEND='noninteractive' \
+            sudo apt-fast install --no-install-recommends -y libssl1.1
+      - name: 'Install dependencies (macOS)'
+        if: runner.os == 'macOS'
+        run: brew install make
+      - name: 'Install dependencies (Windows)'
+        if: runner.os == 'Windows'
+        shell: bash
+        run: |
+          mkdir dist
+          curl -L https://nim-lang.org/download/mingw64.7z -o dist/mingw64.7z
+          curl -L https://nim-lang.org/download/dlls.zip -o dist/dlls.zip
+          7z x dist/mingw64.7z -odist
+          7z x dist/dlls.zip -obin
+          echo "::add-path::${{ github.workspace }}/dist/mingw64/bin"
+
+      - name: 'Add build binaries to PATH'
+        shell: bash
+        run: echo "::add-path::${{ github.workspace }}/bin"
+
+      - name: 'Build 1-stage compiler from csources'
+        shell: bash
+        run: |
+          ncpu=
+          case '${{ runner.os }}' in
+          'Linux')
+            ncpu=$(nproc)
+            ;;
+          'macOS')
+            ncpu=$(sysctl -n hw.ncpu)
+            ;;
+          'Windows')
+            ncpu=$NUMBER_OF_PROCESSORS
+            ;;
+          esac
+          [[ -z "$ncpu" || $ncpu -le 0 ]] && ncpu=1
+
+          make -C csources -j $ncpu CC=gcc ucpu='${{ matrix.cpu }}'
+
+      - name: 'Build koch'
+        shell: bash
+        run: nim c koch
+
+      - name: 'Build the real compiler'
+        shell: bash
+        run: ./koch boot
+
+      - name: 'Run SSL nimDisableCertificateValidation integration tests'
+        shell: bash
+        run: nim c -d:nimDisableCertificateValidation -d:ssl -r -p:. tests/untestable/thttpclient_ssl_disabled.nim
+
+      - name: 'Run SSL certificate check integration tests'
+        # Not supported on Windows due to old openssl version
+        if: runner.os != 'Windows'
+        shell: bash
+        run: nim c -d:ssl -p:. --threads:on -r tests/untestable/thttpclient_ssl.nim

appveyor.yml

@@ -29,6 +29,8 @@ install:
   - cd ..
 
 build_script:
+  - openssl version
+  - openssl version -d
   - bin\nim c koch
   - koch runCI
 

changelog.md

@@ -144,6 +144,8 @@ echo f
   empty. This was required for intuitive behaviour of the strscans module
   (see bug #13605).
 - `std/oswalkdir` was buggy, it's now deprecated and reuses `std/os` procs
+- `net.newContext` now performs SSL Certificate checking on Linux and OSX.
+  Define `nimDisableCertificateValidation` to disable it globally.
 
 
 ## Language additions

lib/pure/httpclient.nim

@@ -119,6 +119,14 @@
 ## You will also have to compile with ``ssl`` defined like so:
 ## ``nim c -d:ssl ...``.
 ##
+## Certificate validation is NOT performed by default.
+## This will change in future.
+##
+## A set of directories and files from the `ssl_certs <ssl_certs.html>`_
+## module are scanned to locate CA certificates.
+##
+## See `newContext <net.html#newContext>`_ to tweak or disable certificate validation.
+##
 ## Timeouts
 ## ========
 ##
@@ -552,6 +560,7 @@ proc newHttpClient*(userAgent = defUserAgent, maxRedirects = 5,
   ## default is 5.
   ##
   ## ``sslContext`` specifies the SSL context to use for HTTPS requests.
+  ## See `SSL/TLS support <##ssl-tls-support>`_
   ##
   ## ``proxy`` specifies an HTTP proxy to use for this HTTP client's
   ## connections.

lib/pure/net.nim

@@ -67,6 +67,8 @@
 {.deadCodeElim: on.} # dce option deprecated
 import nativesockets, os, strutils, parseutils, times, sets, options,
   std/monotimes
+from ospaths import getEnv
+from ssl_certs import scanSSLCertificates
 export nativesockets.Port, nativesockets.`$`, nativesockets.`==`
 export Domain, SockType, Protocol
 
@@ -83,7 +85,7 @@ when defineSsl:
     SslError* = object of Exception
 
     SslCVerifyMode* = enum
-      CVerifyNone, CVerifyPeer
+      CVerifyNone, CVerifyPeer, CVerifyPeerUseEnvVars
 
     SslProtVersion* = enum
       protSSLv2, protSSLv3, protTLSv1, protSSLv23
@@ -517,17 +519,30 @@ when defineSsl:
         raiseSSLError("Verification of private key file failed.")
 
   proc newContext*(protVersion = protSSLv23, verifyMode = CVerifyPeer,
-      certFile = "", keyFile = "", cipherList = "ALL"): SslContext =
+                   certFile = "", keyFile = "", cipherList = "ALL",
+                   caDir = "", caFile = ""): SSLContext =
     ## Creates an SSL context.
     ##
     ## Protocol version specifies the protocol to use. SSLv2, SSLv3, TLSv1
     ## are available with the addition of ``protSSLv23`` which allows for
     ## compatibility with all of them.
     ##
-    ## There are currently only two options for verify mode;
-    ## one is ``CVerifyNone`` and with it certificates will not be verified
-    ## the other is ``CVerifyPeer`` and certificates will be verified for
-    ## it, ``CVerifyPeer`` is the safest choice.
+    ## There are three options for verify mode:
+    ## ``CVerifyNone``: certificates are not verified;
+    ## ``CVerifyPeer``: certificates are verified;
+    ## ``CVerifyPeerUseEnvVars``: certificates are verified and the optional
+    ## environment variables SSL_CERT_FILE and SSL_CERT_DIR are also used to
+    ## locate certificates
+    ##
+    ## The `nimDisableCertificateValidation` define overrides verifyMode and
+    ## disables certificate verification globally!
+    ##
+    ## CA certificates will be loaded, in the following order, from:
+    ##
+    ##  - caFile, caDir, parameters, if set
+    ##  - if `verifyMode` is set to ``CVerifyPeerUseEnvVars``,
+    ##    the SSL_CERT_FILE and SSL_CERT_DIR environment variables are used
+    ##  - a set of files and directories from the `ssl_certs <ssl_certs.html>`_ file.
     ##
     ## The last two parameters specify the certificate file path and the key file
     ## path, a server socket will most likely not work without these.
@@ -550,18 +565,41 @@ when defineSsl:
 
     if newCTX.SSL_CTX_set_cipher_list(cipherList) != 1:
       raiseSSLError()
-    case verifyMode
-    of CVerifyPeer:
-      newCTX.SSL_CTX_set_verify(SSL_VERIFY_PEER, nil)
-    of CVerifyNone:
+
+    when defined(nimDisableCertificateValidation) or defined(windows):
       newCTX.SSL_CTX_set_verify(SSL_VERIFY_NONE, nil)
+    else:
+      case verifyMode
+      of CVerifyPeer, CVerifyPeerUseEnvVars:
+        newCTX.SSL_CTX_set_verify(SSL_VERIFY_PEER, nil)
+      of CVerifyNone:
+        newCTX.SSL_CTX_set_verify(SSL_VERIFY_NONE, nil)
+
     if newCTX == nil:
       raiseSSLError()
 
     discard newCTX.SSLCTXSetMode(SSL_MODE_AUTO_RETRY)
     newCTX.loadCertificates(certFile, keyFile)
 
-    result = SslContext(context: newCTX, referencedData: initSet[int](),
+    when not defined(nimDisableCertificateValidation) and not defined(windows):
+      if verifyMode != CVerifyNone:
+        # Use the caDir and caFile parameters if set
+        if caDir != "" or caFile != "":
+          if newCTX.SSL_CTX_load_verify_locations(caDir, caFile) != 0:
+            raise newException(IOError, "Failed to load SSL/TLS CA certificate(s).")
+
+        else:
+          # Scan for certs in known locations. For CVerifyPeerUseEnvVars also scan
+          # the SSL_CERT_FILE and SSL_CERT_DIR env vars
+          var found = false
+          for fn in scanSSLCertificates():
+            if newCTX.SSL_CTX_load_verify_locations(fn, "") == 0:
+              found = true
+              break
+          if not found:
+            raise newException(IOError, "No SSL/TLS CA certificates found.")
+
+    result = SSLContext(context: newCTX, referencedData: initSet[int](),
       extraInternal: new(SslContextExtraInternal))
 
   proc getExtraInternal(ctx: SslContext): SslContextExtraInternal =
@@ -645,6 +683,7 @@ when defineSsl:
     ## This must be called on an unconnected socket; an SSL session will
     ## be started when the socket is connected.
     ##
+    ## FIXME:
     ## **Disclaimer**: This code is not well tested, may be very unsafe and
     ## prone to security vulnerabilities.
 
@@ -660,7 +699,25 @@ when defineSsl:
     if SSL_set_fd(socket.sslHandle, socket.fd) != 1:
       raiseSSLError()
 
-  proc wrapConnectedSocket*(ctx: SslContext, socket: Socket,
+  proc checkCertName(socket: Socket, hostname: string) =
+    ## Check if the certificate Subject Alternative Name (SAN) or Subject CommonName (CN) matches hostname.
+    ## Wildcards match only in the left-most label.
+    ## When name starts with a dot it will be matched by a certificate valid for any subdomain
+    when not defined(nimDisableCertificateValidation) and not defined(windows):
+      assert socket.isSSL
+      let certificate = socket.sslHandle.SSL_get_peer_certificate()
+      if certificate.isNil:
+        raiseSSLError("No SSL certificate found.")
+
+      const X509_CHECK_FLAG_ALWAYS_CHECK_SUBJECT = 0x1.cuint
+      const size = 1024
+      var peername: string = newString(size)
+      let match = certificate.X509_check_host(hostname.cstring, hostname.len.cint,
+        X509_CHECK_FLAG_ALWAYS_CHECK_SUBJECT, peername)
+      if match != 1:
+        raiseSSLError("SSL Certificate check failed.")
+
+  proc wrapConnectedSocket*(ctx: SSLContext, socket: Socket,
                             handshake: SslHandshakeType,
                             hostname: string = "") =
     ## Wraps a connected socket in an SSL context. This function effectively
@@ -671,6 +728,7 @@ when defineSsl:
     ## This should be called on a connected socket, and will perform
     ## an SSL handshake immediately.
     ##
+    ## FIXME:
     ## **Disclaimer**: This code is not well tested, may be very unsafe and
     ## prone to security vulnerabilities.
     wrapSocket(ctx, socket)
@@ -682,6 +740,9 @@ when defineSsl:
         discard SSL_set_tlsext_host_name(socket.sslHandle, hostname)
       let ret = SSL_connect(socket.sslHandle)
       socketError(socket, ret)
+      when not defined(nimDisableCertificateValidation) and not defined(windows):
+        if hostname.len > 0 and not isIpAddress(hostname):
+          socket.checkCertName(hostname)
     of handshakeAsServer:
       let ret = SSL_accept(socket.sslHandle)
       socketError(socket, ret)
@@ -1638,6 +1699,9 @@ proc connect*(socket: Socket, address: string,
 
       let ret = SSL_connect(socket.sslHandle)
       socketError(socket, ret)
+      when not defined(nimDisableCertificateValidation) and not defined(windows):
+        if not isIpAddress(address):
+          socket.checkCertName(address)
 
 proc connectAsync(socket: Socket, name: string, port = Port(0),
                   af: Domain = AF_INET) {.tags: [ReadIOEffect].} =

lib/pure/ssl_certs.nim

@@ -0,0 +1,97 @@
+#
+#
+#            Nim's Runtime Library
+#        (c) Copyright 2017 Nim contributors
+#
+#    See the file "copying.txt", included in this
+#    distribution, for details about the copyright.
+#
+## Scan for SSL/TLS CA certificates on disk
+## The default locations can be overridden using the SSL_CERT_FILE and
+## SSL_CERT_DIR environment variables.
+
+import os, strutils
+from ospaths import existsEnv, getEnv
+import strutils
+
+# SECURITY: this unnecessarily scans through dirs/files regardless of the
+# actual host OS/distribution. Hopefully all the paths are writeble only by
+# root.
+
+# FWIW look for files before scanning entire dirs.
+
+const certificate_paths = [
+    # Debian, Ubuntu, Arch: maintained by update-ca-certificates, SUSE, Gentoo
+    # NetBSD (security/mozilla-rootcerts)
+    # SLES10/SLES11, https://golang.org/issue/12139
+    "/etc/ssl/certs/ca-certificates.crt",
+    # OpenSUSE
+    "/etc/ssl/ca-bundle.pem",
+    # 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",
+    # CentOS/RHEL 7
+    "/etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem",
+    # OpenBSD, FreeBSD (optional symlink)
+    "/etc/ssl/cert.pem",
+    # Mac OS X
+    "/System/Library/OpenSSL/certs/cert.pem",
+    # Fedora/RHEL
+    "/etc/pki/tls/certs",
+    # Android
+    "/system/etc/security/cacerts",
+    # FreeBSD
+    "/usr/local/share/certs",
+    # NetBSD
+    "/etc/openssl/certs",
+]
+
+iterator scanSSLCertificates*(useEnvVars = false): string =
+  ## Scan for SSL/TLS CA certificates on disk.
+  ##
+  ## if `useEnvVars` is true, the SSL_CERT_FILE and SSL_CERT_DIR
+  ## environment variables can be used to override the certificate
+  ## directories to scan or specify a CA certificate file.
+  if existsEnv("SSL_CERT_FILE"):
+    yield getEnv("SSL_CERT_FILE")
+
+  elif existsEnv("SSL_CERT_DIR"):
+    let p = getEnv("SSL_CERT_DIR")
+    for fn in joinPath(p, "*").walkFiles():
+      yield fn
+
+  else:
+    for p in certificate_paths:
+      if p.endsWith(".pem") or p.endsWith(".crt"):
+        if existsFile(p):
+          yield p
+      elif existsDir(p):
+        for fn in joinPath(p, "*").walkFiles():
+          yield fn
+
+# Certificates management on windows
+# when defined(windows) or defined(nimdoc):
+#
+#   import openssl
+#
+#   type
+#     PCCertContext {.final, pure.} = pointer
+#     X509 {.final, pure.} = pointer
+#     CertStore {.final, pure.} = pointer
+#
+#   # OpenSSL cert store
+#
+#   {.push stdcall, dynlib: "kernel32", importc.}
+#
+#   proc CertOpenSystemStore*(hprov: pointer=nil, szSubsystemProtocol: cstring): CertStore
+#
+#   proc CertEnumCertificatesInStore*(hCertStore: CertStore, pPrevCertContext: PCCertContext): pointer
+#
+#   proc CertFreeCertificateContext*(pContext: PCCertContext): bool
+#
+#   proc CertCloseStore*(hCertStore:CertStore, flags:cint): bool
+#
+#   {.pop.}