-
Notifications
You must be signed in to change notification settings - Fork 121
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
IDs are predictable, should clarify in README #78
Comments
@fta2012 just out of interest, if you can't know the timestamp portion can you ever crack the whole thing? |
You just need to search upwards from the last known timestamp. Also if it's a batch insert the next id is likely to be within the same millisecond as the others. |
I'm open to a PR, with a caveat. For such an attack to work:
Some older apps may be vulnerable to an ID guessing attack. |
The older PRNG is easier but the new one is similarly vulnerable. You need to switch to crypto.getRandomValues if you really want it to be unpredictable. |
Agreed. Open to a PR. |
If it's just replacing Math.random with the crypto version it should be a small change. I probably won't get to it so this is up for grabs for anyone who wants to contribute. |
The crypto API is not in the browser. |
I don't think that cuids being predictable is necessarily a bad thing. |
@MarkHerhold For some apps, it could be a security risk. |
FYI crypto is already available in most browsers. |
I'm open to a PR that uses the crypto API if it's available. Note that it is available in IE11 with the In Node, we'll have to use the Node crypto API to avoid this vulnerability... note that it is NOT compatible with universal JS, so we'll have to continue to use different builds for Node and Browsers. We could write a facade around the different APIs using a |
This should be fixed now. |
I was looking through this project since it's a dependency for something else I was checking out.
I noticed that you are using Math.random(). This is fine but if so you shouldn't claim that the ids are unguessable:
Since Math.random() is not cryptographically secure it's easy to predict the next values. I would remove the two security/secure paragraphs completely and add a warning instead.
EDIT: Just for fun I wrote a proof of concept here that can predict all CUIDs(excluding timestamp portion) after seeing four consecutive ids. It's for node v4/v5 only since they've updated their RNG algorithm but the approach for later versions is similar.
The text was updated successfully, but these errors were encountered: