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 {