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

Make unpredictableSeed use getrandom (syscall) on Linux #10623

Merged
merged 1 commit into from
Jan 25, 2025

Conversation

0xEAB
Copy link
Member

@0xEAB 0xEAB commented Jan 19, 2025

This patch changes unpredictableSeed to use the getrandom syscall on Linux.

Currently, unpredictableSeed calls arc4random() on applicable BSD systems;
for everything else it executes RDRAND on InlineAsm_X86_Any-compatible targets or falls back to a homebrew solution.

@0xEAB 0xEAB added Severity:Enhancement OS:Linux Issues specific to Linux labels Jan 19, 2025
@0xEAB 0xEAB requested a review from LightBender January 19, 2025 00:55
@dlang-bot
Copy link
Contributor

Thanks for your pull request, @0xEAB!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#10623"

@rikkimax
Copy link
Contributor

That function needs a warning.

It is possible that the kernel may not have enough entropy stored to give a value.

You want to call it sparingly and only after the system has been booted fully.

@0xEAB
Copy link
Member Author

0xEAB commented Jan 19, 2025

and only after the system has been booted fully.

I don’t think this is accurate.
Quoting random(4):

When read during early boot time, /dev/urandom may return data prior to the entropy pool being initialized.

And also:

If this is of concern in your application, use getrandom(2) […] instead.

@rikkimax
Copy link
Contributor

Yes, during booting it may be empty. Once initialized the chance of it to be empty depends upon if it has been misused.

In any case, a warning is needed ;)

@0xEAB 0xEAB force-pushed the unpredictable-seed branch from 6fc083d to e1ace22 Compare January 19, 2025 02:10
@0xEAB 0xEAB requested a review from rikkimax January 19, 2025 02:10
@0xEAB 0xEAB force-pushed the unpredictable-seed branch from e1ace22 to 20b2d00 Compare January 19, 2025 02:12
@0xEAB
Copy link
Member Author

0xEAB commented Jan 19, 2025

@rikkimax
I’ve added further notes and a randomnes-quality warning. Please check whether you consider those sufficient.

@everyone
Please spell/grammar check those new paragraphs of technobabble.

@0xEAB 0xEAB force-pushed the unpredictable-seed branch from 20b2d00 to 83cb01f Compare January 19, 2025 02:17
@rikkimax
Copy link
Contributor

@rikkimax
I’ve added further notes and a randomnes-quality warning. Please check whether you consider those sufficient.

You have gone above and beyond what I was wanting!

Good job.

std/random.d Outdated
/**
A "good" seed for initializing random number engines. Initializing
with $(D_PARAM unpredictableSeed) makes engines generate different
random number sequences every run.

This function utilizes the system (CS-)PRNG where available and implemented
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use "(cryptographically secure) pseudo-random number generator" and introduce the acronym, and then use it throughout (rather than using the acronym first, and then spelling it out later).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to:

This function utilizes the system cryptographically-secure pseudo-random
number generator (CSPRNG)
or pseudo-random number generator (PRNG)
where available and implemented (currently arc4random on applicable BSD
systems or getrandom on Linux) to generate “high quality” pseudo-random
numbers – if possible.

@0xEAB
Copy link
Member Author

0xEAB commented Jan 24, 2025

You know what, I’ll take this chance that this hasn’t been merged yet to ask a question or propose a change here.

static assert(buffer.sizeof <= 256);

const status = (() @trusted => getrandom(&buffer, buffer.sizeof, 0))();
assert(status == buffer.sizeof);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Are we fine that this assert might not make it into user code because of -release?
  • Should we rather do if (status != buffer.sizeof) { assert(false); }?
  • Should we rather do if (status != buffer.sizeof) { return fallbackSeed(); }?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't worry about it.

You get what you get when you use it.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is enforce for that. Is this optional by the way? so I can set it by a flag , 0, this, 1 getrandom, 2, something else kinda thing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling enforce() would make this nothrow @nogc function, well, throw and @gc.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is just a way to generate a "good" seed for initializing random number engines – not a CSPRNG. (And if it were one, we definitely shouldn’t overengineer it. It could probably return an “optional”-type result then – but make it configurable – with options not applicable to all operating system –, rather nope. Also keep in mind, this thing has already been utilizing arc4random on applicable targets.)

@dlang-bot dlang-bot merged commit f2c49d3 into dlang:master Jan 25, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants