From 7f5bd8971c61a0d09906c6079f26687cfcd6699e 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++ | 21 +++++++++++++++++++++ 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 | 8 ++++++++ src/workerd/jsg/jsg.c++ | 3 ++- src/workerd/jsg/jsg.h | 9 ++++++++- src/workerd/jsg/setup.c++ | 4 +++- src/workerd/jsg/setup.h | 7 ++++--- src/workerd/server/server.c++ | 8 +++++++- src/workerd/server/workerd-api.c++ | 10 ++++++++-- src/workerd/server/workerd-api.h | 1 + src/workerd/tests/test-fixture.c++ | 4 +++- 18 files changed, 123 insertions(+), 26 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..2aea7314dfd 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,24 @@ ZeroOnFree::~ZeroOnFree() noexcept(false) { OPENSSL_cleanse(inner.begin(), inner.size()); } +void checkPbkdfLimits(jsg::Lock& js, size_t iterations) { + KJ_IF_SOME(workerIsolate, Worker::Isolate::from(js)) { + auto& limits = workerIsolate.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, ").")); + } + } else { + // If the jsg::Lock embedder data has not been set then we're running in an + // environment that does not yet pass along the necessary information for the + // Worker::Isolate::from(...) below to work. Let's fall back on the old default + // limit here. + JSG_REQUIRE(iterations <= DEFAULT_MAX_PBKDF2_ITERATIONS, DOMNotSupportedError, + kj::str( + "Pbkdf2 failed: iteration counts above 100000 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..ee4716a8f87 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, &isolate); lock->requestGcForTesting(); } diff --git a/src/workerd/io/worker.c++ b/src/workerd/io/worker.c++ index fdc1000e8d1..5d865242290 100644 --- a/src/workerd/io/worker.c++ +++ b/src/workerd/io/worker.c++ @@ -503,7 +503,7 @@ struct Worker::Isolate::Impl { oldCurrentApi(currentApi), limitEnforcer(isolate.getLimitEnforcer()), consoleMode(isolate.consoleMode), - lock(isolate.api->lock(stackScope)) { + lock(isolate.api->lock(stackScope, &isolate)) { WarnAboutIsolateLockScope::maybeWarn(); // Increment the success count to expose forward progress to all threads. @@ -975,7 +975,7 @@ Worker::Isolate::Isolate(kj::Own apiParam, metrics->created(); // We just created our isolate, so we don't need to use Isolate::Impl::Lock (nor an async lock). jsg::V8StackScope stackScope; - auto lock = api->lock(stackScope); + auto lock = api->lock(stackScope, this); auto features = api->getFeatureFlags(); lock->setCaptureThrowsAsRejections(features.getCaptureThrowsAsRejections()); @@ -1251,6 +1251,12 @@ Worker::Script::~Script() noexcept(false) { impl = nullptr; } +kj::Maybe Worker::Isolate::from(jsg::Lock& js) { + auto embedderData = js.getEmbedderData(); + if (embedderData == nullptr) return kj::none; + return *static_cast(embedderData); +} + 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..9b834615627 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 kj::Maybe from(jsg::Lock& js); + inline const IsolateObserver& getMetrics() const { return *metrics; } inline kj::StringPtr getId() const { return id; } @@ -422,6 +425,11 @@ class Worker::Api { // that this doesn't have to allocate and so it's not possible to hold a lock while returning // to the event loop. + virtual kj::Own lock(jsg::V8StackScope& stackScope, const void* embedderData) const { + // In the default impl we do not pass along any embedder data to the jsg::Lock + return lock(stackScope); + } + // Get the FeatureFlags this isolate is configured with. Returns a Reader that is owned by the // Api. virtual CompatibilityFlags::Reader getFeatureFlags() const = 0; diff --git a/src/workerd/jsg/jsg.c++ b/src/workerd/jsg/jsg.c++ index 76f03d001f2..06af25c5113 100644 --- a/src/workerd/jsg/jsg.c++ +++ b/src/workerd/jsg/jsg.c++ @@ -115,9 +115,10 @@ void Data::moveFromTraced(Data& other, v8::TracedReference& otherTrace other.tracedHandle = kj::none; } -Lock::Lock(v8::Isolate* v8Isolate) +Lock::Lock(v8::Isolate* v8Isolate, const void* embedderData) : v8Isolate(v8Isolate), locker(v8Isolate), previousData(v8Isolate->GetData(2)), + embedderData(embedderData), warningsLogged(IsolateBase::from(v8Isolate).areWarningsLogged()) { if (previousData != nullptr) { // Hmm, there's already a current lock. It must be a recursive lock (i.e. a second lock taken diff --git a/src/workerd/jsg/jsg.h b/src/workerd/jsg/jsg.h index 9ab82011812..8e75c4c1a47 100644 --- a/src/workerd/jsg/jsg.h +++ b/src/workerd/jsg/jsg.h @@ -2231,18 +2231,25 @@ class Lock { void runMicrotasks(); void terminateExecution(); + inline const void* getEmbedderData() const { return embedderData; } + private: friend class IsolateBase; template friend class Isolate; - Lock(v8::Isolate* v8Isolate); + Lock(v8::Isolate* v8Isolate, const void* embedderData = nullptr); ~Lock() noexcept(false); v8::Locker locker; void* previousData; + // The embedderData is an additional void* that the embedder may use + // to associate an additional bit of context with the Lock. It is set + // via the constructor and accessed via getEmbedderData() + const void* embedderData = nullptr; + bool warningsLogged; }; diff --git a/src/workerd/jsg/setup.c++ b/src/workerd/jsg/setup.c++ index 77246064ae8..5337143c9ac 100644 --- a/src/workerd/jsg/setup.c++ +++ b/src/workerd/jsg/setup.c++ @@ -271,7 +271,9 @@ namespace { } } -IsolateBase::IsolateBase(const V8System& system, v8::Isolate::CreateParams&& createParams, kj::Own observer) +IsolateBase::IsolateBase(const V8System& system, + v8::Isolate::CreateParams&& createParams, + kj::Own observer) : system(system), ptr(newIsolate(kj::mv(createParams))), heapTracer(ptr), diff --git a/src/workerd/jsg/setup.h b/src/workerd/jsg/setup.h index 0d9a36c4af5..5c2ee3be36b 100644 --- a/src/workerd/jsg/setup.h +++ b/src/workerd/jsg/setup.h @@ -205,7 +205,8 @@ class IsolateBase { kj::TreeMap codeMap; explicit IsolateBase(const V8System& system, v8::Isolate::CreateParams&& createParams, - kj::Own observer); + kj::Own observer); + ~IsolateBase() noexcept(false); KJ_DISALLOW_COPY_AND_MOVE(IsolateBase); @@ -343,8 +344,8 @@ class Isolate: public IsolateBase { // `V8StackScope` must be provided to prove that one has been created on the stack before // taking a lock. Any GC'd pointers stored on the stack must be kept within this scope in // order for V8's stack-scanning GC to find them. - Lock(const Isolate& isolate, V8StackScope&) - : jsg::Lock(isolate.ptr), jsgIsolate(const_cast(isolate)) { + Lock(const Isolate& isolate, V8StackScope&, const void* embedderData = nullptr) + : jsg::Lock(isolate.ptr, embedderData), jsgIsolate(const_cast(isolate)) { jsgIsolate.clearDestructionQueue(); } 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 bbd340c85a0..7690924fd11 100644 --- a/src/workerd/server/workerd-api.c++ +++ b/src/workerd/server/workerd-api.c++ @@ -124,7 +124,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) { @@ -168,7 +171,10 @@ WorkerdApi::WorkerdApi(jsg::V8System& v8System, WorkerdApi::~WorkerdApi() noexcept(false) {} kj::Own WorkerdApi::lock(jsg::V8StackScope& stackScope) const { - return kj::heap(impl->jsgIsolate, stackScope); + return lock(stackScope, nullptr); +} +kj::Own WorkerdApi::lock(jsg::V8StackScope& stackScope, const void* embedderData) const { + return kj::heap(impl->jsgIsolate, stackScope, embedderData); } CompatibilityFlags::Reader WorkerdApi::getFeatureFlags() const { return *impl->features; diff --git a/src/workerd/server/workerd-api.h b/src/workerd/server/workerd-api.h index 97f17566048..9e1dd6dd4fd 100644 --- a/src/workerd/server/workerd-api.h +++ b/src/workerd/server/workerd-api.h @@ -22,6 +22,7 @@ class WorkerdApi final: public Worker::Api { static const WorkerdApi& from(const Worker::Api&); kj::Own lock(jsg::V8StackScope& stackScope) const override; + kj::Own lock(jsg::V8StackScope& stackScope, const void* embedderData) const override; CompatibilityFlags::Reader getFeatureFlags() const override; jsg::JsContext newContext(jsg::Lock& lock) const override; jsg::Dict unwrapExports( 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 {