Page MenuHomeFreeBSD

Add /boot/entropy at install time, and be more careful with permissions
ClosedPublic

Authored by dteske on Oct 18 2015, 8:10 PM.
Tags
None
Referenced Files
F103261428: D3933.id9933.diff
Fri, Nov 22, 6:45 PM
Unknown Object (File)
Wed, Oct 30, 4:04 AM
Unknown Object (File)
Sat, Oct 26, 6:32 AM
Unknown Object (File)
Oct 12 2024, 4:49 AM
Unknown Object (File)
Sep 27 2024, 5:24 AM
Unknown Object (File)
Sep 18 2024, 6:01 PM
Unknown Object (File)
Sep 15 2024, 7:03 PM
Unknown Object (File)
Sep 15 2024, 5:25 AM
Subscribers

Details

Reviewers
jmg
markm
delphij
Group Reviewers
secteam
Summary

Now that /boot/entropy is loaded by loader at boot time, we should generate this file also at install time.

We were also not being explicit about permissions on these files, so make sure that if umask or other things changes, that these will have correct permissions. We should probably also updated the rc.d scripts to do the same incase they are created by hand.

In the case /entropy is created by hand w/ incorrect permissions, no script fixes this.

Related: https://github.com/HardenedBSD/hardenedBSD/issues/167

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 1021
Build 1024: arc lint + arc unit

Event Timeline

jmg retitled this revision from to Add /boot/entropy at install time, and be more careful with permissions.
jmg updated this object.
jmg edited the test plan for this revision. (Show Details)
jmg added reviewers: delphij, secteam, markm, dteske.
jmg set the repository for this revision to rS FreeBSD src repository - subversion.
markm edited edge metadata.

This was on my TODO list. I wish I'd known it was this simple!

usr.sbin/bsdinstall/scripts/entropy
29โ€“33

The double quotes are superfluous, but not a particularly big deal.

This revision is now accepted and ready to land.Oct 18 2015, 8:41 PM

I had thought that we weren't writing out even /entropy files, but @op pointed me to this file.

usr.sbin/bsdinstall/scripts/entropy
29โ€“33

only if BSDINSTALL_CHROOT is guaranteed to never have spaces in it, and as this is passed in externally, I prefer not to make that assumption.

Well, I attempted to test this, and I don't believe that this file is being called at the end of install.

I d/l'd FreeBSD-11.0-CURRENT-amd64-20151016-r289420-disc1.iso, and put the new entropy file in the iso manually via:
strings -td FreeBSD-11.0-CURRENT-amd64-20151016-r289420-disc1.iso | grep 'BSDINSTALL_CHROOT/entropy'
echo $((659639709/2048))

Then:
dd if=FreeBSD-11.0-CURRENT-amd64-20151016-r289420-disc1.iso bs=2k iseek=322089 count=1 | hexdump -C | less
to verify that I had the correct file,

and then:
dd if=/tmp/entropy conv=notrunc of=FreeBSD-11.0-CURRENT-amd64-20151016-r289420-disc1.iso bs=2k oseek=322089 count=1

to overwrite the file...

I then used bhyve to do an install:
truncate -s 4g 11snap.img
sh /usr/share/examples/bhyve/vmrun.sh -m 4g -d 11snap.img -I FreeBSD-11.0-CURRENT-amd64-20151016-r289420-disc1.iso 11snap

Then selected all the defaults for an install, and when it was done and rebooted, I booted single user mode, and inspected the fs, and found that neither /entropy nor /boot/entropy were created.

jmg edited edge metadata.
  • add /boot/entropy to install generation.. Also, be extra careful
  • add quotes around the $i for more space protection..

I have done a basic test, and this looks good, I'll commit shortly..

This revision now requires review to proceed.Oct 19 2015, 1:13 AM

though using dd to put it in an image works, it doesn't change the file size, so I needed to delete some comments, and add white space... once I did that, I was able to verify that the script ran fine...

markm edited edge metadata.

I'm happy.

This revision is now accepted and ready to land.Oct 19 2015, 7:01 AM
In D3933#81686, @markm wrote:

This was on my TODO list. I wish I'd known it was this simple!

Could you please share your TODO list somewhere?

In D3933#81711, @jmg wrote:
  • add /boot/entropy to install generation.. Also, be extra careful
  • add quotes around the $i for more space protection..

I have done a basic test, and this looks good, I'll commit shortly..

We have one more issue in relation to this bug: https://github.com/HardenedBSD/hardenedBSD/commit/73e53f0af947e9ad77653f44ef99a2d66394789c

Possible the current candidate supersede this change. I will test them today.

delphij requested changes to this revision.Oct 29 2015, 4:26 PM
delphij edited edge metadata.

Perhaps we can use umask instead?

Also do we really need bsdinstall entropy when creating a jail? (Note that /etc/rc.d/random have nojail in it, so it's not actually being used there).

usr.sbin/bsdinstall/scripts/entropy
32

Instead of this can we just do umask 077 before the dd and umask 022 after it?

This revision now requires changes to proceed.Oct 29 2015, 4:26 PM

for jails we don't need to create the file, but I don't see any harm in having this file created... I looked briefly at the jail script, and I don't see a good way to turn it off in the jail case...

usr.sbin/bsdinstall/scripts/entropy
32

hmm.. We should just do umask 077 and not restore after.. I just confirmed w/ dteske that these scripts are executed, not sourced.

jmg edited edge metadata.
  • add /boot/entropy to install generation.. Also, be extra careful
  • add quotes around the $i for more space protection..
  • use umask per delphij...
jmg marked 4 inline comments as done.Nov 4 2015, 12:05 AM

mark various parts done

dteske edited reviewers, added: jmg; removed: dteske.

Take

dteske edited edge metadata.

Remove unnecessary sub-shell (bsdinstall verbs are exec'd;
no need to protect environment from umask effects).

jmg edited reviewers, added: dteske; removed: jmg.

take back so I can update and confirm my change is same..

  • add /boot/entropy to install generation.. Also, be extra careful
  • add quotes around the $i for more space protection..
  • use umask per delphij...
  • dteske says the parens are not needed.. remove them..
  • add /boot/entropy to install generation.. Also, be extra careful
  • add quotes around the $i for more space protection..
  • use umask per delphij...
  • dteske says the parens are not needed.. remove them..
  • pull in dteske's version..
dteske edited reviewers, added: jmg; removed: dteske.

So sorry to commandeer again; but I have to fix a mistake in my previous code which was written hastily during the conference

Please re-commandeer. Sorry for prior misgiving (refactoring was ``off'' for chown(8) at end of for-loop).

delphij edited edge metadata.

Looks good to me.

This revision is now accepted and ready to land.Nov 4 2015, 4:56 AM
markm edited edge metadata.

I'm still Happy!

Any update on this change?

Hrm I thought Devin have already integrated this weeks ago but a quick glance suggests it's not. Devin, looks like all issues was resolved already?

See prior comment; I asked jmg to re-commandeer for the commit. I only commandeered it so I could update it -- had no intention of truly owning it and wanted jmg to drive it.

But... it looks like jmg is relinquishing to me by way of implicit ETIMEOUT on taking-back.
I'll go ahead and drive this in (most likely by EOD today).

In D3933#89226, @dteske wrote:

See prior comment; I asked jmg to re-commandeer for the commit. I only commandeered it so I could update it -- had no intention of truly owning it and wanted jmg to drive it.

But... it looks like jmg is relinquishing to me by way of implicit ETIMEOUT on taking-back.
I'll go ahead and drive this in (most likely by EOD today).

Ping? :) @dteske @jmg

Committed to HEAD as SVN r292832 -- will merge to stable/{9,10} after MFC timeout