Skip to content

net, openssl: support setting the minimum supported SSL/TLS version #14556

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
alaviss wants to merge 1 commit into from
Closed

net, openssl: support setting the minimum supported SSL/TLS version #14556

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions changelog.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,20 @@
- Added `genasts.genAst` that avoids the problems inherent with `quote do` and can
be used as a replacement.

- `net.newContext` drops support for setting the _exact_ version of SSL/TLS.
Copy link
Member

@FedericoCeratto FedericoCeratto Jan 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting only a minimum version is good but perhaps not flexible enough. As it happened in the past, a newer version can have a critical vulnerability that is not present in previous versions. Users might want to either disable that version or explicitly set a list of enabled versions.
(this is why SSL used flags v2 / v3 / v23)

Copy link
Collaborator Author

@alaviss alaviss Jan 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a breaking change, so I want to push it first.

That said, I'm not sure how to add a "max version" parameter, as that would mean we are artificially limiting the maximum version with whatever the stdlib have at the time (assuming a default parameter of type SslProtVersion). It might be better to provide "maximum version" as a separate proc, so it would be opt-in.

Copy link
Contributor

@shirleyquirk shirleyquirk Apr 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also like to be able to set the maximum version, turning off tls1.3 is the only way i know to force a client to only use the cipher you specify, e.g. for tls-psk.

as for how, it would be enough to include a SSL_CTX_set_max_proto_version proc in openssl, I dont think it needs to be any friendlier than that

This has been replaced by the ability to set the _minimum_ supported version.

The **default** minimum supported version is now `protTLS`. Details about
this constant can be found below.

- `net.SslProtVersion` now specify the minimum version to use with `newContext`.
`protSSLv2`, `protSSLv3` and `protSSLv23` has now been deprecated in favor of
explicit TLS versions.

A new constant `protTLS` is provided, which tracks the latest recommended TLS
version with reasonable security and compatibility with the Internet. This
constant is the replacement for the old `protSSLv23`.

## Language changes

- `nimscript` now handles `except Exception as e`.
Expand Down
77 changes: 60 additions & 17 deletions lib/pure/net.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ when useWinVersion:

when defineSsl:
import openssl
import dynlib
when not defined(nimDisableCertificateValidation):
from ssl_certs import scanSSLCertificates

Expand All @@ -115,7 +116,14 @@ when defineSsl:
CVerifyNone, CVerifyPeer, CVerifyPeerUseEnvVars

SslProtVersion* = enum
protSSLv2, protSSLv3, protTLSv1, protSSLv23
## Minimum supported SSL/TLS versions
protSSLv2 {.deprecated.}, ## **Deprecated:** SSLv2
protSSLv3 {.deprecated.}, ## **Deprecated:** SSLv3
protSSLv23 {.deprecated.}, ## **Deprecated:** Any SSL/TLS version, same as protTLSv1
protTLSv1, ## TLSv1
protTLSv1_1, ## TLSv1.1
protTLSv1_2, ## TLSv1.2
protTLSv1_3 ## TLSv1.3

SslContext* = ref object
context*: SslCtx
Expand All @@ -136,6 +144,10 @@ when defineSsl:
serverGetPskFunc: SslServerGetPskFunc
clientGetPskFunc: SslClientGetPskFunc

const protTLS* = protTLSv1_2
## The recommended minimum TLS version with adequate security and
## compatibility with the Internet.
Comment on lines +147 to +149
Copy link
Collaborator Author

@alaviss alaviss Jun 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd want this to be in ssl_config, but then it'd cause cyclic imports...


else:
type
SslContext* = ref object # TODO: Workaround #4797.
Expand Down Expand Up @@ -501,7 +513,7 @@ when defineSsl:
ERR_load_BIO_strings()
OpenSSL_add_all_algorithms()

proc raiseSSLError*(s = "") =
proc raiseSSLError*(s = "") {.noreturn.} =
## Raises a new SSL error.
if s != "":
raise newException(SslError, s)
Expand Down Expand Up @@ -561,14 +573,13 @@ when defineSsl:
if SSL_CTX_check_private_key(ctx) != 1:
raiseSSLError("Verification of private key file failed.")

proc newContext*(protVersion = protSSLv23, verifyMode = CVerifyPeer,
proc newContext*(minProtVersion = protTLS, verifyMode = CVerifyPeer,
certFile = "", keyFile = "", cipherList = CiphersIntermediate,
caDir = "", caFile = ""): SslContext =
## Creates an SSL context.
## Creates an SSL/TLS 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.
## The minimum supported SSL/TLS version can be specified with
## `minProtVersion`. See `SslProtVersion <#SslProtVersion>`_.
##
## There are three options for verify mode:
## `CVerifyNone`: certificates are not verified;
Expand All @@ -595,16 +606,48 @@ when defineSsl:
## or using ECDSA:
## - `openssl ecparam -out mykey.pem -name secp256k1 -genkey`
## - `openssl req -new -key mykey.pem -x509 -nodes -days 365 -out mycert.pem`
var newCTX: SslCtx
case protVersion
of protSSLv23:
newCTX = SSL_CTX_new(SSLv23_method()) # SSlv2,3 and TLS1 support.
of protSSLv2:
raiseSSLError("SSLv2 is no longer secure and has been deprecated, use protSSLv23")
of protSSLv3:
raiseSSLError("SSLv3 is no longer secure and has been deprecated, use protSSLv23")
of protTLSv1:
newCTX = SSL_CTX_new(TLSv1_method())
if minProtVersion in {protSSLv2, protSSLv3}:
raiseSSLError("SSLv2/3 is no longer secure and has been deprecated, use protTLS")

var newCTX = SSL_CTX_new(SSLv23_method())
let minVersion: cint =
case minProtVersion
of protTLSv1, protSSLv23:
Copy link
Member

@FedericoCeratto FedericoCeratto Jan 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

protSSLv23 is silently using TLS1 only?

Copy link
Collaborator Author

@alaviss alaviss Jan 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, for compatibility with the original, TLS1 is set as the lowest supported version. See https://github.com/nim-lang/Nim/pull/14556/files#diff-123d798b39756a95de82d119d2fe6661157ebb20407e2c394b7490e7588d2294R97 for the documentation.

TLS1_VERSION
of protTLSv1_1:
TLS1_1_VERSION
of protTLSv1_2:
TLS1_2_VERSION
of protTLSv1_3:
TLS1_3_VERSION
of protSSLv2, protSSLv3:
doAssert false, "unreachable!"
high(cint) # XXX: required since doAssert doesn't satisfy the noreturn check.

let useFallback: bool =
when not defined(openssl10) or defined(libressl):
try:
if getOpenSSLVersion() < 0x010100000:
true
elif newCTX.SSL_CTX_set_min_proto_version(minVersion) != 1:
raiseSSLError()
else:
false
except LibraryError:
# We are dealing with a super old LibreSSL
true
else:
true

if useFallback:
var flags: clong = SSL_OP_NO_SSLv2 or SSL_OP_NO_SSLv3
if minProtVersion > protTLSv1:
flags = flags or SSL_OP_NO_TLSv1
if minProtVersion > protTLSv1_1:
flags = flags or SSL_OP_NO_TLSv1_1
if minProtVersion > protTLSv1_2:
flags = flags or SSL_OP_NO_TLSv1_2
discard SSL_CTX_set_options(newCTX, flags)

if newCTX.SSL_CTX_set_cipher_list(cipherList) != 1:
raiseSSLError()
Expand Down
60 changes: 56 additions & 4 deletions lib/wrappers/openssl.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,13 +175,15 @@ const
SSL_CTRL_GET_SESS_CACHE_SIZE* = 43
SSL_CTRL_SET_SESS_CACHE_MODE* = 44
SSL_CTRL_GET_SESS_CACHE_MODE* = 45
SSL_CTRL_CLEAR_OPTIONS* = 47
SSL_CTRL_GET_MAX_CERT_LIST* = 50
SSL_CTRL_SET_MAX_CERT_LIST* = 51 #* Allow SSL_write(..., n) to return r with 0 < r < n (i.e. report success
# * when just a single record has been written): *
SSL_CTRL_SET_TLSEXT_SERVERNAME_CB = 53
SSL_CTRL_SET_TLSEXT_SERVERNAME_ARG = 54
SSL_CTRL_SET_TLSEXT_HOSTNAME = 55
SSL_CTRL_SET_ECDH_AUTO* = 94
SSL_CTRL_SET_MIN_PROTO_VERSION* = 123
TLSEXT_NAMETYPE_host_name* = 0
SSL_TLSEXT_ERR_OK* = 0
SSL_TLSEXT_ERR_ALERT_WARNING* = 1
Expand All @@ -198,8 +200,17 @@ const
SSL_OP_NO_SSLv2* = 0x01000000
SSL_OP_NO_SSLv3* = 0x02000000
SSL_OP_NO_TLSv1* = 0x04000000
SSL_OP_NO_TLSv1_1* = 0x08000000
SSL_OP_NO_TLSv1_1* = 0x10000000
SSL_OP_NO_TLSv1_2* = 0x08000000
SSL_OP_NO_SSL_MASK* = SSL_OP_NO_SSLv2 or SSL_OP_NO_SSLv3 or SSL_OP_NO_TLSv1 or
SSL_OP_NO_TLSv1_1 or SSL_OP_NO_TLSv1_2
SSL_OP_ALL* = 0x000FFFFF
SSL2_VERSION* = 0x0002
SSL3_VERSION* = 0x0300
TLS1_VERSION* = 0x0301
TLS1_1_VERSION* = 0x0302
TLS1_2_VERSION* = 0x0303
TLS1_3_VERSION* = 0x0304
SSL_VERIFY_NONE* = 0x00000000
SSL_VERIFY_PEER* = 0x00000001
SSL_ST_CONNECT* = 0x1000
Expand Down Expand Up @@ -257,6 +268,9 @@ proc TLSv1_method*(): PSSL_METHOD{.cdecl, dynlib: DLLSSLName, importc.}
# and support SSLv3, TLSv1, TLSv1.1 and TLSv1.2
# SSLv23_method(), SSLv23_server_method(), SSLv23_client_method() are removed in 1.1.0

proc SSL_CTX_ctrl*(ctx: SslCtx, cmd: cint, larg: clong, parg: pointer): clong{.
cdecl, dynlib: DLLSSLName, importc.}

when compileOption("dynlibOverride", "ssl") or defined(noOpenSSLHacks):
# Static linking

Expand Down Expand Up @@ -294,9 +308,23 @@ when compileOption("dynlibOverride", "ssl") or defined(noOpenSSLHacks):
proc SSL_state(ssl: SslPtr): cint {.cdecl, dynlib: DLLSSLName, importc.}
proc SSL_in_init*(ssl: SslPtr): cint {.inline.} =
SSl_state(ssl) and SSL_ST_INIT

proc SSL_CTX_set_options*(ctx: SslCtx, op: clong): clong {.inline.} =
SSL_CTX_ctrl(ctx, SSL_CTRL_OPTIONS, op, nil)

proc SSL_CTX_clear_options*(ctx: SslCtx, op: clong): clong {.inline.} =
SSL_CTX_ctrl(ctx, SSL_CTRL_OPTIONS, op, nil)

when defined(libressl):
proc SSL_CTX_set_min_proto_version*(ctx: SslCtx, version: cint): cint {.cdecl, dynlib: DLLSSLName, importc.}
else:
proc SSL_in_init*(ssl: SslPtr): cint {.cdecl, dynlib: DLLSSLName, importc.}
proc SSL_CTX_set_ciphersuites*(ctx: SslCtx, str: cstring): cint {.cdecl, dynlib: DLLSSLName, importc.}
proc SSL_CTX_set_options*(ctx: SslCtx, op: clong): clong {.cdecl, dynlib: DLLSSLName, importc.}
proc SSL_CTX_clear_options*(ctx: SslCtx, op: clong): clong {.cdecl, dynlib: DLLSSLName, importc.}

proc SSL_CTX_set_min_proto_version*(ctx: SslCtx, version: cint): cint =
cint SSL_CTX_ctrl(ctx, SSL_CTRL_SET_MIN_PROTO_VERSION, clong version, nil)

template OpenSSL_add_all_algorithms*() = discard

Expand Down Expand Up @@ -402,13 +430,40 @@ else:
let theProc = cast[proc() {.cdecl.}](sslSymNullable("OPENSSL_add_all_algorithms_conf"))
if not theProc.isNil: theProc()

proc SSL_CTX_set_options*(ctx: SslCtx, op: clong): clong =
let theProc {.global.} = cast[proc(ctx: SslCtx, op: clong): clong {.cdecl, gcsafe.}](sslSymNullable("SSL_CTX_set_options"))
if not theProc.isNil:
theProc(ctx, op)
else:
SSL_CTX_ctrl(ctx, SSL_CTRL_OPTIONS, op, nil)

proc SSL_CTX_clear_options*(ctx: SslCtx, op: clong): clong =
let theProc {.global.} = cast[proc(ctx: SslCtx, op: clong): clong {.cdecl, gcsafe.}](sslSymNullable("SSL_CTX_clear_options"))
if not theProc.isNil:
theProc(ctx, op)
else:
SSL_CTX_ctrl(ctx, SSL_CTRL_CLEAR_OPTIONS, op, nil)

proc getOpenSSLVersion*(): culong =
## Return OpenSSL version as unsigned long or 0 if not available
let theProc = cast[proc(): culong {.cdecl, gcsafe.}](utilSymNullable("OpenSSL_version_num", "SSLeay"))
result =
if theProc.isNil: 0.culong
else: theProc()

proc SSL_CTX_set_min_proto_version*(ctx: SslCtx, version: cint): cint =
## Set the minimum supported protocol version.
const MainProc = "SSL_CTX_set_min_proto_version"
let theProc {.global.} = cast[proc(ctx: SslCtx, version: cint): cint {.cdecl, gcsafe.}](sslSymNullable(MainProc))
Copy link
Member

@Araq Araq Apr 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These .global procs are bad style, prefer real globals instead. Also: You probably want .raises: [], locks: 0

Copy link
Collaborator Author

@alaviss alaviss May 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't that mean I'd have to produce an unique global name for every proc?

if not theProc.isNil:
theProc(ctx, version)
elif getOpenSSLVersion() == 0x020000000:
# For LibreSSL this is provided as a function, throw if it couldn't be
# found.
raiseInvalidLibrary(MainProc)
else:
cint SSL_CTX_ctrl(ctx, SSL_CTRL_SET_MIN_PROTO_VERSION, clong version, nil)

proc SSL_in_init*(ssl: SslPtr): cint =
# A compatibility wrapper for `SSL_in_init()` for OpenSSL 1.0, 1.1 and LibreSSL
const MainProc = "SSL_in_init"
Expand Down Expand Up @@ -552,9 +607,6 @@ proc CRYPTO_malloc_init*() =
when not useWinVersion and not defined(macosx) and not defined(android) and not defined(nimNoAllocForSSL):
CRYPTO_set_mem_functions(allocWrapper, reallocWrapper, deallocWrapper)

proc SSL_CTX_ctrl*(ctx: SslCtx, cmd: cint, larg: clong, parg: pointer): clong{.
cdecl, dynlib: DLLSSLName, importc.}

proc SSL_CTX_callback_ctrl(ctx: SslCtx, typ: cint, fp: PFunction): int{.
cdecl, dynlib: DLLSSLName, importc.}

Expand Down