Re: [PATCH 1/2] plainmount: Support decryption of devices encrypted in plain mode.

[Glenn Washburn]

On Mon, 31 Jan 2022 17:40:24 +0000
Maxim Fomin <maxim@fomin.one> wrote:

> ------- Original Message -------
> 
> On Monday, January 31st, 2022 at 14:15, Milan Broz <gmazyland@gmail.com> 
> wrote:
> 
> > On 30/01/2022 20:40, Maxim Fomin wrote:
> >
> > > This patch introduces support for plain encryption mode (plain dm-crypt) 
> > > via
> > >
> > > new module and command named 'plainmount'. The command allows to open 
> > > devices
> > >
> > > encrypted in plain mode (without LUKS) with following syntax:
> > >
> > > +
> >
> > ...
> >
> > > +#define GRUB_PLAINMOUNT_UUID "00000000000000000000000000000000"
> > >
> > > +#define GRUB_PLAINMOUNT_CIPHER "aes-cbc-essiv:sha256"
> > >
> > > +#define GRUB_PLAINMOUNT_DIGEST "ripemd160"
> > >
> > > +#define GRUB_PLAINMOUNT_KEY_SIZE 256
> > >
> > > +#define GRUB_PLAINMOUNT_SECTOR_SIZE 512
> >
> > Sooner or later we will have to change this default in cryptsetup
> >
> > (as ripemd and CBC mode are no longer the best options) and you
> >
> > you will create data corruption here (as there is no way in plain
> >
> > mode to check that the mode is set correctly).
> >
> > Not sure if it is possible, but in normal system it should be required
> >
> > that these parameters are set in /etc/crypttab, grub should perhaps
> >
> > require explicit setting them in config too?
> >
> > Milan
> 
> These default values can be fixed or removed altogether. Some additional 
> points.
> 
> 1. Just to clarify - mounting device in plain mode with wrong parameters (it 
> is irrelevant
> whether default values or explicitly set arguments are wrong) does not case 
> data corruption
> per se, data corruption requires writing to such device. And writing to 
> unformatted device
> is complicated by the fact that no command operating on files will work with 
> such device.
> User can corrupt data only if he uses some grub command which has 
> functionality to write
> arbitrary byte data to arbitrary device (like 'dd' command) at arbitrary 
> device offset.

I agree. I don't think data corruption is a big issue because GRUB
doesn't (generally) write to disks.

> 2. Still, solving this problem may be possible. After decryption additional 
> check can be added -
> test whether some fs is recognized on target device (or some additional 
> virtual device like
> 'lvm' appeared). I will investigate whether this is technically possible and 
> add such checks in
> the next version of the patch.

I thought about this too, but I don't think the lack of a filesystem or
structured data should indicate that the decryption key was bad. It
could be another plainmount or random key data.

> 3. The question of how to keep settings (on live system) is still open 
> because plainmount
> command is not supported by grub-mkconfig yet. I agree that support in 
> grub-mkconfig must
> require explicit parameter setting. These parameters can be specified in 
> /etc/default/grub
> like GRUB_CRYPTODISK_PLAINMOUNT_CIPHER=xxx (and similar for additional 
> parameters). Then
> grub-mkconfig must be changed to recognize such variables.

I think this can be a separate patch series and that grub-mkconfig
should not support plainmount until such a change is made. So it must
be configured by hand. I think this is fine for the niche audience that
will use this.

> Currently plainmount just processes arguments - whether they were generated 
> by mkconfig and
> came from /etc/default/grub (hypothetically) or user directly edited grub.cfg 
> is not in scope.
> 
> What can be done at this step is removing default arguments for 
> cipher/hash/key size
> - this will stimulate user to check arguments correctness. Under current 
> implementation plainmount
> will silently create incorrect disk when these parameters are omiited, so 
> such change will
> impove data safety. Also, from my understanding these cipher/hash types are 
> used rarely nowadays,
> (meaning these default values will likely not be used anyway) so this is 
> additional argument
> to remove them.

I'm fine with default arguments, _if_ the defaults are common (eg. if
cryptsetup has defaults use those). As a user of plainmount, I think
I'd want to always be explicit incase the defaults changed. Of course,
not having defaults is okay too.

Glenn