Investigate use of Math.random for permission tokens

[dborkan]

As of #33 we are using Math.random to generate permission tokens. We need to see if this is secure enough, or if we should move to the crypto library.

[agallant]

Hey - sorry for not taking a look at this earlier. Here's my understanding of what the flow is:

• Alice generates a random token, gives it to Bob (via the actual Firebase provider channel)
• Bob uses this token to add Alice as a "friend"
• Alice presumably checks her friends against the token as needed

Is this correct? If so, I think the quality of the randomness is unlikely to be a significant issue - we're trusting the channel in either case.

Also, FWIW there's still movement on the related Firefox issue (https://bugzilla.mozilla.org/show_bug.cgi?id=842818#c60) - that is, we should hopefully get proper crypto in webworkers in the not-too-distant future.

[iislucas]

Generally, security advice we got from reviews was never use Math.random.
Useage ends up creeping into bad places in unexpected ways. But if our
crypto random requires message passing, we will probably want to be
thinking a little harder about this, and maybe makes more sense to use
Math.Random, but make sure to document each use.

On Fri, 6 Nov 2015 at 13:24 soycode [email protected] wrote:

Hey - sorry for not taking a look at this earlier. Here's my understanding
of what the flow is:

• Alice generates a random token, gives it to Bob (via the actual
  Firebase provider channel)
• Bob uses this token to add Alice as a "friend"
• Alice presumably checks her friends against the token as needed

Is this correct? If so, I think the quality of the randomness is unlikely
to be a significant issue - we're trusting the channel in either case.

Also, FWIW there's still movement on the related Firefox issue (
https://bugzilla.mozilla.org/show_bug.cgi?id=842818#c60) - that is, we
should hopefully get proper crypto in webworkers in the not-too-distant
future.

—
Reply to this email directly or view it on GitHub
#34 (comment)
.

[agallant]

The crypto random doesn't require message passing, just requires rather ugly filling in Firefox webworkers. I think it has stabilized a bit since @dborkan filed this issue so we could still give it a go. I agree that Math.random is basically always bad to depend on for anything security related, but from the flow as I understand it I don't think we are really depending on the randomness for security qualities here and just for uniqueness (the security is basically "trust Firebase").

That said, if I'm misunderstanding the flow (e.g. if we are expecting Alice/Bob to use a sidechannel for this token) then the randomness may be security sensitive, in which case yes we need to figure out using crypto.getRandomValues.

[iislucas]

Understood. Security team's argument wasn't about our previous usage of it
either, it was just about it being a bad code smell and, in the long run,
they argue that it's best to invest in making your random libraries
crypto-secure, and never using Math.random for the sake of your general
project/code health. I'm sure anytime you think about it you can fix it,
the problem is when people don't think about it, but copy/paste code from
other places. Hence their argument for never using Math.random, even when
its safe to do so. Same kind of thing as other good style/code health.

On Fri, 6 Nov 2015 at 14:14 soycode [email protected] wrote:

The crypto random doesn't require message passing, just requires rather
ugly filling in Firefox webworkers. I do think it has stabilized a bit
since @dborkan https://github.com/dborkan filed this issue so we could
still give it a go. I do agree that Math.random is basically always bad to
depend on for anything security related, but from the flow as I understand
it I don't think we are really depending on the randomness for security
qualities here and just for uniqueness (the security is basically "trust
Firebase").

That said, if I'm misunderstanding the flow (e.g. if we are expecting
Alice/Bob to use a sidechannel for this token) then the randomness may
be security sensitive, in which case yes we need to figure out using
crypto.getRandomValues.

—
Reply to this email directly or view it on GitHub
#34 (comment)
.

[agallant]

Fair point - if we want to make it a general code style/health guideline I'd support that. I'll see how simple/reliable our fill can be, and hopefully the Firefox issue will be resolved in the not-too-distant future as well.