From d274c3babce1e39ec5112166aa0197e37fd3d65c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Thu, 11 May 2023 22:26:43 +0000 Subject: [PATCH] crypto,https,tls: disable engines if perms enabled When the experimental permission model is enabled, the running JavaScript code is subject to certain restrictions, all of which can be bypassed or even disabled by native code due to the nature of the permission model implementation. That is why Node.js native addons are disabled by default when the permission model is enabled. However, the built-in crypto, https, and tls modules still allow loading custom OpenSSL engines. Because OpenSSL engines can execute arbitrary (native) code while being loaded by Node.js, this has the same security implications as allowing native addons. In other words, allowing user code to load OpenSSL engines at runtime effectively enables bypassing any supposed security restrictions. This patch adds appropriate checks before attempting to dynamically load an OpenSSL engine that throw an error if the permission model is enabled. PR-URL: https://github.com/nodejs-private/node-private/pull/409 Reviewed-By: Rafael Gonzaga CVE-ID: CVE-2023-30586 --- doc/api/permissions.md | 2 ++ src/crypto/crypto_context.cc | 14 ++++++++++++++ src/crypto/crypto_util.cc | 7 +++++++ src/node_errors.h | 1 + src/permission/permission.h | 2 ++ 5 files changed, 26 insertions(+) diff --git a/doc/api/permissions.md b/doc/api/permissions.md index e29cbc7580e75d..f1d5e1b6e15ebf 100644 --- a/doc/api/permissions.md +++ b/doc/api/permissions.md @@ -544,6 +544,8 @@ Wildcards are supported too: There are constraints you need to know before using this system: * Native modules are restricted by default when using the Permission Model. +* OpenSSL engines currently cannot be requested at runtime when the Permission + Model is enabled, affecting the built-in crypto, https, and tls modules. * Relative paths are not supported through the CLI (`--allow-fs-*`). * The model does not inherit to a child node process. * The model does not inherit to a worker thread. diff --git a/src/crypto/crypto_context.cc b/src/crypto/crypto_context.cc index 36b59eb4d5dab3..3876adf7d72211 100644 --- a/src/crypto/crypto_context.cc +++ b/src/crypto/crypto_context.cc @@ -648,6 +648,13 @@ void SecureContext::SetEngineKey(const FunctionCallbackInfo& args) { CHECK_EQ(args.Length(), 2); + if (UNLIKELY(env->permission()->enabled())) { + return THROW_ERR_CRYPTO_CUSTOM_ENGINE_NOT_SUPPORTED( + env, + "Programmatic selection of OpenSSL engines is unsupported while the " + "experimental permission model is enabled"); + } + CryptoErrorStore errors; Utf8Value engine_id(env->isolate(), args[1]); EnginePointer engine = LoadEngineById(*engine_id, &errors); @@ -1102,6 +1109,13 @@ void SecureContext::SetClientCertEngine( // support multiple calls to SetClientCertEngine. CHECK(!sc->client_cert_engine_provided_); + if (UNLIKELY(env->permission()->enabled())) { + return THROW_ERR_CRYPTO_CUSTOM_ENGINE_NOT_SUPPORTED( + env, + "Programmatic selection of OpenSSL engines is unsupported while the " + "experimental permission model is enabled"); + } + CryptoErrorStore errors; const Utf8Value engine_id(env->isolate(), args[0]); EnginePointer engine = LoadEngineById(*engine_id, &errors); diff --git a/src/crypto/crypto_util.cc b/src/crypto/crypto_util.cc index 9b1d7acfc46a97..2cfacb27292236 100644 --- a/src/crypto/crypto_util.cc +++ b/src/crypto/crypto_util.cc @@ -638,6 +638,13 @@ void SetEngine(const FunctionCallbackInfo& args) { const node::Utf8Value engine_id(env->isolate(), args[0]); + if (UNLIKELY(env->permission()->enabled())) { + return THROW_ERR_CRYPTO_CUSTOM_ENGINE_NOT_SUPPORTED( + env, + "Programmatic selection of OpenSSL engines is unsupported while the " + "experimental permission model is enabled"); + } + args.GetReturnValue().Set(SetEngine(*engine_id, flags)); } #endif // !OPENSSL_NO_ENGINE diff --git a/src/node_errors.h b/src/node_errors.h index ddb87df20ef4af..36189615b9376d 100644 --- a/src/node_errors.h +++ b/src/node_errors.h @@ -37,6 +37,7 @@ void AppendExceptionLine(Environment* env, V(ERR_CLOSED_MESSAGE_PORT, Error) \ V(ERR_CONSTRUCT_CALL_REQUIRED, TypeError) \ V(ERR_CONSTRUCT_CALL_INVALID, TypeError) \ + V(ERR_CRYPTO_CUSTOM_ENGINE_NOT_SUPPORTED, Error) \ V(ERR_CRYPTO_INITIALIZATION_FAILED, Error) \ V(ERR_CRYPTO_INVALID_AUTH_TAG, TypeError) \ V(ERR_CRYPTO_INVALID_COUNTER, TypeError) \ diff --git a/src/permission/permission.h b/src/permission/permission.h index 61341ab29134f2..d294922154828c 100644 --- a/src/permission/permission.h +++ b/src/permission/permission.h @@ -39,6 +39,8 @@ class Permission { return is_scope_granted(permission, res); } + FORCE_INLINE bool enabled() const { return enabled_; } + static PermissionScope StringToPermission(const std::string& perm); static const char* PermissionToString(PermissionScope perm); static void ThrowAccessDenied(Environment* env,