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

Kernel: Use write(2) to /dev/(u)random as entropy source #24992

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

logkos
Copy link
Contributor

@logkos logkos commented Sep 3, 2024

This feature allows userspace to easily supply kernel random event pool with use of write(2) on random device nodes. This function is widely adopted by other UNIX-like systems and allows use of software events as an entropy source. Before we make sure on effects of repetitive or malicious input, writing to random device nodes is exclusive to root.

@github-actions github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label Sep 3, 2024
@supercomputer7
Copy link
Member

Is there an actual use case for this? I don't see any userspace program using this functionality.
Are you expecting users to run echo 'something random' > /dev/urandom?
What happens if malicious program tries to make /dev/urandom unusable by entering the same input over and over again? Or is this scenario not possible? we should always be cautious with user input to things like this device.

@logkos
Copy link
Contributor Author

logkos commented Sep 3, 2024

Linux and BSDs allow writes to /dev/(u)random to fill the entropy pool (via rng-tools or something fancy like radioactive decay and CCTV footage of lava lambs). Entropy should not be pulled down in case of repetitive input since events get hashed before being sent to RNG and other sources still can get into pool. One mitigation would be changing file perms for random devices

@supercomputer7
Copy link
Member

Linux and BSDs allow writes to /dev/(u)random to fill the entropy pool (via rng-tools or something fancy like radioactive decay and CCTV footage of lava lambs). Entropy should not be pulled down in case of repetitive input since events get hashed before being sent to RNG and other sources still can get into pool. One mitigation would be changing file perms for random devices

The fact that you add support for this in the kernel is not an issue.
I just don’t understand what program will use this - I ask this because without an example of this, this would be essentially dead code.

@logkos
Copy link
Contributor Author

logkos commented Sep 6, 2024

I just don’t understand what program will use this - I ask this because without an example of this, this would be essentially dead code.

If anyone wants to supply software generated/collected entropy or aims to port rng-tools, this feature allows that. Might be used for an userspace driver

@supercomputer7
Copy link
Member

I will probably give this another review in Thursday or Friday. Anyway, we should aim at adding some sort of userspace code to use this functionality. Maybe some port can be added for this? Or just our own code…

@supercomputer7
Copy link
Member

I still have mixed feelings on the topic. On one hand, I really want to see some userspace code doing something useful with this feature. On the other hand, it's really not a big change, so maybe we should just merge it as-is and think about this later on, if you say you will work on it in the future.

I will let the maintainers decide on this :)

@logkos logkos changed the title Kernel: use input to /dev/random as entropy source Kernel: use write(2) to /dev/(u)random as entropy source Sep 13, 2024
@logkos logkos changed the title Kernel: use write(2) to /dev/(u)random as entropy source Kernel: Use write(2) to /dev/(u)random as entropy source Sep 13, 2024
@BuggieBot
Copy link
Member

Hello!

One or more of the commit messages in this PR do not match the SerenityOS code submission policy, please check the lint_commits CI job for more details on which commits were flagged and why.
Please do not close this PR and open another, instead modify your commit message(s) with git commit --amend and force push those changes to update this PR.

a write syscall from userspace can now supply entropy pool.

This change stands in line with other UNIX-like systems.
We restrict write access to /dev/(u)random.
So only root can feed the kernel entropy pool.

This is a common practice for hardening.
Copy link
Member

@timschumi timschumi left a comment

Choose a reason for hiding this comment

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

+1 on the "I would like to see something use this" front.

@@ -124,7 +124,7 @@ static ErrorOr<void> prepare_bare_minimum_devtmpfs_directory_structure()
TRY(populate_device_node_with_symlink(DeviceNodeType::Character, "/dev/mem"sv, 0600, 1, 1));
TRY(populate_device_node_with_symlink(DeviceNodeType::Character, "/dev/null"sv, 0666, 1, 3));
TRY(populate_device_node_with_symlink(DeviceNodeType::Character, "/dev/full"sv, 0666, 1, 7));
TRY(populate_device_node_with_symlink(DeviceNodeType::Character, "/dev/random"sv, 0666, 1, 8));
TRY(populate_device_node_with_symlink(DeviceNodeType::Character, "/dev/random"sv, 0644, 1, 8));
Copy link
Member

Choose a reason for hiding this comment

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

Is there even such a thing as bad entropy? As far as I know/heard, you can't actually hurt the randomness of the RNG by mixing in bad data as long as it is mixed in properly.

Copy link
Contributor Author

@logkos logkos Sep 14, 2024

Choose a reason for hiding this comment

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

RHEL and FreeBSD restrict writes to /dev/random for the sake of hardening so we might adopt this as well

(Or maybe no, there is much more to harvest in user apps)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👀 pr-needs-review PR needs review from a maintainer or community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants