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 {