Project

General

Profile

Feature #11897

Persist a random seed across boots

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

Status:
Needs Validation
Priority:
Normal
Assignee:
Category:
Installation
Target version:
-
Start date:
11/04/2016
Due date:
% Done:

50%

Feature Branch:
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 boot Tails from a USB stick
Then I want to have good entropy from a random seed, regardless of whether I have persistence enabled or not

Related issues

Related to Tails - Feature #7642: Investigate whether we should resume shipping a static random seed Resolved 09/02/2016
Related to Tails - Feature #16891: Ensure enough entropy is available when setting up persistence Confirmed
Related to Tails - Bug #16980: Increase size of random seed in the kernel command-line Confirmed
Duplicated by Tails - Feature #7675: Persist entropy pool seeds Duplicate 11/04/2016
Blocks Tails - Feature #16977: Consider crediting some of the entropy we add from an unused sector Confirmed

Associated revisions

Revision 4afc828d (diff)
Added by segfault about 1 year ago

Improve how the random seed is stored (refs: #11897)

  • Move script from init-bottom to the earlier live-realpremount stage,
    in order to make use of the random seed when generating the new GUID
    as part of #15292.
  • Don't update seeds in the filesystem, which are only used to write to
    /dev/urandom anyway, which we already do ourselves.
  • Skip check for magic number, because not writing anything to
    /dev/urandom is as bad as writing whatever is stored in the seed
    sector (worst case is that entropy is not increased at all).
  • Update the seed in initramfs-pre-shutdown-hook, because else we lose
    all the entropy gathered between boot and shutdown.

Revision 07c57aa5 (diff)
Added by segfault 11 months ago

Initialize random seed in premount stage (refs: #11897)

To have it initialized before executing the partitioning script

Revision 401869fc (diff)
Added by segfault 11 months ago

Prevent seed script to exit if called without arguments (refs: #11897)

Revision 8a9377ad (diff)
Added by segfault 11 months ago

Add seed to kernel command-line (refs: #11897)

Revision 7fcf1dd1 (diff)
Added by segfault 11 months ago

Also update seed in kernel command-line during shutdown (refs: #11897)

Revision c71a84bf (diff)
Added by segfault 11 months ago

Don't try to update the seed on an ISO (refs: #11897)

Also fixes mixed use of tabs and spaces.

Revision 6830e8f3 (diff)
Added by segfault 2 months ago

Simplify regex (refs: #11897)

Revision 515d292b (diff)
Added by segfault 2 months ago

Mount system partition read-write while updating random seed (refs: #11897)

Revision 5b5f252e (diff)
Added by segfault about 2 months ago

Fix write on read-only filesystem (refs: #11897)

Revision f779b8ba (diff)
Added by segfault about 1 month ago

Bring back random seed sector (refs: #11897)

We decided that we want to use both methods to seed the entropy pool,
the one using the kernel command-line and the seed stored in LBA 34.

Revision fd73d31e (diff)
Added by segfault about 1 month ago

Reduce bytes stored in kernel command-line seed (refs: #11897)

By repeatedly dividing the length of the random seed string by two, the
first length that didn't break the syslinux menu was 32, i.e. 16 bytes.

We could check if some length between 32 and 64 would also work. Or we
could use base64 to produce a string that encodes more bytes per
character. But we agreed that we don't want to spend more time on this,
because we also use the random seed stored in LBA 34, and once we
migrate to GRUB, we can probably use a 512 Byte seed by storing it in a
variable.

Revision d955e759 (diff)
Added by segfault about 1 month ago

Bring back random seed sector (refs: #11897)

We decided that we want to use both methods to seed the entropy pool,
the one using the kernel command-line and the seed stored in LBA 34.

Revision 5d8e3f76 (diff)
Added by segfault about 1 month ago

Reduce bytes stored in kernel command-line seed (refs: #11897)

By repeatedly dividing the length of the random seed string by two, the
first length that didn't break the syslinux menu was 32, i.e. 16 bytes.

We could check if some length between 32 and 64 would also work. Or we
could use base64 to produce a string that encodes more bytes per
character. But we agreed that we don't want to spend more time on this,
because we also use the random seed stored in LBA 34, and once we
migrate to GRUB, we can probably use a 512 Byte seed by storing it in a
variable.

Revision b53f9b71 (diff)
Added by segfault about 1 month ago

Fix mount command-line option (refs: #11897)

Revision e1040a58 (diff)
Added by segfault about 1 month ago

Improve shell script style (refs: #11897)

  • Add missing quotes
  • Use consistent indentation
  • Use more consistent code style
  • Simplify while loop

Revision ee375e00 (diff)
Added by segfault about 1 month ago

Add plymouth as prerequirement (refs: #11897)

We did the same thing for the partitioning script, in order to show the
plymouth splash screen while the script is executed.

History

#1 Updated by bertagaz almost 3 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 3 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 almost 3 years ago

  • Status changed from Confirmed to In Progress

#4 Updated by u about 2 years 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 about 2 years ago

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

#6 Updated by BitingBird about 2 years ago

  • Target version changed from 2017 to SponsorT_2016_Internal

#7 Updated by BitingBird about 2 years ago

  • Target version changed from SponsorT_2016_Internal to 2017

#8 Updated by kurono about 2 years ago

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

#9 Updated by kurono almost 2 years 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 almost 2 years ago

  • Assignee set to bertagaz

(as per roadmap)

#11 Updated by kurono over 1 year ago

#12 Updated by u over 1 year ago

  • Target version changed from 2017 to 2018

2017 is over.

#13 Updated by intrigeri over 1 year 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 over 1 year 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 over 1 year 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 over 1 year 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 over 1 year 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 year 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 year 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 year 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 about 1 year 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 about 1 year 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 about 1 year 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 about 1 year 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 about 1 year 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 about 1 year 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 about 1 year 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 about 1 year 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 about 1 year 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 about 1 year 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 about 1 year 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 about 1 year 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 about 1 year 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 about 1 year 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 about 1 year 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 about 1 year 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 about 1 year 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 about 1 year 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.

#39 Updated by kurono about 1 year ago

segfault wrote:

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?

The seed installation is made inside "live.reset_mbr()", which is applied in all tail installation cases, except when the device is "/dev/loop" (CD/ISOs?).

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 also missed that :( I don't know yet if the current method is better or worse than the kernel command-line method.

#40 Updated by segfault 11 months ago

This is how I would try to use the kernel command-line for the randomness seeding:

  • During installation, have Tails Installer create a file /EFI/BOOT/random.cfg, with a random string in it
  • Include /EFI/BOOT/random.cfg in /EFI/BOOT/syslinux.cfg using APPEND.
  • During boot, in the init-bottom stage (i.e. after the device was mounted), replace /EFI/BOOT/random.cfg with a new random string.
  • During shutdown, also replace /EFI/BOOT/random.cfg with a new random string.

The advantage of using the kernel commandline is that the entropy is restored very early in the boot process, early in the start_kernel() method.

I was about to argue that spending time on implementing this might not be worth it, because it would be good enough if we do it in the initramfs premount stage. During my work on #15292, I noticed that the kernel prints warning messages like this if urandom is read before it's initialized:

random: sgdisk: uninitialized urandom read (16 bytes read)

Without the code for #15292, there are no such warnings, and this code is executed in the premount initramfs stage, so it should be enough to initialize the random seed before that. So I thought. But actually there is another message, much earlier during boot, which also warns that the RNG is being used before it's initialized:

random: get_random_bytes called from start_kernel+0x94/0x52e with crng_init=0

Further messages like these are probably omitted (it depends on whether a macro is defined or not, I don't want to spend time on figuring out when it is defined). I also don't want to spend time on figuring out whether these random bytes are used for something security critical or not. Instead, I will try to implement what I described above.

#41 Updated by intrigeri 11 months ago

  • During shutdown, also replace /EFI/BOOT/random.cfg with a new random string.

If that's too hard to implement, maybe we can skip that.

[…] I don't want to spend time on figuring out when it is defined). I also don't want to spend time on figuring out whether these random bytes are used for something security critical or not. Instead, I will try to implement what I described above.

Makes sense to me!

#42 Updated by segfault 11 months ago

I don't think we will be able to get rid of these warnings. Neither the entropy added from the command-line (add_device_randomness()), nor the bytes written to /dev/urandom (random_write()) are "credited" as entropy - i.e. the functions that mix the bytes in the entropy pool do not call credit_entropy_bits(). This means that the entropy is not counted towards the 128 bits required to mark the RNG as initialized.

So both methods do increase the quality of the RNG, but the kernel doesn't know that we actually added good entropy, so it doesn't mark the RNG as initialized.

#43 Updated by segfault 11 months ago

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

#44 Updated by segfault 11 months ago

Restoring the whole entropy pool (512 Bytes) via the kernel command-line requires adding a long random string (1024 if we use hex, 683 characters if we use base64 - which might cause issues if the string by chance includes valid kernel parameters). This is quite annoying when you edit the command-line in the boot menu.

#45 Updated by segfault 11 months ago

intrigeri wrote:

  • During shutdown, also replace /EFI/BOOT/random.cfg with a new random string.

If that's too hard to implement, maybe we can skip that.

If we would skip that, we would not persist any of the entropy gathered during a Tails session. We would only keep the entropy from the random seed created by Tails Installer - which we will no longer have once #15292 is done.

#46 Updated by bertagaz 11 months ago

Nice to see this going forward. As a side note, it's usually best practice to ask to the ticket owner before taking over, specially if she wasn't there during a discussion and there were maybe money involved. But I get I did not react much lately on this (MIA, and focussing on the blueprint first to get a clear plan, as we were not so many to work on it), and there were no grants for this anyway.

#47 Updated by u 11 months ago

bertagaz wrote:

Nice to see this going forward. As a side note, it's usually best practice to ask to the ticket owner before taking over, specially if she wasn't there during a discussion and there were maybe money involved. But I get I did not react much lately on this (MIA, and focussing on the blueprint first to get a clear plan, as we were not so many to work on it), and there were no grants for this anyway.

I agree that when a ticket is assigned to someone, it's good practice to talk to them before taking it over. But in this case, it seems that the three of you have signed up to work in a team and unfortunately Redmine allows to encode ownership only to a single person and this was not marked on the ticket explicitly, and only marked in the notes of our roadmap session.

I'm remembered of a talk at DebConf where somebody said something along the lines "in Debian, if you don't look and leave people alone you can be sure they'll make something great." This ticket's history seems to confirm such dynamics :) It looks to me like things have been moving forward a lot in the last 9 months, since this ticket was taken over by segfault & kurono, so maybe we should thank them instead :) Thanks!

#48 Updated by bertagaz 11 months ago

u wrote:

I agree that when a ticket is assigned to someone, it's good practice to talk to them before taking it over. But in this case, it seems that the three of you have signed up to work in a team and unfortunately Redmine allows to encode ownership only to a single person and this was not marked on the ticket explicitly, and only marked in the notes of our roadmap session.

I'm remembered of a talk at DebConf where somebody said something along the lines "in Debian, if you don't look and leave people alone you can be sure they'll make something great." This ticket's history seems to confirm such dynamics :) It looks to me like things have been moving forward a lot in the last 9 months, since this ticket was taken over by segfault & kurono, so maybe we should thank them instead :) Thanks!

In case I've not been clear in what I meant: I do not want to ashame $someone and I'm happy to see this going forward. I'm thankfull to segfault and kurono for that, and I know I've not been responsive enough to get this ticket further since some times.

Now as you mention, we're talking about "team". I'm just raising the other part of the mixed feelings I had seeing this ticket being worked on, mentioning a discussion I had no clue and no notifications before this ticket was taken over, despite being member of that team. That goes badly with the feeling to have to struggle to get people working on a (still unfinished and to be reviewed) blueprint before jumping into the code. Sorry but I don't think it's a redmine encoding problem.

Now that I've hopefully clarified what my feelings are, please go forward and don't wait for me, I don't feel attached to this team anymore. I'm not sure to like the "just let people do" way of building a team though, but if that's the way to go, I'll try to catch on it.

#49 Updated by kurono 7 months ago

bertagaz wrote:

...
Now that I've hopefully clarified what my feelings are, please go forward and don't wait for me, I don't feel attached to this team anymore. I'm not sure to like the "just let people do" way of building a team though, but if that's the way to go, I'll try to catch on it.

I'm really sorry with bertagaz. I guess we left ourselves get carried by the excitement of having this implemented. On the other hand, I was always assigned to this ticket (I proposed it), and I ask segfault to take it over. But sure, we should have waited for bertagaz, since he also wanted to help with this. Also, there was never money involved here.

#50 Updated by intrigeri 7 months ago

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

#51 Updated by intrigeri 7 months ago

The "FYI/RFC: early-rng-init-tools" thread on debian-devel@ is related to what we're doing. I think anyone working on this (including reviewing the branch once it's deemed ready by its developers) should read it to check that our implementation does not fall into the traps identified there :)

#52 Updated by intrigeri 4 months ago

  • QA Check deleted (Info Needed)

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

#53 Updated by segfault 2 months ago

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

#54 Updated by segfault 2 months ago

I continued working on this, and I think it's close to being ready for review. A few remarks:

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.

Restoring the whole entropy pool (512 Bytes) via the kernel command-line requires adding a long random string (1024 if we use hex, 683 characters if we use base64 - which might cause issues if the string by chance includes valid kernel parameters). This is quite annoying when you edit the command-line in the boot menu.

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.

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 #16891). We will have to write a small C proram for that.

And lastly, we might want to rename this issue to better describe the main advantage of the work done here: To persist the entropy pool, even if no persistent volume was created (in contrast to #7675). The Tails Installer bits are less relevant now, because the recommended way to install Tails on a USB is the USB Image, not Tails Installer. I would still keep the Tails Installer bits, because they are finished and we still support installing from a Tails ISO via the Tails Installer (from within an existing Tails at least).

#55 Updated by segfault about 2 months ago

  • 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 about 1 month ago

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

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

#57 Updated by intrigeri about 1 month ago

  • 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 about 1 month ago

  • Description updated (diff)

#59 Updated by intrigeri about 1 month ago

#60 Updated by intrigeri about 1 month ago

#61 Updated by cypherpunks about 1 month ago

Restoring the whole entropy pool (512 Bytes) via the kernel command-line requires adding a long random string (1024 if we use hex, 683 characters if we use base64 - which might cause issues if the string by chance includes valid kernel parameters). This is quite annoying when you edit the command-line in the boot menu.

You don't need to "restore" the whole 512 bytes. Using 32 bytes (or even 16) is plenty.

#62 Updated by segfault about 1 month ago

cypherpunks wrote:

You don't need to "restore" the whole 512 bytes. Using 32 bytes (or even 16) is plenty.

Some explanation would help. My sources for using 512 bytes are https://linux.die.net/man/4/urandom and https://github.com/torvalds/linux/blob/master/drivers/char/random.c#L162. Of course restoring any entropy at all is better than none, but why do you think 32 or 16 would be "plenty"?

#63 Updated by segfault about 1 month ago

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

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:

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

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

#64 Updated by segfault about 1 month ago

  • Description updated (diff)

#65 Updated by segfault about 1 month ago

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

Or maybe I'll just reply here, because I don't think my points are relevant for tails-dev subscribers.

Thanks for updating the blueprint! If you agree, I would make these changes to it:

  • Note in the "Random value on the kernel command-line" section that the entropy added here isn't credited and therefore doesn't increase /proc/sys/kernel/random/entropy_avail and doesn't cause the entropy pool to be marked as initialized.
  • Note in "Random value stored in an unused sector" that here we could use a custom program to credit the added entropy.
  • Add a section "Block creation of persistence until enough entropy has been gathered", referencing #16891 (I think this would be a much better solution than blocking boot).

Btw, in case you missed it, here is a piece on random seeds by systemd developers, which was published recently: https://systemd.io/RANDOM_SEEDS.html

It contains a lot of interesting information, even though I don't think systemd's entropy initialization will help us.

#66 Updated by segfault about 1 month ago

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 see a reason now: The random seed could have been generated from low entropy itself. So maybe we should also store the content of /proc/sys/kernel/random/entropy_avail and only credit up to that amount. Does this make sense?

#67 Updated by cypherpunks about 1 month ago

Some explanation would help. My sources for using 512 bytes are https://linux.die.net/man/4/urandom and https://github.com/torvalds/linux/blob/master/drivers/char/random.c#L162. Of course restoring any entropy at all is better than none, but why do you think 32 or 16 would be "plenty"?

The pool is 512 bytes in size, but you only need 32 or 16 bytes (256 or 128 bits, respectively) to achieve cryptographic security. In fact, /dev/urandom since Linux 4.8 uses ChaCha20 with a 256 bit (32 byte) seed.

#68 Updated by cypherpunks about 1 month ago

I see a reason now: The random seed could have been generated from low entropy itself. So maybe we should also store the content of /proc/sys/kernel/random/entropy_avail and only credit up to that amount. Does this make sense?

Never credit public entropy! The standard urandom scripts don't do that. Any entropy which is not collected from a secret source (e.g. precise interrupt timings) should never be credited. That doesn't mean that it won't be mixed in, just that the kernel won't assume that the input is entirely secret and potentially unblock even before sufficient entropy is collected.

#69 Updated by cypherpunks about 1 month ago

As for why the seed is 512 bytes, that is primarily because sectors are typically 512 bytes, so filling up an entire sector ensures that nothing "goes to waste". After all, it doesn't hurt, and it gives devs a warm fuzzy feeling to think that they might be saving more than 256 bits of entropy.

But seriously though... don't credit those bits.

#70 Updated by cypherpunks about 1 month ago

I see a reason now: The random seed could have been generated from low entropy itself. So maybe we should also store the content of /proc/sys/kernel/random/entropy_avail and only credit up to that amount. Does this make sense?

That's not a good idea. It takes some time for entropy in the input pool to "trickle down" to the secondary pools. This causes entropy to be effectively reset if the system is powered up then powered down with less than a few minutes of uptime. This is made worse by the kernel's odd handling of kernel.random.entropy_avail. One solution would be to hash the previous seed with randomness from the kernel, and save the result in a new seed. But regardless, crediting entropy allows an attacker to potentially predict the LUKS master key (or any other secrets) if it is generated shortly after boot, as the system would not block because it thinks it has enough entropy, but it doesn't.

There's a very good reason why entropy in the kernel command line, and any entropy added from add_device_randomness(), is not credited.

#71 Updated by segfault about 1 month ago

cypherpunks wrote:

I see a reason now: The random seed could have been generated from low entropy itself. So maybe we should also store the content of /proc/sys/kernel/random/entropy_avail and only credit up to that amount. Does this make sense?

Never credit public entropy! The standard urandom scripts don't do that. Any entropy which is not collected from a secret source (e.g. precise interrupt timings) should never be credited. That doesn't mean that it won't be mixed in, just that the kernel won't assume that the input is entirely secret and potentially unblock even before sufficient entropy is collected.

Someone able to read the seed stored in an unused sector on the device would require either root access inside Tails or physical access to the Tails device. In either case, we can't protect any secrets created on the device after someone had this kind of access (root can just read it, someone with physical access can modify Tails and the boot code). So I don't see why we shouldn't credit this entropy.

Also, systemd advises to credit the entropy added by the random seed stored in /var/lib/systemd/random-seed, it's only disabled by default to avoid static seed shipped in OS images to be credited (see https://systemd.io/RANDOM_SEEDS.html).

#72 Updated by cypherpunks about 1 month ago

So I don't see why we shouldn't credit this entropy.

Because someone can retroactively determine the entropy pool contents (or a subset). Do you really want someone to be able to grab the flash drive, then from the stored seed, reconstruct a reduced keyspace for brute forcing the LUKS device?

#73 Updated by intrigeri about 1 month ago

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

#74 Updated by intrigeri about 1 month ago

segfault wrote:

Yes, my proposal is to bring back the "store the seed in an unused sector" functionality.
[...]
I don't think we can, and even then I would argue that we should also use the seed in an unused sector: […]

OK, makes sense to me, go ahead whenever you feel like it :)

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

OK, so that's another next step.

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

Agreed.

OK, let's move the discussion about crediting entropy or not to #16977.

#75 Updated by intrigeri about 1 month ago

segfault wrote:

Thanks for updating the blueprint! If you agree, I would make these changes to it:

  • Note in the "Random value on the kernel command-line" section that the entropy added here isn't credited and therefore doesn't increase /proc/sys/kernel/random/entropy_avail and doesn't cause the entropy pool to be marked as initialized.

OK, adding it to the blueprint should help us remember to move it from the blueprint to the design doc on this branch before the final review'n'merge rounds.

  • Note in "Random value stored in an unused sector" that here we could use a custom program to credit the added entropy.

Agreed, perhaps in a "Future improvements" section, with a pointer to #16977.

  • Add a section "Block creation of persistence until enough entropy has been gathered", referencing #16891 (I think this would be a much better solution than blocking boot).

OK, in the "Future improvements" section too.

Btw, in case you missed it, here is a piece on random seeds by systemd developers, which was published recently: https://systemd.io/RANDOM_SEEDS.html

I did miss it. This seems to be a good candidate for the "Also see" section of the blueprint :)

#76 Updated by intrigeri about 1 month ago

cypherpunks wrote:

So I don't see why we shouldn't credit this entropy.

Because someone can retroactively determine the entropy pool contents (or a subset). Do you really want someone to be able to grab the flash drive, then from the stored seed, reconstruct a reduced keyspace for brute forcing the LUKS device?

Thank you, cypherpunks, for chiming in! Let's continue this part of the discussion on #16977: segfault and I have agreed that our first iteration will not credit this entropy.

#77 Updated by segfault about 1 month ago

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.

By repeatedly dividing the length of the random string by 2, the first length that doesn't break the syslinux menu is 32, i.e. 16 Bytes.

#78 Updated by cypherpunks about 1 month ago

16 bytes is 128 bits, and 2^128 is a very huge keyspace. Although 32 bytes is often more recommended because it transitions the difficulty of exhaustive search from "impossible without using up the entire energy of the sun" to "impossible before the heat death of the universe". I wouldn't sweat 16 bytes if that's all syslinux can handle. You could get a few more bytes with a different encoding, e.g. base64 but with = replaced with something else so it can't conflict with kernel options, or you could start it with "seed=<base64 goes here>" so that even the = sign won't cause a conflict.

#79 Updated by cypherpunks about 1 month ago

If you use base64 with "seed=", you can get 24 bytes (192 bits) using only 38 characters. If that's still too much and it needs to be under 32 characters, then you can get 18 bytes (144 bits). It would look like:

seed=FrOUuC4Xn6BMnT4g5VfIp0QR

You need the "seed=" to disambiguate it so there are no conflicts. Base64 is better than hex for that.

#80 Updated by segfault about 1 month ago

Thanks cypherpunks. Yes, we could increase the seed size if we used base64 (I also proposed that somewhere above). And we do prefix the seed with randomseed=. But I think it should be fine to just use the 16 Bytes seed for now - especially since we still add the 512 Bytes in the initramfs. As discussed with intrigeri, we can increase the size once we migrate to GRUB.

What do you think, @intrigeri?

#81 Updated by intrigeri about 1 month ago

What do you think, intrigeri?

Fine, let's not bother now and instead file a ticket (blocked by #15806) to increase this once we're using GRUB for all supported USB boot methods.

#82 Updated by segfault about 1 month ago

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.

#83 Updated by segfault about 1 month ago

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

#84 Updated by segfault about 1 month ago

intrigeri wrote:

What do you think, intrigeri?

Fine, let's not bother now and instead file a ticket (blocked by #15806) to increase this once we're using GRUB for all supported USB boot methods.

Done, see #16980.

#85 Updated by cypherpunks about 1 month ago

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.

#86 Updated by segfault about 1 month ago

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

#87 Updated by intrigeri about 1 month ago

Hi @segfault,

segfault wrote:

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

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?

#88 Updated by segfault 26 days ago

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

#89 Updated by intrigeri 26 days ago

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.

Quite the opposite, I think this is very sensible of you, and I greatly appreciate you're considering the big picture :)

#90 Updated by segfault 12 days ago

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.

In order to obfuscate the number of boots, I propose that, during first boot, we repeatedly write a random string to the random seed sector, a randomly chosen number of times (maybe between 1000 and 2000 if that doesn't take too long). I asked someone who knows more about this stuff and they said that it would probably work to obfuscate the number of boots this way.

Also available in: Atom PDF