Skip to content

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.
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