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

WASM compilation Random() always has the same seed #56609

Closed
kealist opened this issue Aug 30, 2024 · 13 comments
Closed

WASM compilation Random() always has the same seed #56609

kealist opened this issue Aug 30, 2024 · 13 comments
Assignees
Labels
area-dart2wasm Issues for the dart2wasm compiler. type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@kealist
Copy link

kealist commented Aug 30, 2024

  • Dart version and tooling diagnostic info (dart info)

Dart 3.6.0-197.0.dev windows_x64

  • Whether you are using Windows, macOS, or Linux (if applicable)

Windows

  • Whether you are using Chrome, Safari, Firefox, Edge (if applicable)

Chrome

Today we identified WASM builds were always generating the same UUIDs.

https://github.com/Rexios80/uuid_wasm_test

with an additional test

import 'package:test/test.dart';
import 'package:uuid/uuid.dart';
import 'dart:math';

void main() {
  test('random test', () {
    for (var i = 0; i < 5; i++) {
      print(Random().nextInt(600));
    }
  });
}

This will always return:

472
247
387
474
201

when compiled for dart2wasm but will be seeded randomly for dart2js.

I assume this could also be some sort of security problem as well?

@dart-github-bot
Copy link
Collaborator

Summary: The Random() function in Dart's WASM compilation always produces the same sequence of numbers, leading to deterministic UUID generation. This issue potentially poses a security risk as it could allow predictable behavior in applications relying on random values.

@julemand101
Copy link
Contributor

I assume this could also be some sort of security problem as well?

If anybody uses plain Random for security related features, then they already have massive security problems even without the seed being fixed. Random.secure is a thing for a reason. :)

Not saying this is not bad. :)

@julemand101
Copy link
Contributor

julemand101 commented Aug 30, 2024

Found the following code which could indicate the issue:

// Use a singleton Random object to get a new seed if no seed was passed.
static final _prng = new _Random._withState(_initialSeed());
static int _setupSeed(int seed) => mix64(seed);
// TODO: Make this actually random
static int _initialSeed() => 0xCAFEBABEDEADBEEF;
static int _nextSeed() {
// Trigger the PRNG once to change the internal state.
_prng._nextState();
return _prng._stateLow;
}

Kinda an important TODO that most likely should have an issue number attached to it to keep track of it?

@dart-github-bot
Copy link
Collaborator

Summary: The Random class in Dart's WASM compilation consistently generates the same sequence of numbers, leading to predictable UUIDs and potentially security vulnerabilities. This behavior is not observed in Dart2JS compilation.

@dart-github-bot dart-github-bot added area-dart2wasm Issues for the dart2wasm compiler. triage-automation See https://github.com/dart-lang/ecosystem/tree/main/pkgs/sdk_triage_bot. type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) labels Aug 30, 2024
@Rexios80
Copy link
Contributor

This is a catastrophic issue that should be fixed and cherry-picked ASAP. Anyone generating UUIDs in Flutter web WASM apps could be unintentionally generating UUID collisions and lose a lot of data.

@julemand101
Copy link
Contributor

This is a catastrophic issue that should be fixed and cherry-picked ASAP. Anyone generating UUIDs in Flutter web WASM apps could be unintentionally generating UUID collisions and lose a lot of data.

Just want to add here that you should never generate UUID using default Random() generator. Use Random.secure() for that. Reason being that the amount of unique seeds for Random() are limited to 32 bits.

So if you have lot of clients running, it is actually not that impossible to get two clients running with same seed just by accident.

(But UUID should also really be generated by backend...)

@Rexios80
Copy link
Contributor

Just want to add here that you should never generate UUID using default Random() generator. Use Random.secure() for that. Reason being that the amount of unique seeds for Random() are limited to 32 bits.

If that's the case then this is a catastrophic issue in the uuid package

Our app generates a lot of data while offline, so generating UUIDs server-side doesn't seem like a viable option

@julemand101
Copy link
Contributor

I agree that it would be better if the UUID package did focus on security first and performance can then be enabled with a unsecure flag documented what it does and the consequences of using it. Most projects does not require really fast generation of UUID's. And Random.secure are available on all platforms as far as I know.

But yes, I don't disagree that this issue should be fixed with a hotfix on stable. It is quite serious.

@igloo15
Copy link

igloo15 commented Aug 30, 2024

I agree that this is a huge issue. Even beyond the UUID and security/secure issues, the default behavior of Random() should be to have a different seed each time. Typically this is done by using time as the seed.

Utilizing the same seed for testing stuff is a valid use case but should not be the default behavior. That is the purpose of passing a specific seed you define in the constructor Random(123) in order to get a repeated set of values for testing.

This is unexpected default behavior and I expect most people had no idea this was the case. I knew Random() was not secure and could not always be random but I expected that it not being random was a like 1 in 100 chance not every time.

@lrhn lrhn removed the triage-automation See https://github.com/dart-lang/ecosystem/tree/main/pkgs/sdk_triage_bot. label Aug 30, 2024
@daegalus
Copy link

daegalus commented Aug 30, 2024

Hi, I have released a new version of UUID that makes Random.secure() the default for UUIDs.

Some back story: Random.secure() didn't always exist, and I had custom Crypto implementations in the past to do that, but were slow. And once it did exist, there were regressions with IE11 back in the day and people using DartAngular at the time were having issues. So I flipflopped between secure and non-secure Random a lot over the years.

To my knowledge, this is no longer an issue, so I have made it default again. the uuid library is 11+ years old, so there is a lot of legacy decisions and iteration.

@osa1 osa1 self-assigned this Sep 2, 2024
@julemand101
Copy link
Contributor

@osa1 Thanks for the quick fix of this issue. Have it been considered if this is something that should be cherry-picked to Stable and/or Beta?

@osa1
Copy link
Member

osa1 commented Sep 3, 2024

@julemand101 we just considered it, we will try to cherry-pick it into the next beta. I will create the cherry-pick request tomorrow, then it's up to the release team to decide, but I expect it to be merged as it's not a risky patch. (a tiny patch that can't break anyting)

@osa1
Copy link
Member

osa1 commented Sep 4, 2024

I think there's a new beta cutoff coming soon and the commit that fixed this issue will be included as well, so we don't need a cherry-pick. The commit should be merged to https://github.com/dart-lang/sdk/commits/beta/ in a few days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-dart2wasm Issues for the dart2wasm compiler. type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

No branches or pull requests

8 participants