workerd icon indicating copy to clipboard operation
workerd copied to clipboard

crypto: 100,000 iterations of PBKDF2 is insecure

Open sylvain101010 opened this issue 1 year ago • 13 comments

Hello,

workerd (and Cloudflare Workers) limits the number of PBKDF2 iterations to 100,000 to avoid DoS.

We can see the code here: https://github.com/cloudflare/workerd/blob/d1a3c89716591753406923e177b54da3475dd01c/src/workerd/api/crypto-impl-pbkdf2.c%2B%2B#L45C31-L45C37

While I understand the reason, especially in a multi-tenant environment like Cloudflare Workers, 100,000 iterations is far from secure, as per OWASP guidelines, which, in 2023, recommends at least 210,000 iterations for PBKDF2-SHA-512 and 600,000 iterations for PBKDF2-SHA-256, making any application hashing passwords from Cloudflare Workers insecure.

This is why I'm requesting an increase to at least 400,000 iterations.

sylvain101010 avatar Oct 25 '23 12:10 sylvain101010

@dom96

Warfields avatar Oct 25 '23 16:10 Warfields

I'll take a look at this later today.

fhanau avatar Oct 25 '23 16:10 fhanau

@fhanau ... one approach we might consider here is looking at the iteration count and if it's higher than a given threshold, move that outside of the isolate lock (and IoContext) and schedule it on the underlying kj event loop using a kj::evalLater or kj::evalSoon, etc, then pop back into the isolate lock with the result. That should effectively treat it as non-blocking background i/o as far as the isolate is concerned. We'd still want compute limits to be enforced but I'm thinking that it would allow us to get away with allowing a larger number of iterations.

jasnell avatar Oct 26 '23 19:10 jasnell

As discussed internally, moving the computation out of the isolate lock would add a lot of new complexity.

What we want to do here is permit a higher iteration count for people who have higher CPU limits. The original 100k limit was put in place when Workers had a 50ms total time limit -- that's about how much you could do in 50ms. Since our CPU time-limiting code cannot interrupt BoringSSL in the middle of running PBKDF, we have to limit the iterations upfront. But, these days most Workers have a 30s time limit so we should be able to increase the limit on PBKDF2 much higher for workers with such a higher limit.

Or if we can find an easy way to be able to interrupt the PBKDF2 loop when the CPU time limit is hit, then we could make the iterations unlimited...

kentonv avatar Oct 30 '23 18:10 kentonv

Just ran into this unfortunately after converting a Node/Prisma app into Workers/D1 structure. I wish I had known of this limitation or I wouldn't have started down this path.

I know these kinds of comments can be a bit insufferable (sorry), but any idea on a timeline here or a soft commitment on this being raised? It's a dealbreaker and I would love to know if I just need to stop going down the Worker-based infra path for my use case.

Thanks much.

sjc5 avatar Nov 19 '23 01:11 sjc5

@irvinebroque how should we prioritize this?

kentonv avatar Nov 20 '23 21:11 kentonv

We see a path to allow a higher iteration count, but this change won't happen this year.

irvinebroque avatar Nov 21 '23 08:11 irvinebroque

A change has been landed that makes the max iteration count configurable in workerd, with the default max iteration count removed in workerd. However, in the production environment the current limit will remain for at least some period of time.

jasnell avatar Jan 03 '24 04:01 jasnell

Do you have a rough timeline for this feature to land? I am interested in this as well.

dxvid-pts avatar Feb 02 '24 15:02 dxvid-pts

No ETA yet. The underlying mechanism that allows the limit to be configured has been landed but we still need to do more work on the production system to enable use. I should be able to get back to this soon once I clear a few higher priority items off my list.

jasnell avatar Feb 02 '24 15:02 jasnell

Any change since the last message?:)

nutriplan-app avatar Apr 18 '24 08:04 nutriplan-app

Not yet. @irvinebroque we should prioritize figuring out when/how to raise the limits in production based on the change that was landed.

jasnell avatar Apr 19 '24 00:04 jasnell

Any updates on this?

mrvillage avatar Jul 25 '24 19:07 mrvillage