[PATCH] disk/loopback: Don't verify loopback images

[PATCH] disk/loopback: Don't verify loopback images

[Chris Coulson]

When a file is verified, the entire contents of the verified file are
loaded in to memory and retained until the file handle is closed. A
consequence of this is that opening a loopback image can incur a
significant memory cost.

As loopback devices are just another disk implementation, don't treat
loopback images any differently to physical disk images, and skip
verification of them. Files opened from the filesystem within a loopback
image will still be passed to verifier modules where required.

Signed-off-by: Chris Coulson <chris.coulson@canonical.com>
---
 grub-core/disk/loopback.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/grub-core/disk/loopback.c b/grub-core/disk/loopback.c
index cdf9123fa..01267e577 100644
--- a/grub-core/disk/loopback.c
+++ b/grub-core/disk/loopback.c
@@ -93,7 +93,8 @@ grub_cmd_loopback (grub_extcmd_context_t ctxt, int argc, char **args)
     return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
 
   file = grub_file_open (args[1], GRUB_FILE_TYPE_LOOPBACK
-			 | GRUB_FILE_TYPE_NO_DECOMPRESS);
+			 | GRUB_FILE_TYPE_NO_DECOMPRESS |
+			 GRUB_FILE_TYPE_SKIP_SIGNATURE);
   if (! file)
     return grub_errno;
 
-- 
2.25.1

Re: [PATCH] disk/loopback: Don't verify loopback images

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1643 bytes --]

On Mon, Jun 1, 2020, 15:21 Chris Coulson <chris.coulson@canonical.com>
wrote:

> When a file is verified, the entire contents of the verified file are
> loaded in to memory and retained until the file handle is closed. A
> consequence of this is that opening a loopback image can incur a
> significant memory cost.
>
> As loopback devices are just another disk implementation, don't treat
> loopback images any differently to physical disk images, and skip
> verification of them. Files opened from the filesystem within a loopback
> image will still be passed to verifier modules where required.
>
> Signed-off-by: Chris Coulson <chris.coulson@canonical.com>
> ---
>  grub-core/disk/loopback.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/grub-core/disk/loopback.c b/grub-core/disk/loopback.c
> index cdf9123fa..01267e577 100644
> --- a/grub-core/disk/loopback.c
> +++ b/grub-core/disk/loopback.c
> @@ -93,7 +93,8 @@ grub_cmd_loopback (grub_extcmd_context_t ctxt, int argc,
> char **args)
>      return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
>
>    file = grub_file_open (args[1], GRUB_FILE_TYPE_LOOPBACK
> -                        | GRUB_FILE_TYPE_NO_DECOMPRESS);
> +                        | GRUB_FILE_TYPE_NO_DECOMPRESS |
> +                        GRUB_FILE_TYPE_SKIP_SIGNATURE);
>
Maybe the verifier should rather decide to skip verification if fuller type
is loopback?

>    if (! file)
>      return grub_errno;
>
> --
> 2.25.1
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #2: Type: text/html, Size: 2561 bytes --]

Re: [PATCH] disk/loopback: Don't verify loopback images

[Dimitri John Ledkov]

On Tue, 2 Jun 2020 at 12:12, Vladimir 'phcoder' Serbinenko
<phcoder@gmail.com> wrote:
>
>
>
> On Mon, Jun 1, 2020, 15:21 Chris Coulson <chris.coulson@canonical.com> wrote:
>>
>> When a file is verified, the entire contents of the verified file are
>> loaded in to memory and retained until the file handle is closed. A
>> consequence of this is that opening a loopback image can incur a
>> significant memory cost.
>>
>> As loopback devices are just another disk implementation, don't treat
>> loopback images any differently to physical disk images, and skip
>> verification of them. Files opened from the filesystem within a loopback
>> image will still be passed to verifier modules where required.
>>
>> Signed-off-by: Chris Coulson <chris.coulson@canonical.com>
>> ---
>>  grub-core/disk/loopback.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/grub-core/disk/loopback.c b/grub-core/disk/loopback.c
>> index cdf9123fa..01267e577 100644
>> --- a/grub-core/disk/loopback.c
>> +++ b/grub-core/disk/loopback.c
>> @@ -93,7 +93,8 @@ grub_cmd_loopback (grub_extcmd_context_t ctxt, int argc, char **args)
>>      return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
>>
>>    file = grub_file_open (args[1], GRUB_FILE_TYPE_LOOPBACK
>> -                        | GRUB_FILE_TYPE_NO_DECOMPRESS);
>> +                        | GRUB_FILE_TYPE_NO_DECOMPRESS |
>> +                        GRUB_FILE_TYPE_SKIP_SIGNATURE);
>
> Maybe the verifier should rather decide to skip verification if fuller type is loopback?

How would it be used then? For example, I imagine one can gpg sign the
.iso or a .squashfs and then assume everything inside it is trusted
(even if things inside are not signed).
However, I don't believe any verifier works this way today, nor not
sure it is desired versus signing individual things inside the
loopback device.

At the moment, without this patch, a lot of things break. It is common
to loopback mount .iso which currently eats all the RAM and machines
crash with out of memory. (I.e. loopback mounting 2.5GB .iso).
Similarly it is common to use things like WUBI, where .raw image file
is loopback mounted from NTFS. If one is doing secureboot and tpm is
present that again results in out of memory.

Taking the measurement / checksum / verifying the loopback device is
not a problem, but keeping it all in RAM is. And imho trusting
unsigned things inside verified loopback device is dubious too.

So yeah, probably it's something that verifiers should be able to
decide upon and perform intelligently. But at the moment, the only
practical answer for all of them is to skip.

-- 
Regards,

Dimitri.

Re: [PATCH] disk/loopback: Don't verify loopback images

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 3302 bytes --]

On Tue, Jun 2, 2020, 13:21 Dimitri John Ledkov <xnox@ubuntu.com> wrote:

> On Tue, 2 Jun 2020 at 12:12, Vladimir 'phcoder' Serbinenko
> <phcoder@gmail.com> wrote:
> >
> >
> >
> > On Mon, Jun 1, 2020, 15:21 Chris Coulson <chris.coulson@canonical.com>
> wrote:
> >>
> >> When a file is verified, the entire contents of the verified file are
> >> loaded in to memory and retained until the file handle is closed. A
> >> consequence of this is that opening a loopback image can incur a
> >> significant memory cost.
> >>
> >> As loopback devices are just another disk implementation, don't treat
> >> loopback images any differently to physical disk images, and skip
> >> verification of them. Files opened from the filesystem within a loopback
> >> image will still be passed to verifier modules where required.
> >>
> >> Signed-off-by: Chris Coulson <chris.coulson@canonical.com>
> >> ---
> >>  grub-core/disk/loopback.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/grub-core/disk/loopback.c b/grub-core/disk/loopback.c
> >> index cdf9123fa..01267e577 100644
> >> --- a/grub-core/disk/loopback.c
> >> +++ b/grub-core/disk/loopback.c
> >> @@ -93,7 +93,8 @@ grub_cmd_loopback (grub_extcmd_context_t ctxt, int
> argc, char **args)
> >>      return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
> >>
> >>    file = grub_file_open (args[1], GRUB_FILE_TYPE_LOOPBACK
> >> -                        | GRUB_FILE_TYPE_NO_DECOMPRESS);
> >> +                        | GRUB_FILE_TYPE_NO_DECOMPRESS |
> >> +                        GRUB_FILE_TYPE_SKIP_SIGNATURE);
> >
> > Maybe the verifier should rather decide to skip verification if fuller
> type is loopback?
>
> How would it be used then? For example, I imagine one can gpg sign the
> .iso or a .squashfs and then assume everything inside it is trusted
> (even if things inside are not signed).
> However, I don't believe any verifier works this way today, nor not
> sure it is desired versus signing individual things inside the
> loopback device.
>
> At the moment, without this patch, a lot of things break. It is common
> to loopback mount .iso which currently eats all the RAM and machines
> crash with out of memory. (I.e. loopback mounting 2.5GB .iso).
> Similarly it is common to use things like WUBI, where .raw image file
> is loopback mounted from NTFS. If one is doing secureboot and tpm is
> present that again results in out of memory.
>
> Taking the measurement / checksum / verifying the loopback device is
> not a problem, but keeping it all in RAM is. And imho trusting
> unsigned things inside verified loopback device is dubious too.
>
> So yeah, probably it's something that verifiers should be able to
> decide upon and perform intelligently. But at the moment, the only
> practical answer for all of them is to skip.
>
GPG one can read the file in chunks and then keep in memory only hash of
every chunk to prevent TOCTOU.
But then the question is when is our makes sense versus signing individual
files. I can see uses like signing squashfs.
We probably need a "secure device" flag somewhere long term

>
> --
> Regards,
>
> Dimitri.
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #2: Type: text/html, Size: 4703 bytes --]

Re: [PATCH] disk/loopback: Don't verify loopback images

[Dimitri John Ledkov]

[-- Attachment #1: Type: text/plain, Size: 4236 bytes --]

On Tue, 2 Jun 2020, 13:53 Vladimir 'phcoder' Serbinenko, <phcoder@gmail.com>
wrote:

>
>
> On Tue, Jun 2, 2020, 13:21 Dimitri John Ledkov <xnox@ubuntu.com> wrote:
>
>> On Tue, 2 Jun 2020 at 12:12, Vladimir 'phcoder' Serbinenko
>> <phcoder@gmail.com> wrote:
>> >
>> >
>> >
>> > On Mon, Jun 1, 2020, 15:21 Chris Coulson <chris.coulson@canonical.com>
>> wrote:
>> >>
>> >> When a file is verified, the entire contents of the verified file are
>> >> loaded in to memory and retained until the file handle is closed. A
>> >> consequence of this is that opening a loopback image can incur a
>> >> significant memory cost.
>> >>
>> >> As loopback devices are just another disk implementation, don't treat
>> >> loopback images any differently to physical disk images, and skip
>> >> verification of them. Files opened from the filesystem within a
>> loopback
>> >> image will still be passed to verifier modules where required.
>> >>
>> >> Signed-off-by: Chris Coulson <chris.coulson@canonical.com>
>> >> ---
>> >>  grub-core/disk/loopback.c | 3 ++-
>> >>  1 file changed, 2 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/grub-core/disk/loopback.c b/grub-core/disk/loopback.c
>> >> index cdf9123fa..01267e577 100644
>> >> --- a/grub-core/disk/loopback.c
>> >> +++ b/grub-core/disk/loopback.c
>> >> @@ -93,7 +93,8 @@ grub_cmd_loopback (grub_extcmd_context_t ctxt, int
>> argc, char **args)
>> >>      return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename
>> expected"));
>> >>
>> >>    file = grub_file_open (args[1], GRUB_FILE_TYPE_LOOPBACK
>> >> -                        | GRUB_FILE_TYPE_NO_DECOMPRESS);
>> >> +                        | GRUB_FILE_TYPE_NO_DECOMPRESS |
>> >> +                        GRUB_FILE_TYPE_SKIP_SIGNATURE);
>> >
>> > Maybe the verifier should rather decide to skip verification if fuller
>> type is loopback?
>>
>> How would it be used then? For example, I imagine one can gpg sign the
>> .iso or a .squashfs and then assume everything inside it is trusted
>> (even if things inside are not signed).
>> However, I don't believe any verifier works this way today, nor not
>> sure it is desired versus signing individual things inside the
>> loopback device.
>>
>> At the moment, without this patch, a lot of things break. It is common
>> to loopback mount .iso which currently eats all the RAM and machines
>> crash with out of memory. (I.e. loopback mounting 2.5GB .iso).
>> Similarly it is common to use things like WUBI, where .raw image file
>> is loopback mounted from NTFS. If one is doing secureboot and tpm is
>> present that again results in out of memory.
>>
>> Taking the measurement / checksum / verifying the loopback device is
>> not a problem, but keeping it all in RAM is. And imho trusting
>> unsigned things inside verified loopback device is dubious too.
>>
>> So yeah, probably it's something that verifiers should be able to
>> decide upon and perform intelligently. But at the moment, the only
>> practical answer for all of them is to skip.
>>
> GPG one can read the file in chunks and then keep in memory only hash of
> every chunk to prevent TOCTOU.
> But then the question is when is our makes sense versus signing individual
> files. I can see uses like signing squashfs.
> We probably need a "secure device" flag somewhere long term
>

Preventing TOCTOU is nice.

And "secure device" is a nice concept.

For example a policy can consider contents of LUKS2 encrypted or dmverity
sealed /boot to be trusted. Because encryption prevents tampering with that
filesystem. Especially if it is not decrypted or mounted at runtime.

Do we need to add comments here to explain the design thinking for future
improvements? Or should we change verifiers with a link to a TODO comment
explaining what could be done to make this better?

Or should we hide this flag behind an argument
--skip-sig of the loopback command to give scripting flexibility?



>> --
>> Regards,
>>
>> Dimitri.
>>
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> https://lists.gnu.org/mailman/listinfo/grub-devel
>>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #2: Type: text/html, Size: 6609 bytes --]

Re: [PATCH] disk/loopback: Don't verify loopback images

[Julian Andres Klode]

On Mon, Jun 01, 2020 at 02:03:37PM +0100, Chris Coulson wrote:
> When a file is verified, the entire contents of the verified file are
> loaded in to memory and retained until the file handle is closed. A
> consequence of this is that opening a loopback image can incur a
> significant memory cost.
> 
> As loopback devices are just another disk implementation, don't treat
> loopback images any differently to physical disk images, and skip
> verification of them. Files opened from the filesystem within a loopback
> image will still be passed to verifier modules where required.

I looked at this patch before and while I don't have a lot of experience
with grub code, I think this is the smallest solution to our issue at
least, given that it seems unlikely anyone actually needs to verify
loopback devices.

Maybe this really needs to build two modules though, one verified-loopback
and one loopback, and then people can build monolithic binaries depending
on what behavor they need.

I think I'll go ahead merging this downstream into our Ubuntu patchset,
so we can move on while we hash out cool plans that allow people to do
both verified and unverified loopbacking?

-- 
debian developer - deb.li/jak | jak-linux.org - free software dev
ubuntu core developer                              i speak de, en

Re: [PATCH] disk/loopback: Don't verify loopback images

[Chris Coulson]

[-- Attachment #1.1.1: Type: text/plain, Size: 4244 bytes --]



On 02/06/2020 13:53, Vladimir 'phcoder' Serbinenko wrote:
>
>
> On Tue, Jun 2, 2020, 13:21 Dimitri John Ledkov <xnox@ubuntu.com
> <mailto:xnox@ubuntu.com>> wrote:
>
>     On Tue, 2 Jun 2020 at 12:12, Vladimir 'phcoder' Serbinenko
>     <phcoder@gmail.com <mailto:phcoder@gmail.com>> wrote:
>     >
>     >
>     >
>     > On Mon, Jun 1, 2020, 15:21 Chris Coulson
>     <chris.coulson@canonical.com <mailto:chris.coulson@canonical.com>>
>     wrote:
>     >>
>     >> When a file is verified, the entire contents of the verified
>     file are
>     >> loaded in to memory and retained until the file handle is closed. A
>     >> consequence of this is that opening a loopback image can incur a
>     >> significant memory cost.
>     >>
>     >> As loopback devices are just another disk implementation, don't
>     treat
>     >> loopback images any differently to physical disk images, and skip
>     >> verification of them. Files opened from the filesystem within a
>     loopback
>     >> image will still be passed to verifier modules where required.
>     >>
>     >> Signed-off-by: Chris Coulson <chris.coulson@canonical.com
>     <mailto:chris.coulson@canonical.com>>
>     >> ---
>     >>  grub-core/disk/loopback.c | 3 ++-
>     >>  1 file changed, 2 insertions(+), 1 deletion(-)
>     >>
>     >> diff --git a/grub-core/disk/loopback.c b/grub-core/disk/loopback.c
>     >> index cdf9123fa..01267e577 100644
>     >> --- a/grub-core/disk/loopback.c
>     >> +++ b/grub-core/disk/loopback.c
>     >> @@ -93,7 +93,8 @@ grub_cmd_loopback (grub_extcmd_context_t
>     ctxt, int argc, char **args)
>     >>      return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename
>     expected"));
>     >>
>     >>    file = grub_file_open (args[1], GRUB_FILE_TYPE_LOOPBACK
>     >> -                        | GRUB_FILE_TYPE_NO_DECOMPRESS);
>     >> +                        | GRUB_FILE_TYPE_NO_DECOMPRESS |
>     >> +                        GRUB_FILE_TYPE_SKIP_SIGNATURE);
>     >
>     > Maybe the verifier should rather decide to skip verification if
>     fuller type is loopback?
>
>     How would it be used then? For example, I imagine one can gpg sign the
>     .iso or a .squashfs and then assume everything inside it is trusted
>     (even if things inside are not signed).
>     However, I don't believe any verifier works this way today, nor not
>     sure it is desired versus signing individual things inside the
>     loopback device.
>
>     At the moment, without this patch, a lot of things break. It is common
>     to loopback mount .iso which currently eats all the RAM and machines
>     crash with out of memory. (I.e. loopback mounting 2.5GB .iso).
>     Similarly it is common to use things like WUBI, where .raw image file
>     is loopback mounted from NTFS. If one is doing secureboot and tpm is
>     present that again results in out of memory.
>
>     Taking the measurement / checksum / verifying the loopback device is
>     not a problem, but keeping it all in RAM is. And imho trusting
>     unsigned things inside verified loopback device is dubious too.
>
>     So yeah, probably it's something that verifiers should be able to
>     decide upon and perform intelligently. But at the moment, the only
>     practical answer for all of them is to skip.
>
> GPG one can read the file in chunks and then keep in memory only hash
> of every chunk to prevent TOCTOU.
> But then the question is when is our makes sense versus signing
> individual files. I can see uses like signing squashfs.
> We probably need a "secure device" flag somewhere long term
>
>
Hi,

That's not how the GPG verifier works today though, is it?

It's not clear from this thread whether there is agreement on what the
approach should be - I don't mind making the verifier decide to skip
verification if that is preferred instead.

Many thanks
- Chris
>
>     -- 
>     Regards,
>
>     Dimitri.
>
>     _______________________________________________
>     Grub-devel mailing list
>     Grub-devel@gnu.org <mailto:Grub-devel@gnu.org>
>     https://lists.gnu.org/mailman/listinfo/grub-devel
>


[-- Attachment #1.1.2: Type: text/html, Size: 7942 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]