Fix up random sampling with brave-crypto random samplers.

[riastradh-brave]

fix #6944

Some of these changes change the distribution of probabilities to
the possible outcomes, where it seemed obvious that a uniform
distribution was intended but not previously attained.

Some of these changes also change the support, the set of possible
outcomes, where it made things simpler and was not obviously intended
one way or another.

Many of these are not necessarily bugs, but if deviations from
uniform are actually intended they should be clearly justified.

Full details:

• Change Math.floor(Math.random() * n) to crypto.random.uniform(n).

  • app/extensions/brave/content/scripts/adInsertion.js

  • js/about/newtab.js (with care to preserve test spying in newTabPageTest.js)

  • js/about/preferences.js (which used |0 rather than Math.floor)

  • test/about/bookmarksManagerTest.js

  • test/lib/brave.js

  • This was obviously intended to be a uniform distribution on
    {0,1,2,...,n-1}. It is nonuniform only because of IEEE 754
    binary64 floating-point approximation.

• Change Math.round(Math.random() * (n-1)) to crypto.random.uniform(n)
  when used as an index into an n-element array.

  • tools/lib/randomHostname.js, _chooseRandomLetter / ALPHABET

  • tools/lib/randomHostname.js, tld / TLDS

  • tools/lib/synopsisHelpers.js, publisher / PROTOCOL_PREFIXES

  • tools/lib/transactionHelpers.js, generateContribution: amount

    • (Adjusted to use the actual length of the array, not just the
      first four elements of an eight-element array.)

  • This gave half the probability to 0 and n - 1 that it gave to 1,
    2, 3, ..., n - 3, n - 2, a much bigger deviation from uniform than
    IEEE 754 binary64 floating-point approximations of the above.
    There was no obvious reason why this nonuniformity was desirable.

• Change Math.round(Math.random() * n) to crypto.random.uniform(n)
  when not used as an index into an array.

  • tools/lib/synopsisHelpers.js, numHosts

  • tools/lib/synopsisHelpers.js, numVisits

  • tools/lib/transactionHelpers.js, generateTransaction: count

  • tools/lib/synopsisHelpers.js, duration

  • This actually changes the support of the distribution to exclude
    n, but in the only places where this was used, it is not otherwise
    obvious that the caller intended to include n in the support. It
    could have been a typo for Math.floor(Math.random() * n) without
    adverse consequences in these contexts.

• Change Math.round(Math.random()) to crypto.random.uniform(2).

  • tools/lib/randomHostname.js, numParts

  • This was obviously intended to be a uniform distribution on {0,1},
    and actually it may even be uniform under Math.random as typically
    implemented, but writing crypto.random.uniform(2) is clearer to
    flip a coin and this way we just avoid Math.random() altogether.

• Obscure corner cases:

  • app/renderer/rendererTabEvents.js, nonce

    • This used Math.random().toString() to generate a string `that is
      unlikely to collide'. There are only 2^64 64-bit floating-point
      values, of which Math.random() returns a small subset. A
      collision is expected after only a few billion uniform random
      samples, which is far from impossible. I replaced it by a
      uniform random 256-bit quantity in hex, which will never collide
      even in the view of a conservative paranoid cryptographer unless
      there is a deeper bug breaking the PRNG.

  • tools/lib/randomHostname.js, partLen

    • The distribution originally used here puts higher probability on
      the minimum part length than everything else, and lower
      probability on the maximum part length than anything else. I
      see no reason why this should not just be uniformly distributed
      on part lengths {minLength, minLength + 1, ..., maxLength - 1,
      maxLength}, so I replaced it by minLength + uniform(maxLength -
      minLength + 1).

  • tools/lib/transactionHelpers.js, generateBallots: votesToCast

    • The previous expression, Math.min(Math.round(Math.random() *
      votesRemaining) + 1, votesRemaining), assigned half weight to 1
      and half-again weight to votesRemaining - 1 (as compared to all
      other numbers) for no obvious reason. I changed it to use 1, if
      only 1 vote is remaining, or 1 + uniform(votesRemaining - 1), if
      more than one vote is remaining.

P.S. It is a bit confusing that we have a module named brave-crypto
which is usually used as const crypto = require('brave-crypto')
while there is also a standard nodejs module called crypto which is
usually used as const crypto = require('crypto'). In one case these
are used in the same file, so I named ours braveCrypto.

Submitter Checklist:

• Submitted a ticket for my issue if one did not already exist.
• Used Github auto-closing keywords in the commit message.
• Added/updated tests for this change (for new code or code which already has tests).
• Ran git rebase -i to squash commits (if needed).
• Tagged reviewers and labelled the pull request as needed.
• Request a security/privacy review as needed. (Ask a Brave employee to help if you cannot access this document.)

Test Plan:

• npm run unittest
• npm run add-simulated-payment-history
• npm run add-simulated-synopsis-visits
• launch browser and browse around

Reviewer Checklist:

• Request a security/privacy review as needed if one was not already requested.

Tests

• Adequate test coverage exists to prevent regressions
• Tests should be independent and work correctly when run individually or as a suite ref
• New files have MPL2 license header

[bsclifton]

I tested some notifications with this and it worked great 😄 👍

[bsclifton]

master 7db1b6f
0.24.x f8877ed
0.23.x 003a4b7