Project

General

Profile

Feature #11897

Create random seed at installation time with Tails Installer

Added by bertagaz almost 2 years ago. Updated 3 days ago.

Status:
In Progress
Priority:
Normal
Assignee:
Category:
Installation
Target version:
Start date:
11/04/2016
Due date:
% Done:

20%

QA Check:
Info Needed
Feature Branch:
segfault/feature/11897-improve-random-seed-file, installer:segfault/feature/11897-Create-install-ramdom-seed
Type of work:
Code
Starter:
No
Affected tool:
Installer

Description

Given I just installed Tails on a USB stick
When I boot it for the first time
Then I want to have good entropy to create the persistent storage
Given I boot Tails from a USB stick
When I do not activate my persistent volume in the Tails Greeter
Then I still want to have good enough entropy from a random seed

For that, it is necessary to have the Tails Installer create a random seed in the system FAT partition at installation time.


Related issues

Related to Tails - Feature #7642: Investigate whether we should resume shipping a static random seed In Progress 09/02/2016
Related to Tails - Feature #7675: Persist entropy pool seeds Confirmed 11/04/2016

History

#1 Updated by bertagaz almost 2 years ago

At the November 4th meeting, we decided that this was a ticket we could start to work on.

We also decided to ask to tickets' assignee to write down a small presentation of the goal of their ticket, to be included in the blueprint that we'll show publicly. So it needs to be clear the ticket is about, what it will implement and why. There's already bits written in the blueprint dedicated paragraph, but it probably needs to be clarified for external readers. Please check and enhance it!

#2 Updated by kurono almost 2 years ago

Use the Tails installer to create a better seed #11897

When Tails is installed the first time, the most common used method is downloading the Tails ISO image, verifying it is legitimate and then copying its content to create a running system. That means that every single user has exactly the same copy of Tails. This is good for verification, but also means that every user has the same initial random seed for CSPRNG operations, used to initialize the some of the most important cryptography functions such as TLS and disk encryption (Persistence), such that they may become predictable.
To solve this problem we plan to use the Tails installer, to initialize the random seed file on every new Tails. This should be a post installation mechanism, after verifying the ISO/disk image hash/signature. We plan to use the strongest random source available in the system where Tails Installer is running from, by Python's os.urandom [1].

[1]https://docs.python.org/2/library/os.html#os.urandom

#3 Updated by kurono over 1 year ago

  • Status changed from Confirmed to In Progress

#4 Updated by u about 1 year ago

  • 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 12 months ago

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

#6 Updated by BitingBird 12 months ago

  • Target version changed from 2017 to SponsorT_2016_Internal

#7 Updated by BitingBird 12 months ago

  • Target version changed from SponsorT_2016_Internal to 2017

#8 Updated by kurono 11 months ago

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

#9 Updated by kurono 11 months ago

  • 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:

and Tails (including the blueprint) as shown in:

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

#10 Updated by intrigeri 11 months ago

  • Assignee set to bertagaz

(as per roadmap)

#11 Updated by kurono 8 months ago

#12 Updated by u 7 months ago

  • Target version changed from 2017 to 2018

2017 is over.

#13 Updated by intrigeri 7 months ago

FYI this implementation option will stop working if/once we ship Tails as a bootable USB disk image and skip the Tails Installer part of the installation process (#11679, https://tails.boum.org/blueprint/usb_install_and_upgrade/usb_bootable_disk_image/).

#14 Updated by segfault 6 months ago

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

#15 Updated by segfault 6 months ago

intrigeri wrote:

FYI this implementation option will stop working if/once we ship Tails as a bootable USB disk image and skip the Tails Installer part of the installation process (#11679, https://tails.boum.org/blueprint/usb_install_and_upgrade/usb_bootable_disk_image/).

True, with #15292 we would sadly lose the increased entropy during first boot. But we could still use the same solution to persist the random seed between subsequent boots.

#16 Updated by segfault 6 months ago

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.

I looked into this now. systemd-random-seed.service is not part of the initramfs setup we currently use in Tails, so running the script in init-bottom is fine.

#17 Updated by segfault 6 months ago

After reading systemd/random-seed.c and this comment in linux/random.c, I think we should also update the random seed in the bootup script, to make sure that we always use a different random seed (even if our shutdown script doesn't get executed, because the system crashed). But then we can't use systemd-random-seed.service, because we will need to update the random pool directly, to be able to read random bytes from it immediately. Instead, we could simply use the sample script from the linux/random.c comment:


 *    echo "Initializing random number generator..." 
 *    random_seed=/var/run/random-seed
 *    # Carry a random seed from start-up to start-up
 *    # Load and then save the whole entropy pool
 *    if [ -f $random_seed ]; then
 *        cat $random_seed >/dev/urandom
 *    else
 *        touch $random_seed
 *    fi
 *    chmod 600 $random_seed
 *    dd if=/dev/urandom of=$random_seed count=1 bs=512

#18 Updated by kurono about 1 month ago

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

#19 Updated by segfault about 1 month ago

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).
  • 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?
  • 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).
  • 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 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 #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.

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

#20 Updated by segfault about 1 month ago

  • Assignee changed from segfault to kurono

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

  • 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 #15737 to successfully build, but these commits are not relevant here. The relevant commits are fd7a4c9a27cc20f5b6994cf46f3166a98f2bf8bf and 627384842b657930967dccb66ff9a944b6fb4318 (the latter cherry-picked from my #15292 branch).

#21 Updated by kurono 27 days ago

  • 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 #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 #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 #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 27 days ago

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

#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

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

Great :)

#23 Updated by kurono 27 days ago

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

#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 26 days ago

  • 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

#25 Updated by segfault 26 days ago

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

Oh, I didn't review and test the latest commits on the installer repository

#26 Updated by intrigeri 26 days ago

  • 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 (#11897#note-13)?

#27 Updated by kurono 26 days ago

segfault wrote:

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

Oh, I didn't review and test the latest commits on the installer repository

I have reverted the commit related to the magic word before the seed.

#28 Updated by segfault 26 days ago

kurono wrote:

Oh, I didn't review and test the latest commits on the installer repository

I have reverted the commit related to the magic word before the seed.

Okay. I now reviewed the other two commits (92595584cde20efa895afe79fe30db89ae4ff67b, a3c57085c7e45060640f07617f3b28d8af031852). They look good, except that the comment and log message say that a seed file is created, which is not correct. I fixed that in my repository (https://gitlab.com/segfault3/tails-installer.git) on branch feature/11897-Create-install-random-seed.

#29 Updated by kurono 26 days ago

segfault wrote:

kurono wrote:

Oh, I didn't review and test the latest commits on the installer repository

I have reverted the commit related to the magic word before the seed.

Okay. I now reviewed the other two commits (92595584cde20efa895afe79fe30db89ae4ff67b, a3c57085c7e45060640f07617f3b28d8af031852). They look good, except that the comment and log message say that a seed file is created, which is not correct. I fixed that in my repository (https://gitlab.com/segfault3/tails-installer.git) on branch feature/11897-Create-install-random-seed.

Thanks :) It seems like I don't have have to such repo.

#30 Updated by segfault 26 days ago

kurono wrote:

Thanks :) It seems like I don't have have to such repo.

Right, I forgot to make the repo public. Did that now. (I wish there was a setting in GitLab to make repos public by default)

#31 Updated by segfault 26 days ago

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

#32 Updated by kurono 26 days ago

  • 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 25 days ago

  • 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 (#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 #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.

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 25 days ago

  • 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 23 days ago

  • 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 (#15292).

So we can't merge this branch before #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?).

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 #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 #15292, there's no hurry I guess.

#36 Updated by segfault 22 days ago

  • 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 (#15292).

So we can't merge this branch before #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.

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 #7675#note-28 and follow-ups for pointers.

Damn, I completely forgot about #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.

#37 Updated by u 3 days ago

Seems the needed info is an answer to:

What's the plan wrt. the USB image project (#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?

BTW this sounds correct to me.

#38 Updated by segfault 3 days ago

u wrote:

Seems the needed info is an answer to:

What's the plan wrt. the USB image project (#11897#note-13)?

No, it's an answer to

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 #7675#note-28 and follow-ups for pointers.

My answer was:

Damn, I completely forgot about #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.

We still have to discuss this, and I would like to know from kurono whether he already thought about appending the seed to the kernel command-line.

Also available in: Atom PDF