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

Avoid introducing a new source of randomness #25

Closed
bakkot opened this issue Sep 24, 2019 · 15 comments
Closed

Avoid introducing a new source of randomness #25

bakkot opened this issue Sep 24, 2019 · 15 comments

Comments

@bakkot
Copy link

bakkot commented Sep 24, 2019

JavaScript currently has a relatively small number of places with nondeterministic behavior, such that making a (race-free) program completely deterministic involves patching only those places. Determinism an extremely useful property for debugging, etc, and is relied upon in practice - see for example the Mugshot paper by Mickens et al. As such I feel strongly that we should avoid introducing any new sources of randomness, especially in built-in modules (which are hard to patch).

cc @erights.

@littledan
Copy link
Member

littledan commented Sep 25, 2019

It's still an open question how built-in modules would be polyfilled, and whether this would be a module; I agree that this should be put in a place where it can be mocked. I think we should put this point aside.

About the underlying concern about reducing the sources of randomness: I don't quite understand the issue. For security, it should be good to have something that reliability gives a good source of randomness, to avoid relying on insecure sources like Math.random. The linked paper excluded Date and web APIs, so I don't understand how their approach would suffer by adding one more thing to the exclusion list.

It seems valuable to do UUID in TC39, so it's available across JavaScript environments, rather than a Web API, which may just come to a few. Would you prefer that it be the latter, though?

@bakkot
Copy link
Author

bakkot commented Sep 25, 2019

I don't quite understand the issue.

The issue is that there should be a small finite list of APIs that you need to patch, which requires being extremely careful when introducing new ones. Any time a new one is introduced we break anyone who is relying on the assumption that they have a complete list, and make it harder to correctly derive what is necessary to make a program deterministic from scratch (which happens anytime someone realizes they want to make their program deterministic and sets out to do so in an ad-hoc way).

We shouldn't introduce a new one just because it's marginally more convenient; we can instead design APIs such as this to get their source of randomness from some other place: for example, by being explicitly passed such a source, or from reading some function off the global when uuid() is invoked, or by importing some other module (assuming again that we have a good story for how to override built-in modules even from the perspective of other built-in modules, which is not quite the same as polyfilling them).

For security, it should be good to have something that reliability gives a good source of randomness, to avoid relying on insecure sources like Math.random.

I agree! But I don't think that should be this proposal.

It seems valuable to do UUID in TC39, so it's available across JavaScript environments, rather than a Web API, which may just come to a few. Would you prefer that it be the latter, though?

No. I would prefer that this proposal be in TC39 and that it not have a source of randomness baked in. I would think this even if it were a web proposal.

@littledan
Copy link
Member

No. I would prefer that this proposal be in TC39 and that it not have a source of randomness baked in. I would think this even if it were a web proposal.

Glad we agree on this being done in TC39.

Could you elaborate on what this API would look like without a source of randomness baked in, and how that randomness could be specified?

@bakkot
Copy link
Author

bakkot commented Sep 25, 2019

Could you elaborate on what this API would look like without a source of randomness baked in, and how that randomness could be specified?

I suggested a couple of possibilities in the previous comment:

for example, by being explicitly passed such a source, or from reading some function off the global when uuid() is invoked, or by importing some other module (assuming again that we have a good story for how to override built-in modules even from the perspective of other built-in modules, which is not quite the same as polyfilling them).

@bcoe
Copy link
Collaborator

bcoe commented Sep 26, 2019

@bakkot I wonder if we could figure out a compromise where there's a secure random number generator passed in by default, but the API exposes options for easily replacing it with a mock.

I'm thinking in terms of making the API as intuitive as possible by default.

@ljharb
Copy link
Member

ljharb commented Sep 26, 2019

One option there might be: a writable nonconfigurable static data property that is effectively looked up every time by the algorithm. Assigning to it changes the default; making it nonwritable prevents further changes.

@bakkot
Copy link
Author

bakkot commented Sep 26, 2019

@bcoe The idea is that you should not have to audit your entire codebase, including all dependencies, to make the program reproducible: it should suffice to just run a shim before everything else which patches all of the nondeterministic APIs (of which there should ideally be few). Requiring every user of the API to use it in a particular way does not accomplish this goal (unless that is the only way to use it).

I like @ljharb's idea. I also like the idea of introducing a global Math.urandom() which this API would look up and use as its source of randomness. With either of those, the API usage would be exactly the same as currently proposed, but it would remain possible to patch.

@broofa
Copy link
Collaborator

broofa commented Sep 26, 2019

Making the RNG a configurable global introduces possible security exploits though, doesn't it? E.g. if a hostile script injects a deterministic RNG to make ids predictable... :-/

@broofa
Copy link
Collaborator

broofa commented Sep 26, 2019

I also like the idea of introducing a global Math.urandom() which this API would look up and use as its source of randomness

Rather than adding new methods to the core ES API (which seems like a big ask), I'd rather see us just standardize on the existing API - crypto.getRandomValues(). We could just document that std:uuid implementations are expected to call into that API as their source of randomness, and leave it to users to shim that as needed.

@bakkot
Copy link
Author

bakkot commented Sep 26, 2019

I'm not sure what you can really do in a threat model which includes "an attacker can run arbitrary code on your page". The server can't generally trust data sent from client-side scripts anyway.

That said, I suppose you could allow passing in a PRNG and falling back to the global if it was not specified. That would be useful with the seeded randoms proposal.

@bakkot
Copy link
Author

bakkot commented Sep 26, 2019

standardize on the existing API - crypto.getRandomValues().

That is not a part of ECMAScript, whereas this proposal is. This proposal can't be written in terms of that API. (But I would want Math.urandom to work like that API, for simplicity.)

And since this proposal would be the only place the language guarantees cryptographically secure random values, I am certain that people who wanted to write code which would work across platforms would immediately start relying on it for that purpose anyway. I really think it would be best to first introduce a source of cryptographically secure random values.

@broofa
Copy link
Collaborator

broofa commented Sep 26, 2019

@bakkot I don't have a good answer here, honestly. Relying on globals opens the door for unintended behavior when people start futzing around with things unexpectedly. It's basically the same issue as prototype pollution. But passing RNG options around is problematic when you need to inject a custom RNG into a call being made deep in some 3rd party code.

Honestly, I'm of half a mind to just say this shouldn't be our problem. If someone wants a deterministic version of this module, they should shim it as part of the import/require process (however that would be done).

@bakkot
Copy link
Author

bakkot commented Sep 26, 2019

opens the door for unintended behavior when people start futzing around with things unexpectedly

I think that ship has sailed; that's largely just how the language works. In practice I do not expect it would be a problem: someone who is deliberately patching Math.urandom is presumably deliberately intending to affect how random numbers are generated on the page. Any userland polyfill for this API would need to rely on some strong source of random numbers anyway.

@sffc
Copy link

sffc commented Oct 3, 2019

If UUID goes into 262, then urandom also belongs in 262. In my opinion, you should add something like Math.urandom() or ArrayBuffer.getSecureRandomBytes() to the same proposal as UUID. Alternatively, propose this to the W3C to include in WebCrypto as something like window.crypto.UUIDv4().

@ctavan
Copy link
Collaborator

ctavan commented Nov 25, 2019

We have included Math.getRandomValues() in this proposal through #33. We will obviously still have to flesh out the details but I do hope that the main concern of this issue has been addressed:

As @sffc has summarized: If UUID goes into ECMA-262, a separate definition for a CSPRNG, that UUID will mandatorily have to rely upon, will also have to go into ECMA-262.

@bakkot @sffc please re-open if you think that your concern has not yet been addressed in an adequate way.

@ctavan ctavan closed this as completed Nov 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants