Tails

#3 Updated by kurono 2016-11-24 09:12:55

• Status changed from Confirmed to In Progress

#4 Updated by Anonymous 2017-07-10 12:15:42

• Subject changed from Create ramdom seed at installation time with Tails Installer to Create random seed at installation time with Tails Installer

#5 Updated by BitingBird 2017-08-28 20:06:17

• related to Feature #7642: Investigate whether we should resume shipping a static random seed added

#6 Updated by BitingBird 2017-08-28 20:09:03

• Target version changed from 2017 to SponsorT_2016_Internal

#7 Updated by BitingBird 2017-08-28 20:09:23

• Target version changed from SponsorT_2016_Internal to 2017

#8 Updated by kurono 2017-09-10 16:35:48

• Feature Branch set to feature/11897-improve-random-seed-file

#9 Updated by kurono 2017-09-25 17:15:50

• Assignee deleted (kurono)
• QA Check set to Info Needed
• Starter set to No

I have implemented a first draft solution for this ticket.
I have modified tails-installer as shown in the feature branch:

• feature/11897-Create-install-ramdom-seed
• https://git-tails.immerda.ch/kurono/liveusb-creator/log/?h=feature/11897-Create-install-ramdom-seed

and Tails (including the blueprint) as shown in:

• feature/11897-improve-random-seed-file
• https://git-tails.immerda.ch/kurono/tails/log/?h=feature/11897-improve-random-seed-file

please review the updated blueprint for the detailed changes.
One question I have is if the changes to Tails itself should be in another ticket, or even in the “Persist entropy pool seeds” one Feature #7675.

#10 Updated by intrigeri 2017-09-26 05:21:04

• Assignee set to bertagaz

(as per roadmap)

#11 Updated by kurono 2017-12-30 14:14:38

• related to Feature #7675: Persist entropy pool seeds added

#12 Updated by Anonymous 2018-01-15 14:11:13

• Target version changed from 2017 to 2018

2017 is over.

#14 Updated by segfault 2018-02-13 18:16:35

• Assignee changed from bertagaz to kurono

As discussed with kurono at the 34c3, I take over reviewing this.

I looked at your solution and I like it. Copying the seed in initramfs seems like a good solution to get the seed ready for use as soon as possible during boot. And using the systemd random seed also seems like a nice solution. I have two questions/remarks:

1. Did you verify that systemd doesn’t read the random seed before the init-bottom stage? IIRC, at least parts of systemd are already executed in initramfs, so it’s possible that the seed is not actually used during first boot.

2. I think it might be even better if, instead of storing the seed on the filesystem, we would store it in an unused sector on the device. This way we don’t have to remount the filesystem in order to update the seed, and verification of the filesystem will be easier. This would also allow us to copy the seed during an earlier initramfs stage, because the filesystem doesn’t have to be mounted yet (which might be necessary in case that systemd already read the seed before the init-bottom stage).

The Tails Installer creates a GPT, which means that the first available sector is sector 2048. The MBR and GPT only use sectors 1 to 33 [1], so we have a lot of unused 512-Byte sectors available for this.

[1] https://en.wikipedia.org/wiki/GUID_Partition_Table#Partition_entries_(LBA_2-33)

Regarding the code review: Both your patches to the Tails Installer and the initramfs scripts look good to me. (The Tails branch was a bit hard to review, because you merged both devel and master into it. I therefore reviewed d6d504a049006b9839ffcf5e8b64c652d265226d against devel.)

Regarding the blueprint update:
You state that it remains an open question whether this solves or complements the persist entropy pool seeds solution. IIUC, this solution persists the random seed of systemd to the next session, so how does this remain an open question?

The rest of the blueprint diff looks good to me.

#18 Updated by kurono 2018-07-17 19:13:29

• Assignee changed from kurono to segfault
• QA Check changed from Info Needed to Ready for QA

segfault wrote:
> As discussed with kurono at the 34c3, I take over reviewing this.
>
> I looked at your solution and I like it. Copying the seed in initramfs seems like a good solution to get the seed ready for use as soon as possible during boot. And using the systemd random seed also seems like a nice solution. I have two questions/remarks:
>

thanks a lot for your deep review :)

> 1. Did you verify that systemd doesn’t read the random seed before the init-bottom stage? IIRC, at least parts of systemd are already executed in initramfs, so it’s possible that the seed is not actually used during first boot.
>

as you already reviewed it, systemd is not part of initramfs in Tails, so this is fine.

> 2. I think it might be even better if, instead of storing the seed on the filesystem, we would store it in an unused sector on the device. This way we don’t have to remount the filesystem in order to update the seed, and verification of the filesystem will be easier. This would also allow us to copy the seed during an earlier initramfs stage, because the filesystem doesn’t have to be mounted yet (which might be necessary in case that systemd already read the seed before the init-bottom stage).
>
> The Tails Installer creates a GPT, which means that the first available sector is sector 2048. The MBR and GPT only use sectors 1 to 33 [1], so we have a lot of unused 512-Byte sectors available for this.
>
> [1] https://en.wikipedia.org/wiki/GUID_Partition_Table#Partition_entries_(LBA_2-33)
>

Great idea!, I have used your suggestion, the initramfs script now stores a random seed in an unused sector on the device. I have used two arbitrary sectors: 35 to store the seed and the end of 34 to store a magic word (seed) to know that there is a seed present.

> Regarding the code review: Both your patches to the Tails Installer and the initramfs scripts look good to me. (The Tails branch was a bit hard to review, because you merged both devel and master into it. I therefore reviewed d6d504a049006b9839ffcf5e8b64c652d265226d against devel.)
>

I have fixed the branch problem.

> Regarding the blueprint update:
> You state that it remains an open question whether this solves or complements the persist entropy pool seeds solution. IIUC, this solution persists the random seed of systemd to the next session, so how does this remain an open question?
>

I updated the blueprint as well with the changes.

> The rest of the blueprint diff looks good to me.

> After reading systemd/random-seed.c and this comment in linux/random.c, I think we should also update the random seed…

Cool, I have done that too :) Everythin is done now in the seed script, and I have deleted the changes to the shutdown hook script, that are not longer necessary. The seed in the unused sector 35 is already been updated in the initramfs script.

I think now my branch is good enough for another review.

#20 Updated by segfault 2018-07-20 00:18:32

• Assignee changed from segfault to kurono

I tested it and it seems to work. This is how I tested it:

• Created a USB image from the ISO using the prototype for Feature #15292.

• Booted the image and checked that LBA 34 contains random looking bytes:

sudo dd if=/dev/sda skip=34 count=1 | hexdump

• Rebooted and checked that LBA 34 contains different random looking bytes.

Do you want to review my code, kurono? If so, note that I also had to merge my bugfix branch for Bug #15737 to successfully build, but these commits are not relevant here. The relevant commits are fd7a4c9a27cc20f5b6994cf46f3166a98f2bf8bf and 627384842b657930967dccb66ff9a944b6fb4318 (the latter cherry-picked from my Feature #15292 branch).

#21 Updated by kurono 2018-07-24 18:45:23

• Assignee changed from kurono to segfault
• QA Check changed from Ready for QA to Dev Needed

segfault wrote:
> kurono wrote:
> > Great idea!, I have used your suggestion, the initramfs script now stores a random seed in an unused sector on the device. I have used two arbitrary sectors: 35 to store the seed and the end of 34 to store a magic word (seed) to know that there is a seed present.
>
> Great!
>
> > > Regarding the code review: Both your patches to the Tails Installer and the initramfs scripts look good to me. (The Tails branch was a bit hard to review, because you merged both devel and master into it. I therefore reviewed d6d504a049006b9839ffcf5e8b64c652d265226d against devel.)
> > >
> >
> > I have fixed the branch problem.
>
> Thanks.
>
> > > […]
> >
> > Cool, I have done that too :) Everythin is done now in the seed script, and I have deleted the changes to the shutdown hook script, that are not longer necessary. The seed in the unused sector 35 is already been updated in the initramfs script.
>
> Some things I noticed during the review:
>
> * We don’t have to update the systemd random-seed in addition to writing to /dev/urandom, because the systemd random-seed is not used for anything else but writing to /dev/urandom (see systemd/random-seed.c.
>

ah ok, I agree.

> * I strongly suspect that we also don’t need /var/lib/urandom/random-seed, but I couldn’t find any information about when this is used at all. Do you know how it is used?
>

I wanted to make it similar to the sample initialization script but I think it is not really needed.

> * When we only write the seed to /dev/urandom, we don’t have to check for a magic number, because it doesn’t make any difference if we write all zeroes to /dev/urandom or nothing at all (in both cases it will not increase entropy).
>

I am not sure about this. What we have concluded in Feature #7642 is that it was better to avoid a static random seed file (or data). We were rather leaving the random device without initialization. If we don’t check if there is an actual seed present, we could go back to that scenario with static seed bytes (zeros?) affecting many Tails users. Of course, it would happen only in the very first boot, since the shutdown script will create the seed for subsequent boots.

according to https://linux.die.net/man/4/urandom:

“Writing to /dev/random or /dev/urandom will update the entropy pool with the data written, but this will not result in a higher entropy count. This means that it will impact the contents read from both files, but it will not make reads from /dev/random faster.”

what does not get affected is the entropy count, it is only relevant for /dev/random that waits for having a higher count. But the actual entropy would get updated with the static seed which may make it predictable.

> * We should still also update the seed before shutdown, because else we lose all the entropy gathered between boot and shutdown. See also the comment in linux/random.c which instructs to update the seed both during boot and before shutdown.
>

I agree.

> * I think it would be better if we restored the seed earlier than at the init-bottom stage (which is the last initramfs stage during boot), so that it can be used in other initramfs scripts. For example, in Feature #15292 we generate a new random GUID during the live-realpremount stage. I think the live-realpremount stage is also the earliest stage at which we can access the device.
>

ok, now that we don’t write to the file system I think its ok to do it before.

> I created a script in the live-realpremount stage on my branch (https://gitlab.com/segfault3/tails/tree/feature/11897-improve-random-seed-file). I reused some code from my Feature #15292 branch and cherry-picked 627384842b657930967dccb66ff9a944b6fb4318 to be able to access the device during live-realpremount.
>
> I’m currently building an image to test this.

It works for me as well. I like your changes, only suggestion would be to review the seed availability issue.

#22 Updated by segfault 2018-07-24 20:35:01

• Assignee changed from segfault to kurono
• QA Check changed from Dev Needed to Ready for QA

kurono wrote:
> > * When we only write the seed to /dev/urandom, we don’t have to check for a magic number, because it doesn’t make any difference if we write all zeroes to /dev/urandom or nothing at all (in both cases it will not increase entropy).
>
> I am not sure about this. What we have concluded in Feature #7642 is that it was better to avoid a static random seed file (or data). We were rather leaving the random device without initialization. If we don’t check if there is an actual seed present, we could go back to that scenario with static seed bytes (zeros?) affecting many Tails users. Of course, it would happen only in the very first boot, since the shutdown script will create the seed for subsequent boots.

#23 Updated by kurono 2018-07-24 21:29:24

• Assignee changed from kurono to intrigeri
• % Done changed from 0 to 20

segfault wrote:
> kurono wrote:
> > > * When we only write the seed to /dev/urandom, we don’t have to check for a magic number, because it doesn’t make any difference if we write all zeroes to /dev/urandom or nothing at all (in both cases it will not increase entropy).
> >
> > I am not sure about this. What we have concluded in Feature #7642 is that it was better to avoid a static random seed file (or data). We were rather leaving the random device without initialization. If we don’t check if there is an actual seed present, we could go back to that scenario with static seed bytes (zeros?) affecting many Tails users. Of course, it would happen only in the very first boot, since the shutdown script will create the seed for subsequent boots.
>
> Feature #7642 is very long, and I don’t want to read all of the comments. Searching for “uninitialized” or “initial” doesn’t bring up anything about it being better to leave the entropy pool uninitialized than initializing with known data. I argue below why I don’t think that this is the case.
>
> > according to https://linux.die.net/man/4/urandom:
> >
> > “Writing to /dev/random or /dev/urandom will update the entropy pool with the data written, but this will not result in a higher entropy count. This means that it will impact the contents read from both files, but it will not make reads from /dev/random faster.”
> >
> > what does not get affected is the entropy count, it is only relevant for /dev/random that waits for having a higher count. But the actual entropy would get updated with the static seed which may make it predictable.
>
> The way I understand it, the entropy does get updated, but it does not get reset. The previous state is not lost, which means that the entropy should never decrease when you write to /dev/urandom (which would also be a pretty serious security issue, because at least on my system, /dev/urandom is world-writable).
>
> I now looked this up in the kernel source code, to make sure that my understanding is correct:
>
> - The write operation of /dev/urandom is set to random_write [1]
> - random_write calls write_pool [2]
> - write_pool calls mix_pool_bytes [3]
> - mix_pool_bytes adds bytes to the entropy pool and mixes them with the existing bytes [4]
>
> [1] https://github.com/torvalds/linux/blob/f89ed2f880ccb117246ba095e12087d9c3df89c5/drivers/char/random.c#L2001
> [2] https://github.com/torvalds/linux/blob/f89ed2f880ccb117246ba095e12087d9c3df89c5/drivers/char/random.c#L1916
> [3] https://github.com/torvalds/linux/blob/f89ed2f880ccb117246ba095e12087d9c3df89c5/drivers/char/random.c#L1909
> [4] https://github.com/torvalds/linux/blob/f89ed2f880ccb117246ba095e12087d9c3df89c5/drivers/char/random.c#L512
>

You are are right, I reviewed again the thread and the conclusion was more that not having a seed is as bad as having the same static and public seed during every system booting. So ok, writing zeroes or garbage to /dev/urandom does not change anything. That specific case is not covered here then.

I have merged your changes in my branch and I am assigning this ticket to @intrigeri.

> > It works for me as well. I like your changes, only suggestion would be to review the seed availability issue.
>
> Great :)

#24 Updated by intrigeri 2018-07-24 23:52:59

• Feature Branch changed from feature/11897-improve-random-seed-file to kurono/feature/11897-improve-random-seed-file, installer:kurono/feature/11897-Create-install-ramdom-seed

#26 Updated by intrigeri 2018-07-25 00:12:42

• Assignee changed from intrigeri to kurono
• QA Check changed from Ready for QA to Dev Needed

Great work, both of you! Here’s a first review. I did not test the code.

• This will need design doc. I understand most of the needed text is already on the blueprint but the topic branch should move (and possibly reorganize) the relevant bits to somewhere under contribute/design.
• Please drop the unrelated commits (AppArmor CUPS profile) from feature/11897-improve-random-seed-file.
• I think the “and grow the system partition” commit message is wrong (for now). Please fix it :)
• In config/chroot_local-includes/usr/local/lib/initramfs-pre-shutdown-hook, please move the added code somewhere else (on top?) rather than in the middle of a set of operations that all go together.
• If we supersede systemd-random-seed.service, perhaps we should disable it to avoid confusion?
• Please quote variables in shell scripts.
• In cat | sed one does not need cat: just pass the input file to sed.
• How will all this code behave:
  • on an “intermediary Tails” i.e. one created by cat’ing the hybrid ISO to a USB stick?
  • on a stick created by UUI?
  • when booting from DVD?
  • on a stick created by an older Tails Installer, that does not initialize the seed?
  • If this all works fine, cool. The design doc should explain why.
• After “Skipping restoring random seed” we exit 1. What’s the effect on the boot process? In case it blocks other services from starting, is it really what we want?
• What’s the plan wrt. “XXX: We assume”?
• Please add at least set -u in new shell scripts, and set -e when it makes sense.
• Please make the regexp in grep ID_PATH= a bit stricter.
• What’s the plan wrt. the USB image project (Feature #11897#note-13)?

#32 Updated by kurono 2018-07-25 16:29:32

• Assignee changed from kurono to segfault

segfault wrote:
> kurono: If that is okay for you, I would start working on the issues intrigeri raised tonight. Or do you want to do that?

I won’t have time to work on this for a while so feel free to continue with it.
I have merge and pushed your changes in my installer branch.

#33 Updated by segfault 2018-07-25 23:57:05

• Assignee changed from segfault to kurono
• Feature Branch changed from kurono/feature/11897-improve-random-seed-file, installer:kurono/feature/11897-Create-install-ramdom-seed to segfault/feature/11897-improve-random-seed-file, installer:segfault/feature/11897-Create-install-ramdom-seed

intrigeri wrote:
> Great work, both of you!

Thanks!

> Here’s a first review. I did not test the code.
>
> * This will need design doc. I understand most of the needed text is already on the blueprint but the topic branch should move (and possibly reorganize) the relevant bits to somewhere under contribute/design.

Okay, I will do that later.

> * Please drop the unrelated commits (AppArmor CUPS profile) from feature/11897-improve-random-seed-file.

Done. Rebased on devel to fix the AppArmor CUPS profile issue.

I’m changing the feature branch to my repo, so kurono doesn’t have to merge my changes.

> * I think the “and grow the system partition” commit message is wrong (for now). Please fix it :)

Done.

> * In config/chroot_local-includes/usr/local/lib/initramfs-pre-shutdown-hook, please move the added code somewhere else (on top?) rather than in the middle of a set of operations that all go together.

Done.

> * If we supersede systemd-random-seed.service, perhaps we should disable it to avoid confusion?

Done by disabling the service in chroot_local_hooks/52-update-rc.d. I used systemctl disable instead of systemctl mask because it’s not a problem if the service is started manually.

> * Please quote variables in shell scripts.

Done.

> * In cat | sed one does not need cat: just pass the input file to sed.

Fixed.

> * How will all this code behave:
> on an “intermediary Tails” i.e. one created by cat’ing the hybrid ISO to a USB stick?
> when booting from DVD?

In these cases, the FSUUID is not set by syslinux, so the code will wait for 60 seconds for a system partition it cannot find and then exit. I think we should instead return immediately if no FSUUID is set: If it’s a DVD, we can’t store a random seed anyway. If it’s an intermediary Tails, I don’t want to spend time on finding a special solution to use a random seed, because of our plans to get rid of the intermediary Tails (Feature #15292).

> on a stick created by UUI?

Here the FSUUID is set and the unitialized block is written to /dev/urandom and gets updated. I.e. during the first boot non-random bytes are written to /dev/urandom, which should not be a problem, as explained in Feature #11897#note-22. On subsequent boots, the random seed is read and updated as expected.

> on a stick created by an older Tails Installer, that does not initialize the seed?

Same as for sticks created with UUI.

> If this all works fine, cool. The design doc should explain why.

Okay.

> * After “Skipping restoring random seed” we exit 1. What’s the effect on the boot process? In case it blocks other services from starting, is it really what we want?

There is no effect. For the record, this is how the seed script is executed:

/scripts/live-realpremount/seed
called by /scripts/live-realpremount/ORDER
called by run_scripts() in /scripts/functions
called by /lib/live/boot/9990-overlay.sh

The ORDER script is generated and calls all the scripts in the directory, it doesn’t care about their exit codes. Do you prefer that we change it to “exit 0” to avoid confusion?

> * What’s the plan wrt. “XXX: We assume”?

I put in that “XXX:” because I’m not completely sure whether we can assume that the parent device is populated in /dev when the system partition is. If not, that would be a race condition, and the random seed would not be updated when the race is lost. Do you think it’s safe to assume that, or should we investigate?

> * Please add at least set -u in new shell scripts, and set -e when it makes sense.

“-e” was already set in the shebang. I added “-u” and moved them to their own set statement.

> * Please make the regexp in grep ID_PATH= a bit stricter.

Done.

> * What’s the plan wrt. the USB image project (Feature #11897#note-13)?

We won’t be able to set the random seed at installation time, but the scripts will still save the entropy pool at shutdown and restore it at boot, so that’s still an improvement.

#34 Updated by segfault 2018-07-26 01:19:09

• Assignee changed from kurono to intrigeri
• QA Check changed from Dev Needed to Ready for QA

segfault wrote:
> > * This will need design doc. I understand most of the needed text is already on the blueprint but the topic branch should move (and possibly reorganize) the relevant bits to somewhere under contribute/design.
>
> Okay, I will do that later.

I drafted something.

#35 Updated by intrigeri 2018-07-28 10:01:01

• Assignee changed from intrigeri to segfault
• QA Check changed from Ready for QA to Dev Needed

segfault wrote:
> intrigeri wrote:
>> * How will all this code behave:
>> on an “intermediary Tails” i.e. one created by cat’ing the hybrid ISO to a USB stick?
>> when booting from DVD?

> In these cases, the FSUUID is not set by syslinux, so the code will wait for 60 seconds for a system partition it cannot find and then exit. I think we should instead return immediately if no FSUUID is set: If it’s a DVD, we can’t store a random seed anyway. If it’s an intermediary Tails, I don’t want to spend time on finding a special solution to use a random seed, because of our plans to get rid of the intermediary Tails (Feature #15292).

So we can’t merge this branch before Feature #15292 is done ⇒ please add a “blocked by” relationship. Or did I get it wrong?

>> * After “Skipping restoring random seed” we exit 1. What’s the effect on the boot process? In case it blocks other services from starting, is it really what we want?

> There is no effect. For the record, this is how the seed script is executed:

> /scripts/live-realpremount/seed
> called by /scripts/live-realpremount/ORDER
> called by run_scripts() in /scripts/functions
> called by /lib/live/boot/9990-overlay.sh

> The ORDER script is generated and calls all the scripts in the directory, it doesn’t care about their exit codes. Do you prefer that we change it to “exit 0” to avoid confusion?

I would like this code not to break the boot unintentionally if the caller ever starts caring about exit codes. So you folks first need to decide what we want to happen (i.e. should this block the boot?) and then adjust the code to make sure it will still do what we want if the caller ever starts caring about exit codes + add a comment explaining why this does not matter currently.

>> * What’s the plan wrt. “XXX: We assume”?

> I put in that “XXX:” because I’m not completely sure whether we can assume that the parent device is populated in /dev when the system partition is. If not, that would be a race condition, and the random seed would not be updated when the race is lost. Do you think it’s safe to assume that, or should we investigate?

I have no idea whether it’s a safe assumption (it might be the case right now but even if it is, is that documented behavior or implementation detail that can change at any time?).

>> * What’s the plan wrt. the USB image project (Feature #11897#note-13)?

> We won’t be able to set the random seed at installation time,

Indeed, for most installation paths. But sticks created by cloning an existing Tails using Tails Installer will still benefit from the changes this branch proposes for Tails Installer, right?

> but the scripts will still save the entropy pool at shutdown and restore it at boot, so that’s still an improvement.

Fine by me!

Two more questions:

• What’s the reasoning that lead you folks to choose this implementation instead of storing the persistent entropy pool seed on the kernel command-line (ideally sourced from a dedicated file) and letting the kernel load it itself? See Feature #7675#note-28 and follow-ups for pointers. I guess that saving the updated seed would be slightly more involved (need to mount the system partition RW) but we would need absolutely no code to load it at boot. I have no strong opinion at this point but at least I’d like to understand why this other option was rejected.
• Is there a plan to write automated tests for this new feature?

I’ll try to start reviewing the code soonish but given the relationship with Feature #15292, there’s no hurry I guess.

#36 Updated by segfault 2018-07-28 23:06:59

• Assignee changed from segfault to kurono
• QA Check changed from Dev Needed to Info Needed

intrigeri wrote:
> segfault wrote:
> > intrigeri wrote:
> >> * How will all this code behave:
> >> on an “intermediary Tails” i.e. one created by cat’ing the hybrid ISO to a USB stick?
> >> when booting from DVD?
>
> > In these cases, the FSUUID is not set by syslinux, so the code will wait for 60 seconds for a system partition it cannot find and then exit. I think we should instead return immediately if no FSUUID is set: If it’s a DVD, we can’t store a random seed anyway. If it’s an intermediary Tails, I don’t want to spend time on finding a special solution to use a random seed, because of our plans to get rid of the intermediary Tails (Feature #15292).
>
> So we can’t merge this branch before Feature #15292 is done ⇒ please add a “blocked by” relationship. Or did I get it wrong?

That’s not what I meant. Let me rephrase. I think it’s OK to not set the random seed in case that the FSUUID is not set, which happens in the following cases:

- The device is a DVD: I don’t see a reasonable way to preserve an entropy pool here, because we can’t write to the DVD at every shutdown.

- The device is an intermediary Tails. This means that the device was not created by Tails Installer, so at the first boot it doesn’t have an entropy seed anyway. And I don’t think it’s worth spending time to find a solution to preserve the entropy between shutdown and boot, because the intermediary Tails is not meant to be rebooted often anyway, and also because we have plans to get rid of it.

I implemented this in commit 7ee6e63045abedebdcfcecc240dc73996d52388f, so that now the script exits immediately if $FSUUID is not set.

> >> * After “Skipping restoring random seed” we exit 1. What’s the effect on the boot process? In case it blocks other services from starting, is it really what we want?
>
> > There is no effect. For the record, this is how the seed script is executed:
>
> > /scripts/live-realpremount/seed
> > called by /scripts/live-realpremount/ORDER
> > called by run_scripts() in /scripts/functions
> > called by /lib/live/boot/9990-overlay.sh
>
> > The ORDER script is generated and calls all the scripts in the directory, it doesn’t care about their exit codes. Do you prefer that we change it to “exit 0” to avoid confusion?
>
> I would like this code not to break the boot unintentionally if the caller ever starts caring about exit codes. So you folks first need to decide what we want to happen (i.e. should this block the boot?) and then adjust the code to make sure it will still do what we want if the caller ever starts caring about exit codes + add a comment explaining why this does not matter currently.

I think the current behavior is what we want, i.e. that it does not block boot. I’m now changing the exit codes to 0 so that this behavior is preserved in case that the caller ever starts caring about the exit codes.

> >> * What’s the plan wrt. “XXX: We assume”?
>
> > I put in that “XXX:” because I’m not completely sure whether we can assume that the parent device is populated in /dev when the system partition is. If not, that would be a race condition, and the random seed would not be updated when the race is lost. Do you think it’s safe to assume that, or should we investigate?
>
> I have no idea whether it’s a safe assumption (it might be the case right now but even if it is, is that documented behavior or implementation detail that can change at any time?).

I changed to code to wait until the parent device is definitely available.

> >> * What’s the plan wrt. the USB image project (Feature #11897#note-13)?
>
> > We won’t be able to set the random seed at installation time,
>
> Indeed, for most installation paths. But sticks created by cloning an existing Tails using Tails Installer will still benefit from the changes this branch proposes for Tails Installer, right?

I think so. I just spent 15 minutes examining the Tails Installer code to figure out if this is indeed the case, but failed to do so. kurono, can you answer this?

> Two more questions:
>
> * What’s the reasoning that lead you folks to choose this implementation instead of storing the persistent entropy pool seed on the kernel command-line (ideally sourced from a dedicated file) and letting the kernel load it itself? See Feature #7675#note-28 and follow-ups for pointers.

Damn, I completely forgot about Feature #7675#note-28. I didn’t take it into account while working on this ticket and also didn’t see any comment from kurono about it. We should discuss whether we want to load the seed via the kernel command-line instead. This would require updating a file that is included in syslinux.cfg, to append the seed to the command-line.

> I guess that saving the updated seed would be slightly more involved (need to mount the system partition RW) but we would need absolutely no code to load it at boot.

We wouldn’t need code to load it at boot, but we would need code to update it during boot, if we want to make sure that the seed is different at every boot. We could still do this in the initramfs.

> * Is there a plan to write automated tests for this new feature?

We should probably write automated tests for this. I can do that once we know for sure that we want to merge this and the review passed.

#50 Updated by intrigeri 2019-02-24 11:20:38

• Target version deleted (2018)

2018 is over and this did not make it on our roadmap for next year (incomplete team). But it would be really nice to have :)

#52 Updated by intrigeri 2019-06-02 15:24:25

• QA Check deleted (Info Needed)

(Preparing to drop the “QA Check” field as per “[Tails-dev] Proposal: Redmine workflow change”.)

#53 Updated by segfault 2019-07-17 21:53:54

• related to Feature #16891: Ensure enough entropy is available when setting up persistence added

#55 Updated by segfault 2019-07-28 12:20:38

• Status changed from In Progress to Needs Validation
• Assignee deleted (kurono)
• % Done changed from 20 to 50
• Feature Branch changed from segfault/feature/11897-improve-random-seed-file, installer:segfault/feature/11897-Create-install-ramdom-seed to feature/11897-improve-random-seed-file, installer:segfault/feature/11897-Create-install-ramdom-seed

I finished and tested the implementation of writing a random seed to the kernel command-line via the syslinux config. It seems to work and is ready for review.

#56 Updated by intrigeri 2019-08-11 13:21:40

• Status changed from Needs Validation to In Progress
• Assignee set to segfault

Hi segfault!

segfault wrote:
> I continued working on this, and I think it’s close to being ready for review.

Neat! I took a quick look at the tails.git branch (not the installer one for now) and read some other stuff to refresh my mind. This is not a proper review (e.g. I’ll ignore the outdated bits of design doc for now) but I hope it’ll help keep the ball rolling :)

> We should consider using both methods to persist the entropy pool, the one via the kernel command-line and the one where we store the seed in an unused sector and feed it to the RNG in the initramfs. We already implemented both, and this would be reduce the risk that users end up with bad entropy in the case that something is broken. And it’s hard to detect if something is broken, because both methods don’t increase the kernel’s entropy count (because the kernel doesn’t know whether we added good random data), so there is no way to check whether the entropy was actually increased.

Good question! Indeed, we need to make up our mind here, at least because the proposed branch is inconsistent in this respect:

• commit:8a9377ad50c9e999f2e4dd0faed96599e9877e53 and commit:7fcf1dd1dcae7e34f3c2873dc6da59849c12fb6d fully replaced the “store the seed in an unused sector” approach with the kernel command-line one.
• The design doc still mentions “LBA 34”.

Regarding “hard to detect if something is broken”: could we use /proc/sys/kernel/random/poolsize to detect whether the data was successfully added to the pool via add_device_randomness in init/main.c? According to https://www.bsi.bund.de/SharedDocs/Downloads/EN/BSI/Publications/Studies/LinuxRNG/LinuxRNG_EN.pdf?__blob=publicationFile&v=10, add_device_randomness adds directly to to input_pool, and /proc/sys/kernel/random/poolsize is “the size of the input_pool in bits”. We could for example print the content of that file during (very) early boot to the Journal, and write an automated test that verifies that the value in there in big enough. Presumably the VMs we run Tails in during our test suite are all similar enough that 1. the work done on this ticket visibly increases this value (so I would create a branch based on current stable, that prints this value to the Journal and adds the test, which should fail; then the same test should pass on this branch); 2. a suitable lower limit, that would work for all situations where we run the test suite, exists.

If we can indeed detect when the kernel command-line version is broken, then I’m slightly in favour of keeping things simple and relying on this sole mechanism.

> This is really quite painful. In my VM, when I edit the kernel command-line in the boot menu with this huge random string in it, the screen isn’t correctly updated when I move the cursor.

Same problem here. I think we can’t allow ourselves to harm usability too much here: for example, some users have to add a parameter on the kernel command-line every time they start Tails, to make it work on their hardware. Given we’ll migrate away from syslinux soon, let’s not spend too much time on that: let’s figure out what’s the longest appended random string that does not break the syslinux menu, and use that.

And then, when we migrate to GRUB, we can reconsider where we write this string: GRUB has variables so presumably the kernel command-line that a user may need to edit would only say something like $randomseed, and then the length of the string is only limited by the kernel.

Deal?

>> In the initramfs, we could use RNDADDENTROPY to also increase the entropy count.

> I think this is a good idea. It will cause the RNG to be marked as initialized and therefore prevent warnings, and it will prevent reads from /dev/random to block (and tails-persistence-setup, if we implement my proposal on Feature #16891). We will have to write a small C program for that.

I’m in favour of this proposal. I don’t feel qualified to determine how much entropy we can credit here but I’ll happily review well-justified code :)
I don’t think it’s a blocker though; if you agree, maybe we could track this on a dedicated ticket?

> And lastly, we might want to rename this issue to better describe the main advantage of the work done here: […]

Yes, please :)

I’ll look into Feature #11898 soonish.

#57 Updated by intrigeri 2019-08-11 14:37:22

• Subject changed from Create random seed at installation time with Tails Installer to Create random seed with Tails Installer, update seed during early boot and shutdown

#58 Updated by intrigeri 2019-08-11 14:39:18

• Description updated

#59 Updated by intrigeri 2019-08-11 15:15:41

• related to deleted (Feature #7675: Persist entropy pool seeds)

#60 Updated by intrigeri 2019-08-11 15:15:51

• has duplicate Feature #7675: Persist entropy pool seeds added

#63 Updated by segfault 2019-08-13 23:32:47

• Subject changed from Create random seed with Tails Installer, update seed during early boot and shutdown to Persist a random seed across boots

intrigeri wrote:
> […] I’ll ignore the outdated bits of design doc for now

I plan to update the design doc once we settled on a design.

> > We should consider using both methods to persist the entropy pool, the one via the kernel command-line and the one where we store the seed in an unused sector and feed it to the RNG in the initramfs. We already implemented both, and this would be reduce the risk that users end up with bad entropy in the case that something is broken. And it’s hard to detect if something is broken, because both methods don’t increase the kernel’s entropy count (because the kernel doesn’t know whether we added good random data), so there is no way to check whether the entropy was actually increased.
>
> Good question! Indeed, we need to make up our mind here, at least because the proposed branch is inconsistent in this respect:
>
> * commit:8a9377ad50c9e999f2e4dd0faed96599e9877e53 and commit:7fcf1dd1dcae7e34f3c2873dc6da59849c12fb6d fully replaced the “store the seed in an unused sector” approach with the kernel command-line one.

Yes, my proposal is to bring back the “store the seed in an unused sector” functionality.

> Regarding “hard to detect if something is broken”: could we use /proc/sys/kernel/random/poolsize to detect whether the data was successfully added to the pool via add_device_randomness in init/main.c? According to https://www.bsi.bund.de/SharedDocs/Downloads/EN/BSI/Publications/Studies/LinuxRNG/LinuxRNG_EN.pdf?__blob=publicationFile&v=10, add_device_randomness adds directly to to input_pool, and /proc/sys/kernel/random/poolsize is “the size of the input_pool in bits”.

I’didn’t read the pdf from the BSI, but I’m pretty sure the answer is no. According to https://linux.die.net/man/4/random, /proc/sys/kernel/random/poolsize just contains the value 4096 (since Linux 2.6). My understanding is that this is the maximum pool size, not the number of bytes added to the pool.

> If we can indeed detect when the kernel command-line version is broken, then I’m slightly in favour of keeping things simple and relying on this sole mechanism.

I don’t think we can, and even then I would argue that we should also use the seed in an unused sector:

But I still see these points in favor of also using a seed in an unused sector, in addition to the kernel command-line:

• We can credit this entropy, which will cause the entropy pool to be initialized afterwards. This will prevent warnings and allows programs which wait for an initialized entropy pool to run earlier (not sure there is currently anything waiting for an initialized entropy pool in Tails though, but is relevant for Feature #16891).
• Simply as a safety measure. Having two sources of entropy will make it more failsafe (the kernel command-line feature was broken before, see https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ee7998c50c2697737c6530431709f77c852bf0d6).

> > This is really quite painful. In my VM, when I edit the kernel command-line in the boot menu with this huge random string in it, the screen isn’t correctly updated when I move the cursor.
>
> Same problem here. I think we can’t allow ourselves to harm usability too much here: for example, some users have to add a parameter on the kernel command-line every time they start Tails, to make it work on their hardware. Given we’ll migrate away from syslinux soon, let’s not spend too much time on that: let’s figure out what’s the longest appended random string that does not break the syslinux menu, and use that.
>
> And then, when we migrate to GRUB, we can reconsider where we write this string: GRUB has variables so presumably the kernel command-line that a user may need to edit would only say something like $randomseed, and then the length of the string is only limited by the kernel.
>
> Deal?

OK. The GRUB variable sounds good, and I’m fine with using a smaller seed until we migrate to GRUB.

> >> In the initramfs, we could use RNDADDENTROPY to also increase the entropy count.
>
> > I think this is a good idea. It will cause the RNG to be marked as initialized and therefore prevent warnings, and it will prevent reads from /dev/random to block (and tails-persistence-setup, if we implement my proposal on Feature #16891). We will have to write a small C program for that.
>
> I’m in favour of this proposal. I don’t feel qualified to determine how much entropy we can credit here but I’ll happily review well-justified code :)

I don’t see any reason why we shouldn’t credit all the entropy that we add, i.e. 512 bytes.

> I don’t think it’s a blocker though; if you agree, maybe we could track this on a dedicated ticket?

Agreed.

> > And lastly, we might want to rename this issue to better describe the main advantage of the work done here: […]
>
> Yes, please :)

Done.

> I’ll look into Feature #11898 soonish.

Thanks, I’ll reply to the tails-dev thread.

#64 Updated by segfault 2019-08-13 23:36:50

• Description updated

#73 Updated by intrigeri 2019-08-14 06:12:34

• blocks Feature #16977: Consider crediting some of the entropy we add from an unused sector added

#83 Updated by segfault 2019-08-15 18:17:46

• related to Bug #16980: Increase size of random seed in the kernel command-line added

#86 Updated by segfault 2019-08-18 18:39:52

• Status changed from In Progress to Needs Validation
• Assignee deleted (segfault)

> I restored the functionality to store/restore the seed in LBA 34, and reduced the length of the kernel command-line seed to 32 characters. Will now build and test.

Works in my tests.

#88 Updated by segfault 2019-08-24 19:16:14

• Assignee set to segfault

cypherpunks wrote:
> This isn’t a reason not to do this, but it should be taken into account that this will allow someone with physical access to determine how many times Tails has booted since it was first installed due to dynamic wear leveling. This is not likely to be an issue for most threat models, but it might be a good idea to mention this in the documentation.

This is still on my mind, I would like to investigate ways to reduce the chance that someone can determine how many times Tails was booted.

intrigeri wrote:
> I think we’ve settled on the design. Do you prefer me to do another code review now, before you update the design doc?
> Or shall we minimize the number of rounds of review, and thus, you update the design doc first?

I can update the design doc first. But I’m not sure whether we have the resources (me updating the design doc + you doing the review) to get this done in time for 4.0~beta2. So I would rather focus on other things for 3.16 / 4.0~beta2. @intrigeri, please tell me if you disagree.

#91 Updated by segfault 2019-10-15 13:10:00

• blocked by deleted (Feature #16977: Consider crediting some of the entropy we add from an unused sector)