Skip to content

Commit

Permalink
Introduce configurable pbkdf2 iteration limit
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
jasnell committed Dec 21, 2023
1 parent f700948 commit 12bc98a
Show file tree
Hide file tree
Showing 12 changed files with 83 additions and 16 deletions.
12 changes: 6 additions & 6 deletions src/workerd/api/crypto-impl-pbkdf2.c++
Original file line number Diff line number Diff line change
Expand Up @@ -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<kj::byte>(length / 8);
OSSLCALL(PKCS5_PBKDF2_HMAC(keyData.asPtr().asChars().begin(), keyData.size(),
Expand Down
10 changes: 10 additions & 0 deletions src/workerd/api/crypto-impl.c++
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include <openssl/ec.h>
#include <openssl/rsa.h>
#include <openssl/crypto.h>
#include <workerd/jsg/setup.h>

namespace workerd::api {
namespace {
Expand Down Expand Up @@ -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
5 changes: 5 additions & 0 deletions src/workerd/api/crypto-impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,11 @@ class ZeroOnFree {
kj::Array<kj::byte> 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);
Expand Down
8 changes: 6 additions & 2 deletions src/workerd/api/node/crypto.h
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,12 @@ class CryptoImpl final: public jsg::Object {
kj::Array<kj::byte> info, uint32_t length);

// Pbkdf2
kj::Array<kj::byte> getPbkdf(kj::Array<kj::byte> password, kj::Array<kj::byte> salt,
uint32_t num_iterations, uint32_t keylen, kj::String name);
kj::Array<kj::byte> getPbkdf(jsg::Lock& js,
kj::Array<kj::byte> password,
kj::Array<kj::byte> salt,
uint32_t num_iterations,
uint32_t keylen,
kj::String name);

// Keys
struct KeyExportOptions {
Expand Down
13 changes: 8 additions & 5 deletions src/workerd/api/node/crypto_pbkdf2.c++
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,19 @@

namespace workerd::api::node {

kj::Array<kj::byte> CryptoImpl::getPbkdf(kj::Array<kj::byte> password,
kj::Array<kj::byte> salt, uint32_t num_iterations, uint32_t keylen, kj::String name) {
kj::Array<kj::byte> CryptoImpl::getPbkdf(
jsg::Lock& js,
kj::Array<kj::byte> password,
kj::Array<kj::byte> 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,
Expand Down
6 changes: 6 additions & 0 deletions src/workerd/io/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -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++"],
Expand Down
17 changes: 17 additions & 0 deletions src/workerd/io/limit-enforcer.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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<size_t> 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.
Expand Down
3 changes: 2 additions & 1 deletion src/workerd/io/worker-entrypoint.c++
Original file line number Diff line number Diff line change
Expand Up @@ -643,7 +643,8 @@ kj::Promise<WorkerInterface::CustomEvent::Result>
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();
}

Expand Down
10 changes: 10 additions & 0 deletions src/workerd/io/worker.c++
Original file line number Diff line number Diff line change
Expand Up @@ -978,6 +978,10 @@ Worker::Isolate::Isolate(kj::Own<Api> 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());

Expand Down Expand Up @@ -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<const Worker::Isolate*>(ptr);
}

bool Worker::Isolate::Impl::Lock::checkInWithLimitEnforcer(Worker::Isolate& isolate) {
shouldReportIsolateMetrics = true;
return limitEnforcer.exitJs(*lock);
Expand Down
3 changes: 3 additions & 0 deletions src/workerd/io/worker.h
Original file line number Diff line number Diff line change
Expand Up @@ -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; }
Expand Down
8 changes: 7 additions & 1 deletion src/workerd/server/server.c++
Original file line number Diff line number Diff line change
Expand Up @@ -2524,12 +2524,18 @@ kj::Own<Server::Service> 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<size_t> 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<IsolateObserver>();
auto limitEnforcer = kj::heap<NullIsolateLimitEnforcer>();
auto api = kj::heap<WorkerdApi>(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.
Expand Down
4 changes: 3 additions & 1 deletion src/workerd/tests/test-fixture.c++
Original file line number Diff line number Diff line change
Expand Up @@ -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<size_t> checkPbkdfIterations(jsg::Lock& lock, size_t iterations) const override {
return kj::none;
}
};

struct MockErrorReporter final: public Worker::ValidationErrorReporter {
Expand Down

0 comments on commit 12bc98a

Please sign in to comment.