From f321bb2060aa84f4470548fffb1b99b4a9cbbf18 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Mon, 4 Dec 2023 14:29:42 -0800 Subject: [PATCH] Introduce configurable pbkdf2 iteration limit Introduces the ability for workerd embedders to configure the max iteration limit for KDFs. Removes the limit for workerd by default. Keeps the current limit of 100,000 when not configured. --- src/workerd/api/crypto-impl-pbkdf2.c++ | 12 ++++++------ src/workerd/api/crypto-impl.c++ | 10 ++++++++++ src/workerd/api/crypto-impl.h | 5 +++++ src/workerd/api/node/crypto.h | 8 ++++++-- src/workerd/api/node/crypto_pbkdf2.c++ | 13 ++++++++----- src/workerd/io/BUILD.bazel | 6 ++++++ src/workerd/io/limit-enforcer.h | 17 +++++++++++++++++ src/workerd/io/worker-entrypoint.c++ | 3 ++- src/workerd/io/worker.c++ | 10 ++++++++++ src/workerd/io/worker.h | 3 +++ src/workerd/jsg/setup.h | 1 + src/workerd/server/server.c++ | 8 +++++++- src/workerd/server/workerd-api.c++ | 5 ++++- src/workerd/tests/test-fixture.c++ | 4 +++- 14 files changed, 88 insertions(+), 17 deletions(-) diff --git a/src/workerd/api/crypto-impl-pbkdf2.c++ b/src/workerd/api/crypto-impl-pbkdf2.c++ index 33f4ef658b8..06b55b3aa61 100644 --- a/src/workerd/api/crypto-impl-pbkdf2.c++ +++ b/src/workerd/api/crypto-impl-pbkdf2.c++ @@ -38,12 +38,12 @@ private: "PBKDF2 requires a positive iteration count (requested ", iterations, ")."); // Note: The user could DoS us by selecting a very high iteration count. Our dead man's switch - // would kick in, resulting in a process restart. We guard against this by limiting the maximum - // iteration count a user can select -- this is an intentional non-conformity. Another approach - // might be to fork OpenSSL's PKCS5_PBKDF2_HMAC() function and insert a check for - // v8::Isolate::IsExecutionTerminating() in the loop, but for now a hard cap seems wisest. - JSG_REQUIRE(iterations <= 100000, DOMNotSupportedError, - "PBKDF2 iteration counts above 100000 are not supported (requested ", iterations, ")."); + // would kick in, resulting in a process restart. We guard against this by limiting the + // maximum iteration count a user can select -- this is an intentional non-conformity. + // Another approach might be to fork OpenSSL's PKCS5_PBKDF2_HMAC() function and insert a + // check for v8::Isolate::IsExecutionTerminating() in the loop, but for now a hard cap seems + // wisest. + checkPbkdfLimits(js, iterations); auto output = kj::heapArray(length / 8); OSSLCALL(PKCS5_PBKDF2_HMAC(keyData.asPtr().asChars().begin(), keyData.size(), diff --git a/src/workerd/api/crypto-impl.c++ b/src/workerd/api/crypto-impl.c++ index 655f0940601..7df5615f73a 100644 --- a/src/workerd/api/crypto-impl.c++ +++ b/src/workerd/api/crypto-impl.c++ @@ -10,6 +10,7 @@ #include #include #include +#include namespace workerd::api { namespace { @@ -210,4 +211,13 @@ ZeroOnFree::~ZeroOnFree() noexcept(false) { OPENSSL_cleanse(inner.begin(), inner.size()); } +void checkPbkdfLimits(jsg::Lock& js, size_t iterations) { + auto& limits = Worker::Isolate::from(js).getLimitEnforcer(); + KJ_IF_SOME(max, limits.checkPbkdfIterations(js, iterations)) { + JSG_FAIL_REQUIRE(DOMNotSupportedError, + kj::str("Pbkdf2 failed: iteration counts above ", max ," are not supported (requested ", + iterations, ").")); + } +} + } // namespace workerd::api diff --git a/src/workerd/api/crypto-impl.h b/src/workerd/api/crypto-impl.h index bb779613aab..bbeefa375e2 100644 --- a/src/workerd/api/crypto-impl.h +++ b/src/workerd/api/crypto-impl.h @@ -339,6 +339,11 @@ class ZeroOnFree { kj::Array inner; }; +// Check that the requested number of iterations for a key-derivation function +// is acceptable. If the requested iterations is not acceptable, a JS error will +// be thrown. Otherwise the method will return normally. +void checkPbkdfLimits(jsg::Lock& js, size_t iterations); + } // namespace workerd::api KJ_DECLARE_NON_POLYMORPHIC(DH); diff --git a/src/workerd/api/node/crypto.h b/src/workerd/api/node/crypto.h index 3626d74851a..9e808d0d0b8 100644 --- a/src/workerd/api/node/crypto.h +++ b/src/workerd/api/node/crypto.h @@ -110,8 +110,12 @@ class CryptoImpl final: public jsg::Object { kj::Array info, uint32_t length); // Pbkdf2 - kj::Array getPbkdf(kj::Array password, kj::Array salt, - uint32_t num_iterations, uint32_t keylen, kj::String name); + kj::Array getPbkdf(jsg::Lock& js, + kj::Array password, + kj::Array salt, + uint32_t num_iterations, + uint32_t keylen, + kj::String name); // Keys struct KeyExportOptions { diff --git a/src/workerd/api/node/crypto_pbkdf2.c++ b/src/workerd/api/node/crypto_pbkdf2.c++ index 7475b4fd413..f0acd4ddb5b 100644 --- a/src/workerd/api/node/crypto_pbkdf2.c++ +++ b/src/workerd/api/node/crypto_pbkdf2.c++ @@ -10,16 +10,19 @@ namespace workerd::api::node { -kj::Array CryptoImpl::getPbkdf(kj::Array password, -kj::Array salt, uint32_t num_iterations, uint32_t keylen, kj::String name) { +kj::Array CryptoImpl::getPbkdf( + jsg::Lock& js, + kj::Array password, + kj::Array salt, + uint32_t num_iterations, + uint32_t keylen, + kj::String name) { // Should not be needed based on current memory limits, still good to have JSG_REQUIRE(password.size() <= INT32_MAX, RangeError, "Pbkdf2 failed: password is too large"); JSG_REQUIRE(salt.size() <= INT32_MAX, RangeError, "Pbkdf2 failed: salt is too large"); // Note: The user could DoS us by selecting a very high iteration count. As with the Web Crypto // API, intentionally limit the maximum iteration count. - JSG_REQUIRE(num_iterations <= 100000, DOMNotSupportedError, - "Pbkdf2 failed: iteration counts above 100000 are not supported (requested ", - num_iterations, ")."); + checkPbkdfLimits(js, num_iterations); const EVP_MD* digest = EVP_get_digestbyname(name.begin()); JSG_REQUIRE(digest != nullptr, TypeError, "Invalid Pbkdf2 digest: ", name, diff --git a/src/workerd/io/BUILD.bazel b/src/workerd/io/BUILD.bazel index a452183bf17..74a81f54d10 100644 --- a/src/workerd/io/BUILD.bazel +++ b/src/workerd/io/BUILD.bazel @@ -105,6 +105,12 @@ wd_cc_library( ], ) +wd_cc_library( + name = "limit-enforcer", + hdrs = ["limit-enforcer.h"], + visibility = ["//visibility:public"] +) + wd_cc_library( name = "worker-interface", srcs = ["worker-interface.c++"], diff --git a/src/workerd/io/limit-enforcer.h b/src/workerd/io/limit-enforcer.h index 6acf0ab91e9..a182cbb2923 100644 --- a/src/workerd/io/limit-enforcer.h +++ b/src/workerd/io/limit-enforcer.h @@ -12,6 +12,8 @@ namespace workerd { struct ActorCacheSharedLruOptions; class IoContext; +static constexpr size_t DEFAULT_MAX_PBKDF2_ITERATIONS = 100'000; + // Interface for an object that enforces resource limits on an Isolate level. // // See also LimitEnforcer, which enforces on a per-request level. @@ -57,6 +59,21 @@ class IsolateLimitEnforcer { // Report resource usage metrics to the given isolate metrics object. virtual void reportMetrics(IsolateObserver& isolateMetrics) const = 0; + + // Called when performing a cypto key derivation function (like pbkdf2) to determine if + // if the requested number of iterations is acceptable. If kj::none is returned, the + // number of iterations requested is acceptable. If a number is returned, the requested + // iterations is unacceptable and the return value specifies the maximum. + virtual kj::Maybe checkPbkdfIterations(jsg::Lock& js, size_t iterations) const { + // By default, historically we've limited this to 100,000 iterations max. We'll set + // that as the default for now. To set a default of no-limit, this would be changed + // to return kj::none. Note, this current default limit is *WAY* below the recommended + // minimum iterations for pbkdf2. + // TODO(maybe): We might consider emitting a warning if the number of iterations is + // too low to be safe. + if (iterations > DEFAULT_MAX_PBKDF2_ITERATIONS) return DEFAULT_MAX_PBKDF2_ITERATIONS; + return kj::none; + } }; // Abstract interface that enforces resource limits on a IoContext. diff --git a/src/workerd/io/worker-entrypoint.c++ b/src/workerd/io/worker-entrypoint.c++ index 1fe652e5578..db26ce6af31 100644 --- a/src/workerd/io/worker-entrypoint.c++ +++ b/src/workerd/io/worker-entrypoint.c++ @@ -643,7 +643,8 @@ kj::Promise void requestGc(const Worker& worker) { TRACE_EVENT("workerd", "Debug: requestGc()"); jsg::V8StackScope stackScope; - auto lock = worker.getIsolate().getApi().lock(stackScope); + auto& isolate = worker.getIsolate(); + auto lock = isolate.getApi().lock(stackScope); lock->requestGcForTesting(); } diff --git a/src/workerd/io/worker.c++ b/src/workerd/io/worker.c++ index fdc1000e8d1..f45723c9346 100644 --- a/src/workerd/io/worker.c++ +++ b/src/workerd/io/worker.c++ @@ -978,6 +978,10 @@ Worker::Isolate::Isolate(kj::Own apiParam, auto lock = api->lock(stackScope); auto features = api->getFeatureFlags(); + KJ_DASSERT(lock->v8Isolate->GetNumberOfDataSlots() >= 3); + KJ_DASSERT(lock->v8Isolate->GetData(3) == nullptr); + lock->v8Isolate->SetData(3, this); + lock->setCaptureThrowsAsRejections(features.getCaptureThrowsAsRejections()); lock->setCommonJsExportDefault(features.getExportCommonJsDefaultNamespace()); @@ -1251,6 +1255,12 @@ Worker::Script::~Script() noexcept(false) { impl = nullptr; } +const Worker::Isolate& Worker::Isolate::from(jsg::Lock& js) { + auto ptr = js.v8Isolate->GetData(3); + KJ_ASSERT(ptr != nullptr); + return *static_cast(ptr); +} + bool Worker::Isolate::Impl::Lock::checkInWithLimitEnforcer(Worker::Isolate& isolate) { shouldReportIsolateMetrics = true; return limitEnforcer.exitJs(*lock); diff --git a/src/workerd/io/worker.h b/src/workerd/io/worker.h index b6e732ce0bd..eb1639d37ec 100644 --- a/src/workerd/io/worker.h +++ b/src/workerd/io/worker.h @@ -264,6 +264,9 @@ class Worker::Isolate: public kj::AtomicRefcounted { ~Isolate() noexcept(false); KJ_DISALLOW_COPY_AND_MOVE(Isolate); + // Get the current Worker::Isolate from the current jsg::Lock + static const Isolate& from(jsg::Lock& js); + inline const IsolateObserver& getMetrics() const { return *metrics; } inline kj::StringPtr getId() const { return id; } diff --git a/src/workerd/jsg/setup.h b/src/workerd/jsg/setup.h index 9dce06f3641..b26e25f634b 100644 --- a/src/workerd/jsg/setup.h +++ b/src/workerd/jsg/setup.h @@ -206,6 +206,7 @@ class IsolateBase { explicit IsolateBase(const V8System& system, v8::Isolate::CreateParams&& createParams, kj::Own observer); + ~IsolateBase() noexcept(false); KJ_DISALLOW_COPY_AND_MOVE(IsolateBase); diff --git a/src/workerd/server/server.c++ b/src/workerd/server/server.c++ index 408200029c5..2af7f22d80f 100644 --- a/src/workerd/server/server.c++ +++ b/src/workerd/server/server.c++ @@ -2524,12 +2524,18 @@ kj::Own Server::makeWorker(kj::StringPtr name, config::Worker:: void completedRequest(kj::StringPtr id) const override {} bool exitJs(jsg::Lock& lock) const override { return false; } void reportMetrics(IsolateObserver& isolateMetrics) const override {} + kj::Maybe checkPbkdfIterations(jsg::Lock& lock, size_t iterations) const override { + // No limit on the number of iterations in workerd + return kj::none; + } }; auto observer = kj::atomicRefcounted(); auto limitEnforcer = kj::heap(); auto api = kj::heap(globalContext->v8System, - featureFlags.asReader(), *limitEnforcer, kj::atomicAddRef(*observer)); + featureFlags.asReader(), + *limitEnforcer, + kj::atomicAddRef(*observer)); auto inspectorPolicy = Worker::Isolate::InspectorPolicy::DISALLOW; if (inspectorOverride != kj::none) { // For workerd, if the inspector is enabled, it is always fully trusted. diff --git a/src/workerd/server/workerd-api.c++ b/src/workerd/server/workerd-api.c++ index 8c8cb80ca23..0077713efc8 100644 --- a/src/workerd/server/workerd-api.c++ +++ b/src/workerd/server/workerd-api.c++ @@ -125,7 +125,10 @@ struct WorkerdApi::Impl { IsolateLimitEnforcer& limitEnforcer, kj::Own observer) : features(capnp::clone(featuresParam)), - jsgIsolate(v8System, Configuration(*this), kj::mv(observer), limitEnforcer.getCreateParams()) {} + jsgIsolate(v8System, + Configuration(*this), + kj::mv(observer), + limitEnforcer.getCreateParams()) {} static v8::Local compileTextGlobal(JsgWorkerdIsolate::Lock& lock, capnp::Text::Reader reader) { diff --git a/src/workerd/tests/test-fixture.c++ b/src/workerd/tests/test-fixture.c++ index bf3d976856d..18e9ecba4c3 100644 --- a/src/workerd/tests/test-fixture.c++ +++ b/src/workerd/tests/test-fixture.c++ @@ -170,7 +170,9 @@ struct MockIsolateLimitEnforcer final: public IsolateLimitEnforcer { void completedRequest(kj::StringPtr id) const override {} bool exitJs(jsg::Lock& lock) const override { return false; } void reportMetrics(IsolateObserver& isolateMetrics) const override {} - + kj::Maybe checkPbkdfIterations(jsg::Lock& lock, size_t iterations) const override { + return kj::none; + } }; struct MockErrorReporter final: public Worker::ValidationErrorReporter {