Skip to content

rand: add support for getrandom() #526

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

Merged
merged 2 commits into from
May 25, 2023
Merged

Conversation

bnoordhuis
Copy link
Contributor

glibc provides arc4random_buf() but musl does not and /dev/urandom is not always available.

glibc provides arc4random_buf() but musl does not and /dev/urandom is
not always available.
if (bytes_read == len)
return;
}
break;
#else
Copy link
Member

Choose a reason for hiding this comment

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

I think the only problem here is the failure condition breaks but that doesn't cause it to go to another subsystem. That said, according to the docs I don't think its actually possible to fail with 256 bytes (but might hang).

I'd be ok if for clarity you just changed the break on rv <= 0 to a continue as that ends up being the same actual behavior, but shows that its intentional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that I think about it, something like a seccomp filter could conceivably block the system call with e.g. ENOSYS. I can make it fall through to the ARES_RAND_FILE case to cover that. Good idea / bad idea?

Copy link
Member

Choose a reason for hiding this comment

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

Not terribly sure how realistic that would be to really occur and be expected to still do something reasonable. But if you want to fall back, that's fine ... but it should remember that and not ever try getrandom() again ... so that if ARES_RAND_FILE fails it doesn't fall back to getrandom() instead of falling through to the random of last resort of rc4 seeded with rand().

But do remember, this is really a best effort thing, it doesn't actually need to be cryptographically secure, just not trivially predictable (which previously it was basically trivially predictable). So cost/benefit analysis of doing too much logic here may not be worth it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point, I'll just s/break/continue then.

Not terribly sure how realistic that would be to really occur and be expected to still do something reasonable.

You would not believe how many bug reports we get over at libuv that boil down to overzealous seccomp filters 🤦

@bradh352 bradh352 merged commit 6995039 into c-ares:main May 25, 2023
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

Successfully merging this pull request may close these issues.

2 participants