Re: [PATCH v3 0/3] Cryptomount detached headers

[brutser--- via Grub-devel]

See below.


Van: Glenn Washburn 
Aan: brutser--- via Grub-devel 
Onderwerp: Re: [PATCH v3 0/3] Cryptomount detached headers
Datum: 05/08/2022 07:00:51 Europe/Paris
Cc: brut...@perso.be;
   dki...@net-space.pl;
   p...@pks.im

On Thu, 4 Aug 2022 18:56:40 +0200 (CEST)
brutser--- via Grub-devel  wrote:

> More testing done:
> 
> the problem most likely is triggered when encrypting partitions (see all my 
> other comments to get a better picture).
> 
> I tested encrypting a complete device /dev/sdb with detached header and this 
> worked fine, no issues, with logical volumes created.

Ok, this is progress. It looks like there may be an issue with using
partitions. I haven't tested that.

> 
> 
> 
> So the problem now is consistent, coreboot, or no coreboot, when encrypting a 
> partition, in my tests /dev/sda2, with a detached header, you cannot decrypt 
> successfully using cryptomount.
> 
> 
> 
> cryptomount -H (hd0,msdos1)/header.bin (hd0,msdos2)

What if you do this instead:

insmod loopback
loopback loop (hd0,msdos2)+
cryptomount -H (hd0,msdos1)/header.bin (loop)

Does this give the same error?

No, this will work normal, ask the passphrase and decrypt the partition!

But when setting linux kernel, this error appears:
attempt to read or write outside of disk 'crypto0'

> 
> will ALWAYS result in the error: 
> 
> 
> 
> "no cryptodisk module can handle this device"
> 
> 
> 
> Strange is that it seems to be possible to ignore the partition and simply 
> try to 'decrypt the device', which should not be possible of course, but it 
> validates the passphrase (as seen below in comments), but
> 
> although it opens a crypto0 slot, this is not usable decrypted data.

This is expected behavior. As long as you enter a correct password or
use a keyfile that is valid for one of the key slots in the detached
header, then it doesn't matter what disk device you give to
cryptomount, it will always be decrypted "successfully". But it will
not be usable unless it is the right block device.

>>> Ok makes sense.

You still never answered my question about what exact QEMU commandline
options you were using

>>> sudo qemu-system-x86_64 -serial stdio -drive 
>>> file=~/disk2.img,format=raw,index=0,media=disk -m 3840 -boot d -M q35 
>>> --accel kvm

, nor if you'd tried to use the options I
suggested for getting the serial output as text. 

>>> yes I tried, but I still had no idea how to get the output and since there 
>>> was a little progress, I didn't want to spend too much time on it:
qemu-system-x86_64: -fw_cfg name=etc/sercon-port,string=0: warning: externally 
provided fw_cfg item names should be prefixed with "opt/"
but i guess it makes it much easier if I can copy the debug logs, so maybe we 
should make this work, can you explain how i can get the logs more detailed?

Also, you never put in
the debug log message for the read hook in cryptodisk like I asked.

>>> yes, I somehow did not read that part, so I will do that right now.

Glenn

> 
> 
> 
> 
> 
> 
> 
> Debug logs: https://imgur.com/a/onRBSMd
> 
> 
> 
> New comments below.
> 
> See comments below.
> 
> 
> Van: Glenn Washburn 
> Aan: brutser--- via Grub-devel 
> Onderwerp: Re: [PATCH v3 0/3] Cryptomount detached headers
> Datum: 03/08/2022 21:54:14 Europe/Paris
> Cc: brut...@perso.be;
>    dki...@net-space.pl;
>    p...@pks.im
> 
> On Tue, 2 Aug 2022 22:49:27 +0200 (CEST)
> brutser--- via Grub-devel  wrote:
> 
> > 
> > Van: Glenn Washburn 
> > Aan: brutser--- via Grub-devel 
> > Onderwerp: Re: [PATCH v3 0/3] Cryptomount detached headers
> > Datum: 02/08/2022 20:58:17 Europe/Paris
> > Cc: brut...@perso.be;
> >    dki...@net-space.pl;
> >    p...@pks.im
> > 
> > Hi Bruster,
> > 
> > Are you able to add your responses inline like I have been doing in my
> > replies? And not top-post, which is posting at the top.
> > 
> > I will reply here, though my mail client is not really doing a great job to 
> > get this inline response done, sorry for that.
> 
> Ok, I figured that. However, you've not responded to much of my
> comments that I wrote inline. Perhaps you only read the first part of
> my response and do not realize that I wrote more response further down.
> Please look at the full email until you see my name, which indicates
> that I am finished responding. Please respond to all of my questions,
> otherwise it makes it difficult to help you and if you don't take the
> time to respond to me fully I am less inclined to help. I would like to
> get this issue sorted out, but I need you to help me debug it right now.
> 
> Also, I have never built GRUB with coreboot, so I don't have a way to
> precisely reproduce your setup anyway. And it currently does work for
> me.
> 
> Glenn
> 
> >>> Comments:
> I think I read all your comments and tried to reply as best as possible.
> 
> I tried the following test to eliminate the coreboot issue >>
> 
> Qemu installation:
> 
> 1. Minimal Debian 11.4 installation on /dev/sda1
> 2. git clone latest grub, make & make install, grub-inst

Re: Linux DRTM on UEFI platforms

[Ard Biesheuvel]

On Tue, 5 Jul 2022 at 21:22, Daniel P. Smith
 wrote:
>
> On 6/10/22 12:40, Ard Biesheuvel wrote:
...
> > The EFI stub to core kernel handover ABI is private to Linux,
> > architecture specific, and subject to change. This is basically the
> > point I made before: if you want to boot Linux in EFI mode, you have
> > to go via the EFI stub, and the EFI stub is in charge of booting the
> > kernel proper. This is why I (we?) suggested some kind of callback
> > mechanism, where the EFI stub makes a D-RTM specific call after EBS(),
> > but does regain control over the boot flow.
>
> Literally, this is not how the hardware works nor in line with the
> architecture developed by TrenchBoot. By doing the Dynamic Launch Event
> (DLE) the system, meaning the CPU, has effectively been reset. The
> CPU/Dynamic Configuration Environment (DCE) is expecting to be provided
> a kernel to measures, an entrypoint within that kernel to jump to, and
> that kernel understands the reset state of the system. As agreed, the
> callback approach is the most satisfactory approach to allow the
> efi-stub to do its private protocol. Once the flow has switched to a
> dynamic launch, the system is now starting the kernel via a dynamic
> launch sequence. The dynamic launch entrypoint, sl-stub, is and will be
> used as the entrypoint regardless of the firmware,
> UEFI/coreboot/oreboot/slimboot/etc., or CPU vendor, Intel/AMD, in use.
> Once efi-stub finishes and invokes the callback, its job is complete and
> the new dl-handler along with sl-stub from the current patch set will
> handle the system and security setup needed before entering into the
> kernel proper.
>
> To help provide clarity, consider the following flows for comparison,
>
> Normal/existing efi-stub:
>   EFI -> efi-stub -> head_64.S
>
> Proposed secure launch:
>   EFI -> efi-stub -> dl-handler -> [cpu] -> sl_stub ->head_64.S
>
> Effectively, all this is doing is after efi-stub is done, instead of
> jumping into head_64.S, it will call the dynamic launch handler to do
> the dynamic launch, let sl_stub bring the system back into an expected
> state, and then enter head_64.S just as efi-stub would have done.
>

OK, understood.

> > If we add another entry point into the kernel proper for the Secure
> > Launch Entry component to use, we are again exposing an internal ABI
> > in a way that limits our future ability to make changes to the EFI <->
> > kernel handover.
>
> I am not sure how you make that jump, but as I stated above, when
> incorporating dynamic launch it is no longer a straight pass from EFI to
> kernel proper. The sl-stub is there to provide a common entry point from
> the CPU as the result of a dynamic launch, regardless of firmware or
> vendor. Once sl-stub is done with its start-of-day setup, it will jump
> to the same location efi-stub would have jumped. This means, efi-stub
> does everything it always has and is free to change what it does. The
> only addition is that it will now enable a call-out to allow secure
> launch to do what it needs to do, and then execution returns back to
> head_64.S. To be concise, it specifically returns where efi-stub would
> have jumped to without removing or circumventing anything from the
> existing flow.
>

Excellent, that answers another question I had regarding the above.

> It should also be noted that sl-stub will not be looking to reconfigure
> the kernel. It will use the kernel as it was set up by efi-stub. The
> only job of sl-stub is to make the world right, measure what efi-stub
> provided for measurement, and then enter the kernel proper. The design
> of the SLRT structure below is specifically not to bind to anything from
> the efi-stub ABI/API. The only information needed to take the
> measurements is the location of any config items, their size, and an
> identifier for each item. The efi-stub is free to add to and/or remove
> from the list of items, along with changing where it is stored, or even
> change the format of existing items. The one recommendation, not
> requirement, is that the identifiers should not freely be changed. While
> it has no impact on sl-stub, it will likely be unpopular with anyone
> attempting to keep a manifest of valid efi-stub config items for
> attestation verification. It will result in having to maintain a
> volatile two-parameter map of (kernel version, identifier) to config
> item, at a minimum.
>

OK, noted. Can we add these recommendations to the header files
please? And let's make it a requirement - it's easier to relax it
later than the other way around.

> 
>
> >> First, a short review of what the Secure Launch Resource Table is and
> >> its purpose. The SLRT is to provide a platform and kernel agnostic
> >> configuration table of the meta-data pertinent to the Secure Launch
> >> entry points for the variety of kernels the TrenchBoot project will be
> >> seeking to support. The specification for the table is in progress, but
> >> the consensus is that it will consist of a header

Re: [PATCH v3 0/3] Cryptomount detached headers

[Glenn Washburn]

On Fri,  5 Aug 2022 11:43:51 +0200 (CEST)
brutser--- via Grub-devel  wrote:

> See below.
> 
> 
> Van: Glenn Washburn 
> Aan: brutser--- via Grub-devel 
> Onderwerp: Re: [PATCH v3 0/3] Cryptomount detached headers
> Datum: 05/08/2022 07:00:51 Europe/Paris
> Cc: brut...@perso.be;
>    dki...@net-space.pl;
>    p...@pks.im
> 
> On Thu, 4 Aug 2022 18:56:40 +0200 (CEST)
> brutser--- via Grub-devel  wrote:
> 
> > More testing done:
> > 
> > the problem most likely is triggered when encrypting partitions (see all my 
> > other comments to get a better picture).
> > 
> > I tested encrypting a complete device /dev/sdb with detached header and 
> > this worked fine, no issues, with logical volumes created.
> 
> Ok, this is progress. It looks like there may be an issue with using
> partitions. I haven't tested that.

I now see where the issue is. Because the encrypted volume is on a
partition, the reads to the disk device which contains the partition
are offset into the disk by the partition start position. This
adjustment to account for the partition is done in the grub_disk_read()
before the read hook gets called. So the read hook gets called with the
offsets already adjusted. Then the read hook will read into the header
file at the offset adjusted for the partition, but the header file is
at the beginning of the file, not at the offset of the partition.

So what needs to be done in the read hook is to, only if the disk is on
a partition, unadjust the offset so that we are reading into the header
file as if it were at the beginning of the disk. This will need to
account for the encrypted volume being nested in many partitions.

> 
> > 
> > 
> > 
> > So the problem now is consistent, coreboot, or no coreboot, when encrypting 
> > a partition, in my tests /dev/sda2, with a detached header, you cannot 
> > decrypt successfully using cryptomount.
> > 
> > 
> > 
> > cryptomount -H (hd0,msdos1)/header.bin (hd0,msdos2)
> 
> What if you do this instead:
> 
> insmod loopback
> loopback loop (hd0,msdos2)+
> cryptomount -H (hd0,msdos1)/header.bin (loop)
> 
> Does this give the same error?
> 
> No, this will work normal, ask the passphrase and decrypt the partition!
> 
> But when setting linux kernel, this error appears:
> attempt to read or write outside of disk 'crypto0'

Interesting, this might be a different bug.

> 
> > 
> > will ALWAYS result in the error: 
> > 
> > 
> > 
> > "no cryptodisk module can handle this device"
> > 
> > 
> > 
> > Strange is that it seems to be possible to ignore the partition and simply 
> > try to 'decrypt the device', which should not be possible of course, but it 
> > validates the passphrase (as seen below in comments), but
> > 
> > although it opens a crypto0 slot, this is not usable decrypted data.
> 
> This is expected behavior. As long as you enter a correct password or
> use a keyfile that is valid for one of the key slots in the detached
> header, then it doesn't matter what disk device you give to
> cryptomount, it will always be decrypted "successfully". But it will
> not be usable unless it is the right block device.
> 
> >>> Ok makes sense.
> 
> You still never answered my question about what exact QEMU commandline
> options you were using
> 
> >>> sudo qemu-system-x86_64 -serial stdio -drive 
> >>> file=~/disk2.img,format=raw,index=0,media=disk -m 3840 -boot d -M q35 
> >>> --accel kvm

Actually, for future reference, you should be able to run the grub-shell
script in the build directory like so:

cat >>EOF | ./grub-shell --disk disk2.img
insmod luks2
insmod lvm
insmod cryptodisk
cryptomount -H (hd0,msdos1)/header.bin (hd0,msdos2)
EOF

If this works, you can then run with shell tracing on to see the exact
QEMU commandline its using.

> 
> , nor if you'd tried to use the options I
> suggested for getting the serial output as text. 
> 
> >>> yes I tried, but I still had no idea how to get the output and since 
> >>> there was a little progress, I didn't want to spend too much time on it:
> qemu-system-x86_64: -fw_cfg name=etc/sercon-port,string=0: warning: 
> externally provided fw_cfg item names should be prefixed with "opt/"

This is an expected warning and can be ignored.

> but i guess it makes it much easier if I can copy the debug logs, so maybe we 
> should make this work, can you explain how i can get the logs more detailed?

Are you getting any output to the console or just that warning?

Glenn

> 
> Also, you never put in
> the debug log message for the read hook in cryptodisk like I asked.
> 
> >>> yes, I somehow did not read that part, so I will do that right now.
> 
> Glenn
> 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > Debug logs: https://imgur.com/a/onRBSMd
> > 
> > 
> > 
> > New comments below.
> > 
> > See comments below.
> > 
> > 
> > Van: Glenn Washburn 
> > Aan: brutser--- via Grub-devel 
> > Onderwerp: Re: [PATCH v3 0/3] Cryptomount detached headers
> > Datum: 03/08/2022 21:54:14 Europe/Paris
> > Cc: brut...@perso.be;
> >    dki...@net-space.pl;
> >    p...@pks.im

[PATCH] grub-shell: Use shell variable instead of autoconf

[Glenn Washburn]

By using shell variable that are set once by the expansion of an autoconf
variable, the resulting shell script is more easily moved and modified
from the build/install directory it was generated for. The resulting
script is more readable as well.

Signed-off-by: Glenn Washburn 
---
 tests/util/grub-shell.in | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/tests/util/grub-shell.in b/tests/util/grub-shell.in
index 4828afb7c..bb7b285e9 100644
--- a/tests/util/grub-shell.in
+++ b/tests/util/grub-shell.in
@@ -384,10 +384,10 @@ if test -z "$debug"; then
 fi
 
 if [ x$boot != xnet ] && [ x$boot != xemu ]; then
-pkgdatadir="@builddir@" "@builddir@/grub-mkrescue" "--output=${isofile}" 
"--override-directory=${builddir}/grub-core" \
+pkgdatadir="${builddir}" "${builddir}/grub-mkrescue" "--output=${isofile}" 
"--override-directory=${builddir}/grub-core" \
--rom-directory="${rom_directory}" \
-   --locale-directory="@srcdir@/po" \
-   --themes-directory="@srcdir@/themes" \
+   --locale-directory="${srcdir}/po" \
+   --themes-directory="${srcdir}/themes" \
$mkimage_extra_arg ${mkrescue_args} \
"/boot/grub/grub.cfg=${cfgfile}" "/boot/grub/testcase.cfg=${source}" \
${files} >/dev/null 2>&1
@@ -482,7 +482,7 @@ copy_extra_files() {
 
 if [ x$boot = xnet ]; then
 netdir=`mktemp -d "${TMPDIR:-/tmp}/tmp.XX"` || exit 1
-pkgdatadir="@builddir@" "@builddir@/grub-mknetdir" 
"--grub-mkimage=${builddir}/grub-mkimage" "--directory=${builddir}/grub-core" 
"--net-directory=$netdir" ${mkrescue_args} > /dev/null
+pkgdatadir="${builddir}" "${builddir}/grub-mknetdir" 
"--grub-mkimage=${builddir}/grub-mkimage" "--directory=${builddir}/grub-core" 
"--net-directory=$netdir" ${mkrescue_args} > /dev/null
 cp "${cfgfile}" "$netdir/boot/grub/grub.cfg"
 cp "${source}" "$netdir/boot/grub/testcase.cfg"
 [ -z "$files" ] || copy_extra_files "$netdir" $files
@@ -493,9 +493,9 @@ elif [ x$boot = xemu ]; then
 mkdir -p "$grubdir/fonts"
 mkdir -p "$grubdir/themes"
 mkdir -p "$grubdir/locale"
-test -f "@builddir@/"unicode.pf2 && cp "@builddir@/"unicode.pf2 
"$grubdir/fonts/unicode.pf2"
-cp -R "@srcdir@/themes/starfield" "$grubdir/themes/starfield"
-for file in "@srcdir@/po/"*.gmo; do
+test -f "${builddir}/"unicode.pf2 && cp "${builddir}/"unicode.pf2 
"$grubdir/fonts/unicode.pf2"
+cp -R "${srcdir}/themes/starfield" "$grubdir/themes/starfield"
+for file in "${srcdir}/po/"*.gmo; do
if [ -f "$file" ]; then
cp "$file" "$grubdir/locale/"
fi
@@ -505,7 +505,7 @@ elif [ x$boot = xemu ]; then
 [ -z "$files" ] || copy_extra_files "$rootdir" $files
 roottar="$(mktemp "${TMPDIR:-/tmp}/tmp.XX")"
 (cd "$rootdir"; tar cf "$roottar" .)
-@builddir@/grub-core/grub-emu -m "$device_map" --memdisk "$roottar" -r 
memdisk -d "/boot/grub" | tr -d "\r" | do_trim
+"${builddir}/grub-core/grub-emu" -m "$device_map" --memdisk "$roottar" -r 
memdisk -d "/boot/grub" | tr -d "\r" | do_trim
 test -n "$debug" || rm -rf "$rootdir"
 test -n "$debug" || rm -f "$roottar"
 else
-- 
2.34.1


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel