Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crypto: 100,000 iterations of PBKDF2 is insecure #1346

Open
sylvain101010 opened this issue Oct 25, 2023 · 13 comments
Open

crypto: 100,000 iterations of PBKDF2 is insecure #1346

sylvain101010 opened this issue Oct 25, 2023 · 13 comments
Labels
feature request Request for Workers team to add a feature workers-runtime

Comments

@sylvain101010
Copy link

sylvain101010 commented Oct 25, 2023

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.

@Warfields
Copy link
Contributor

@dom96

@fhanau
Copy link
Collaborator

fhanau commented Oct 25, 2023

I'll take a look at this later today.

@jasnell
Copy link
Member

jasnell commented Oct 26, 2023

@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.

@kentonv
Copy link
Member

kentonv commented Oct 30, 2023

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...

@sjc5
Copy link

sjc5 commented Nov 19, 2023

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.

@kentonv
Copy link
Member

kentonv commented Nov 20, 2023

@irvinebroque how should we prioritize this?

@irvinebroque
Copy link
Collaborator

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

@jasnell
Copy link
Member

jasnell commented Jan 3, 2024

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 jasnell added the feature request Request for Workers team to add a feature label Jan 3, 2024
@dxvid-pts
Copy link

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

@jasnell
Copy link
Member

jasnell commented Feb 2, 2024

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.

@nutriplan-app
Copy link

Any change since the last message?:)

@jasnell
Copy link
Member

jasnell commented Apr 19, 2024

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

@mrvillage
Copy link

Any updates on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Request for Workers team to add a feature workers-runtime
Projects
None yet
Development

No branches or pull requests