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

Use ioctl RNDADDENTROPY for writing back random seed #4513

Closed
wants to merge 2 commits into
base: master
from

Conversation

4 participants
@bitstreamout
Copy link
Contributor

bitstreamout commented Oct 28, 2016

as this also increases the entropy bit count. Compare with linux/drivers/char/random.c

@keszybz
Copy link
Member

keszybz left a comment

Apart from the issues listed, I'd really like to see a short high-level description (in the commit message), what is happening here ("RNDADDENTROPY ioctl is used to feed ... to the kernel ..."). Basically where the entropy is coming from and where it is going. Also maybe add a link to random(4) in a comment.

@@ -66,11 +94,13 @@ int main(int argc, char *argv[]) {
if (buf_size <= POOL_SIZE_MIN)
buf_size = POOL_SIZE_MIN;

buf = malloc(buf_size);
if (!buf) {
entropy = (struct rand_pool_info*) malloc(sizeof(struct rand_pool_info) + buf_size);

This comment has been minimized.

@keszybz

keszybz Oct 31, 2016

Member

This cast is not needed (malloc returns void *).

r = log_oom();
goto finish;
}
entropy->buf_size = (typeof(entropy->buf_size)) buf_size;

This comment has been minimized.

@keszybz

keszybz Oct 31, 2016

Member

Please don't. entropy->buf_size is an int. Let's not try to hide this.
Removing the cast altogether doesn't result in a warning here, so it should just go away.

@@ -113,7 +143,7 @@ int main(int argc, char *argv[]) {
}
}

k = loop_read(seed_fd, buf, buf_size, false);
k = loop_read(seed_fd, (void*)entropy, buf_size, false);

This comment has been minimized.

@keszybz

keszybz Oct 31, 2016

Member

Likewise, casting to void* is not useful.

@@ -166,7 +196,7 @@ int main(int argc, char *argv[]) {
goto finish;
}

r = loop_write(seed_fd, buf, (size_t) k, false);
r = loop_write(seed_fd, (void*)entropy, (size_t) k, false);

This comment has been minimized.

@keszybz

keszybz Oct 31, 2016

Member

Ditto. You can remove (size_t) cast too.


len = loop_read(fd, (*entropy)->buf, (*entropy)->buf_size, false);
if (len < 0)
return -len;

This comment has been minimized.

@keszybz

keszybz Oct 31, 2016

Member

return len;

@@ -33,9 +35,35 @@

#define POOL_SIZE_MIN 512

static ssize_t get_random(int fd, struct rand_pool_info **entropy) {
ssize_t len;
int ret;

This comment has been minimized.

@keszybz

keszybz Oct 31, 2016

Member

int r is customary in systemd (ret is used for output parameters).

@poettering

This comment has been minimized.

Copy link
Member

poettering commented Oct 31, 2016

Hmm, I am really not convinced adding this is a good idea, see the discussions on #4271. There's really no reason to believe that the random seed stored on disk actually should be adding entropy, as it is systematically incompatible with booting systems from identical images. Consider 100 systems booting up from one and the same image, which ships the same random-seed file (since it is the same image after all), then after bootup there's a good chance that most of them came to the same pool state and think there was actually entropy in it, even though there really isn't as there a ton other systems with the very same pool state...

I don't see how this could be fixed. There's no way to figure out if you are booting from a freshly duplicated image (where you better shouldn't consider the seed to contain entropy) or from a system with a single linear line of clean boots (where it is OK to assume that the seed contains entropy).

I am thinking that this is not worth adding at all, since it creates a fake sense of entropy in a good part of systems (as pre-built images are kinda common these days – think containers –, and individually put together ones kinda old-school). What we maybe could offer is doing this optionally, via a kernel command line switch, but it really needs to be opt-in, not opt-out, and documentation must make clear it's incompatible with a pre-built image style setup.

@keszybz

This comment has been minimized.

Copy link
Member

keszybz commented Oct 31, 2016

Consider 100 systems booting up from one and the same image, which ships the same random-seed file

I believe that if you're making an image for duplication, things like random-seed, ssh keys, etc, should be nuked from the image.

@poettering

This comment has been minimized.

Copy link
Member

poettering commented Nov 2, 2016

I believe that if you're making an image for duplication, things like random-seed, ssh keys, etc, should be nuked from the image.

Sure, but who actually does that? And if they do, who'd think about random-seed?

I mean, the difference between the random-seed on one hand and ssh keys or /etc/machine-id on the other in this regard is that the random seed means weak security, while the machine-id/ssh-key being duplicate mostly just means weird behaviour...

The random seed stuff is particularly relevant in virtualized environments, that have no proper entropy from anywhere else. Physical bare-metal systems do not need it at all, as they generally have hardware RNGs these days, and thus plenty entropy any time they want. However, virtualized environments in particular work with system images that are duplicated across nodes, and thus we need to be extra careful with that.

Also, I am note entirely sure that flushing out the random seed when duplicating the image is really the best idea, since it means that not just the many instances of one specific image share the same entropy pool at runtime, but in fact all instances of all images share it... I think there's some value in shipping a random seed in an image hence...

Merging this change as is at the minimum compat breakage: previously we didn't require people to flush out the random seed when duplicating images, with this change we suddenly would do. I am not sure I like such a compat breakage.

hence, I am still sure: this should be opt-in via a kernel cmdline option, and not the default. And the relationship to duplicated system images must be clearly documented.

@keszybz

This comment has been minimized.

Copy link
Member

keszybz commented Nov 2, 2016

Sure, but who actually does that? And if they do, who'd think about random-seed?

I think people generally know about this, for example the Fedora kickstarts remove machine-id:
https://pagure.io/fedora-kickstarts/blob/master/f/fedora-docker-base.ks#_91

But you're right, nobody seems to care about random-seed.

But maybe we could piggy-back on the removal of random-seed, and do the following: when storing the random-seed, also store the hostname and machine-id in that file. On bootup, if the hostname is localhost, or the hostname is different than what we stored, or if the machine-id is different than what we stored, do not credit the entropy (i.e. write to /dev/urandom as before). If those criteria are met however, credit the entropy (i.e. do what the patch does). ?

@poettering

This comment has been minimized.

Copy link
Member

poettering commented Nov 2, 2016

@keszybz hmm, not a fan of too many automatisms like that, but I figure that sounds acceptible. But I'd still would like to see a boot time option for this, too.

The RNDADDENTROPY ioctl is used to feed back the random entropy
seed saved at previous shutdown, incrementing the entropy count.

Compare with manual page random(4) section ioctl(2) interface.

Signed-off-by: Werner Fink <werner@suse.de>

@bitstreamout bitstreamout force-pushed the bitstreamout:random branch from 594e0dd to 8df1001 Nov 7, 2016

@bitstreamout

This comment has been minimized.

Copy link
Contributor

bitstreamout commented Nov 7, 2016

Thanks for the feedback and your requested changes. Just finished these changes

@poettering

This comment has been minimized.

Copy link
Member

poettering commented Nov 10, 2016

Still, this should not happen unconditionally, see discussion above. Crediting entropy should only happen when we know its safe, see the discussion above.

@poettering poettering referenced this pull request Nov 23, 2016

Open

systemd-random-seed should credit the entropy #4271

1 of 2 tasks complete
@poettering

This comment has been minimized.

Copy link
Member

poettering commented Aug 31, 2017

I will close this now. We can't really solve this properly I fear, we never know where the saved randomness comes from, and whether it is used on a single system only. Sorry.

@poettering poettering closed this Aug 31, 2017

@cgwalters

This comment has been minimized.

Copy link
Member

cgwalters commented Aug 7, 2018

as it is systematically incompatible with booting systems from identical images.

I disagree - people building such images should be cleaning the random seed.

But you're right, nobody seems to care about random-seed.

Well, that's not true; from the same repo:
https://pagure.io/fedora-kickstarts/blob/master/f/fedora-atomic.ks#_125

And further in fact:
https://github.com/projectatomic/rpm-ostree/blob/e94f8c9b5f8e327c792156d6f154aedcba96ef16/src/libpriv/rpmostree-postprocess.c#L589

rpm-ostree gets this correct and has done for a long time now.
And that line links to the bug that added it to %post - and see my complaint there.

So...this is basically "let's punish everyone since we can't know which image-building tools are good".

Anyways here's my strawman proposal: let's do RNDADDENTROPY by default for ConditionVirtualization=!true. People doing bare metal imaging is a far, far smaller set than e.g. people who boot a VM in AWS, add some packages/containers, and then snapshot it and use it to run many VMs.

Concretely this issue affects my home (baremetal) server which is too old to have RDRAND.

@poettering

This comment has been minimized.

Copy link
Member

poettering commented Nov 3, 2018

Another PR, very similar to this one just got opened, in #10621. In contrast to this PR it makes the logic opt-in through a cmdline switch. I am much more positive about that (though I think it should be an env var, see my comments on that PR), simply because it means people have to explicitly opt-in into the entropy crediting logic by adding a drop-in file for systemd-random-seed.service. This way we don't just silently and carelessly credit entropy in all cases, in particular golden images where we really shouldn't.

@poettering

This comment has been minimized.

Copy link
Member

poettering commented Nov 3, 2018

I disagree - people building such images should be cleaning the random seed.

Well, but i am very sure that is not what people do who just use debootstrap or a similar tool to quickly put together an image to deploy. heck, my own tool mkosi didn't do this even, until very recently. Simply because it wasn't necessary, and didn't hurt to leave around

@poettering

This comment has been minimized.

Copy link
Member

poettering commented Nov 3, 2018

Anyways here's my strawman proposal: let's do RNDADDENTROPY by default for ConditionVirtualization=!true. People doing bare metal imaging is a far, far smaller set than e.g. people who boot a VM in AWS, add some packages/containers, and then snapshot it and use it to run many VMs.

Well, I am not convinced that such sloppy solutions are a good idea. People build embedded devices with systemd, and we shouldn't blindly make there systems less secure...

Anyway, I figure that the proposed PR #10621 which makes the crediting optional should make you happy. It would mean that if you know exactly your usecase and that people won't duplicate your images after booting them once, then you can just add the drop-in to your system and entropy will always be credited... But this is only safe if you know exactly how people use your stuff, and that noone copies images blindly after booting them at least once...

@cgwalters

This comment has been minimized.

Copy link
Member

cgwalters commented Nov 3, 2018

Anyway, I figure that the proposed PR #10621 which makes the crediting optional should make you happy. It would mean that if you know exactly your usecase and that people won't duplicate your images after booting them once, then you can just add the drop-in to your system and entropy will always be credited...

Yeah, that sounds fine to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment