Page MenuHomeFreeBSD

Add option to geli allowing to hide passphrase darning boot.
ClosedPublic

Authored by oshogbo on Jul 27 2017, 2:42 PM.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

oshogbo created this revision.Jul 27 2017, 2:42 PM
allanjude edited edge metadata.Jul 27 2017, 4:46 PM

I have not had a chance to test this code yet, but reading it over it looks good to me.

We need to decide how to de-conflict with the sysctl:
kern.geom.eli.visible_passphrase: Visibility of passphrase prompt (0 = invisible, 1 = visible, 2 = asterisk)

sys/geom/eli/g_eli.c
1116 ↗(On Diff #31256)

This should be overridden by the new flag, should it not?

cem edited edge metadata.Jul 27 2017, 6:21 PM

Can you undo the whitespace change from geliboot.c? Phabricator doesn't seem to have a good option to hide white space changes and it detracts from reviewing the (security sensitive) functional change.

oshogbo updated this revision to Diff 31298.Jul 28 2017, 7:56 AM

I have not had a chance to test this code yet, but reading it over it looks good to me.

We need to decide how to de-conflict with the sysctl:
kern.geom.eli.visible_passphrase: Visibility of passphrase prompt (0 = invisible, 1 = visible, 2 = asterisk)

Thanks I miss that!
I thing we should go from the specific case (In our case the disk) to the general options (sysctl).
So for me the most important option is the option per disk.
I submited change.

In D11751#243637, @cem wrote:

Can you undo the whitespace change from geliboot.c? Phabricator doesn't seem to have a good option to hide white space changes and it detracts from reviewing the (security sensitive) functional change.

Bad Pharbricator ;/
For the purpose of review I revert the style changes in the geli_attach function.
Insted of changing whole style I left extra brackets where SLIST_FOREACH_SAFE was.
I hope this make it more readable.

cem accepted this revision.Jul 28 2017, 6:03 PM

Thanks!

sys/boot/geli/geliboot.c
430 ↗(On Diff #31298)

Should this be explicitly converted to boolean?

This revision is now accepted and ready to land.Jul 28 2017, 6:03 PM
allanjude accepted this revision.Jul 28 2017, 6:14 PM
oshogbo updated this revision to Diff 31904.Aug 10 2017, 9:34 PM
oshogbo edited edge metadata.

Hi :)

I know the review passed correctly but I had some issue with this patch.
I think by default we should present the most secure version of the system.
So by default I would disable the stars during the boot and I changed the flag to allow to show stars.

I know that this functionality is already in the release but I feel much bather doing it that way.

I hope you will agree with me :)

This revision now requires review to proceed.Aug 10 2017, 9:34 PM
allanjude accepted this revision.Aug 15 2017, 6:09 PM

We'll need to make sure to note the change in the release notes and the UPDATING file then.

This revision is now accepted and ready to land.Aug 15 2017, 6:09 PM
oshogbo updated this revision to Diff 32136.Aug 16 2017, 7:03 PM
oshogbo edited edge metadata.

Add UPDATING note.

This revision now requires review to proceed.Aug 16 2017, 7:03 PM

If it is going to be on by default, it should be enabled even in the absence of -h when you do geli init. And we need a -H flag for init/label to be able to disable it.

sbin/geom/class/eli/geli.8
299 ↗(On Diff #32136)

if this is the default, then should init only have the option to disable it? Or at least have both?

506 ↗(On Diff #32136)

maybe example on this to to mention printing of * characters

sbin/geom/class/eli/geom_eli.c
963 ↗(On Diff #32136)

so if hidepass is -1 (not specified), it is not enabled by default? I thought you wanted it on by default?

oshogbo updated this revision to Diff 32210.Aug 18 2017, 5:30 PM

Ugh wrong patch applied ;/

allanjude added inline comments.Aug 18 2017, 5:53 PM
sbin/geom/class/eli/geli.8
91 ↗(On Diff #32210)

this is Dd now?

sbin/geom/class/eli/geom_eli.c
1037 ↗(On Diff #32210)

this is -d and -D now?

oshogbo updated this revision to Diff 32230.Aug 19 2017, 12:01 AM

Spelling fixes by AllanJude.

allanjude accepted this revision.Aug 21 2017, 8:16 PM
allanjude added inline comments.
sbin/geom/class/eli/geom_eli.c
123 ↗(On Diff #32230)

h should be d

This revision is now accepted and ready to land.Aug 21 2017, 8:16 PM
This revision was automatically updated to reflect the committed changes.
wblock added inline comments.Sep 1 2017, 7:56 PM
head/UPDATING
55

This is not clear.

The geli password typed at boot is now hidden.  To restore the previous
behavior, see geli(8) for configuration options.
head/sbin/geom/class/eli/geli.8
284

It is really hard to tell what this means. Guessing:

When entering the passphrase to boot from this
encrypted root filesystem, echo
.Ql *
characters.
This makes the length of the passphrase visible.
496

Why is this being repeated? See above for a somewhat more clear version.

500

Maybe

Disable echoing of any characters when a passphrase is entered
to boot from this encrypted root filesystem.
This hides the passphrase length.

@wblock Thank you! Sorry for my poor language ;( Fixed in the r323671.