Re: [PATCH v0] Additional security-relevant documentation

[Jonathan McCune]

Hi,

On Sun, Sep 29, 2013 at 2:29 AM, Andrey Borzenkov <address@hidden> wrote:

Oh, I just really meant document new option to load_env. But thank you
for the efforts!

I originally sent a version of this with my first attempt at a patch for loadenv.c.  I figured since I invested the effort to understand these mechanisms I could give back.  Plus, the places where I was wrong may be areas to improve things. :-)

Also, phcoder has since accepted a variant of this documentation patch that he edited himself to remove the errors regarding public key loading being signature checked.  However, I like some of your suggestions, so please find below a response on each of them.  I will send the corresponding (now smaller) PATCHv1 shortly.

 

В Fri, 27 Sep 2013 10:00:37 -0700
Jon McCune <address@hidden> пишет:

>  * Security::                    Authentication and authorisation
> +* Security and signatures::     Verifying digital signatures in GRUB

I think it should in subsection. Top level Security which includes
subsections Authentication and authorization (current Security) and
Signature verification. There are possibly more topics in the future
and it will unclutter top level menu.

Sounds good. Done.

 

> address@hidden
>  * biosnum::
> +* check_signatures::
>  * chosen::
>  * color_highlight::
>  * color_normal::
> @@ -2809,6 +2811,27 @@ For an alternative approach which also changes BIOS drive mappings for the
>  chain-loaded system, @pxref{drivemap}.
>
>
> address@hidden check_signatures
> address@hidden check_signatures
> +
> +This variable controls whether GRUB enforces digital signature
> +validation (@pxref{Security and signatures}) on all loaded files.  If
> address@hidden,

That looks and sounds strange (to me at least) - how would you pronounce
this sentence? "If set to @samp{enforce}" looks enough, it is in
the description of variable so context in unambiguous.

I read it as "if check_signatures equals enforce", but your suggestion conveys the same meaning and is also clear to me.  Done.

 

> address@hidden to load another file @file{foo} (e.g., a loadable
> +module, a configuration file, or a Linux kernel) implicitly invokes
> address@hidden foo foo.sig} (@pxref{verify_detached}).
> address@hidden must contain a valid digital signature over the
> +contents of @code{foo}, which can be verified with a public key
> +currently trusted by GRUB (@pxref{list_trusted}, @pxref{trust}, and
> address@hidden).  If validation fails, then file @file{foo} cannot
> +be opened.  This failure may halt or otherwise impact the boot
> +process.  An initial trusted public key can be embedded within the
> +GRUB @file{core.img} using the @code{--pubkey} option to
> address@hidden (@pxref{Invoking grub-install}).  Presently it
> +is necessary to write a custom wrapper around @command{grub-mkimage}
> +using the @code{--grub-mkimage} flag to @command{grub-install}.
> +

I have a feeling that all this does not belong here, but in Signature
verification section.

Done, with appropriate cross-references.

 

> +
> address@hidden chosen
> address@hidden chosen
>
> @@ -3458,6 +3481,7 @@ you forget a command, you can run the command @command{help}
>  * cryptomount::                 Mount a crypto device
>  * date::                        Display or set current date and time
>  * devicetree::                  Load a device tree blob
> +* distrust::                    Remove a pubkey from trusted keys

May be it's better start new section Security related commands.
Commands are already indexed, so there is no need to have one long
list. Grouping them by functions makes it easier to browse.

I did not do this yet.  I'm open to the idea, but don't currently feel comfortable enough with the disk-encryption commands to ensure I don't introduce problems.

 

>
> address@hidden distrust
> address@hidden distrust
> +
> address@hidden Command distrust pubkey_id
> +Remove public key @var{pubkey_id} from GRUB's keyring of trusted keys.

I was not able to do it. It is absolutely unclear which part of
list_trusted output should be used as input to this command. If you
know how it works, please document it with examples.

Done.  (You use the last 8 hexadecimal digits all concatenated together... it's the same as the output of gpg --fingerprint.)

 

> +These keys are used to validate signatures when
> address@hidden (@pxref{check_signatures}), and by some
> +invocations of @command{verify_detached} (@pxref{verify_detached}).
> address@hidden and signatures} for more information.
> address@hidden deffn
>

Is it necessary to repeat the same over and over again in every command
description. Reference to section where they are described seems to be
enough.

Tried to trim it down and use cross-references instead.

 

>
> address@hidden list_trusted
> address@hidden list_trusted
> +
> address@hidden Command list_trusted
> +List all public keys trusted by GRUB for validating signatures.

Please document what is actually output (fingerprint) and how it can be
used in distrust to forget the key.

Done.

 

>                                                                 These
> +public keys are used implicitly when environment variable
> address@hidden (@pxref{check_signatures}), and by some
> +invocations of @command{verify_detached}. address@hidden and
> +signatures} for more information.
> address@hidden deffn
>

repetition?

Agree. Dropped.

 

> address@hidden load_env
> address@hidden load_env
>
> address@hidden Command load_env address@hidden file]
> address@hidden Command load_env address@hidden file] address@hidden [whitelisted_variable_name] @dots{}

I think it is better to document long options. They make scripts also
more readable.

Agree. Done.

 

> address@hidden trust
> address@hidden trust
> +
> address@hidden Command trust pubkey_file
> +Read public key from @var{pubkey_file} and add it to GRUB's internal
> +list of trusted public keys.  These keys are used to validate digital
> +signatures when @code{check_signatures=enforce}.

When @var{check_signatures} is set to @samp{enforce}.

Done.  Note that I did not use @var{}.  The texinfo documentation suggests that @var is for "metasyntactic" variables.  I.e., it is appropriate for values that the user replaces, like pubkey_file, but not for explicit names, like check_signatures.  Whether @samp{} is more appropriate than @code{} I cannot say.  I did check the generated html, pdf, and info results, and these values look to be formatted correctly to me.

 

>                                                   Note that if
> address@hidden when this command is run, then
> address@hidden must itself be signed

Really? Code explicitly disables all filters (including signature
verification) and quick test allows me to trust key without having
valid trusted key. I do not know if this is intentional.

You are right.  I was mistaken about this functionality when I first wrote this documentation (at the time I did not understand the disable_...() calls).  As I was not the author, I cannot say whether this behavior is intentional or not.  If it is not intentional, then that is a bug, and should probably be resolved independently from this documentation patch.  (My personal opinion is that it is indeed a bug, and only disabling the compression filters would be more appropriate. Perhaps I will start another thread to discuss.)  Corrected the documentation to reflect the current functionality.

 

>                                           such that an already-loaded
> +public key (@pxref{list_trusted}) can validate that signature.  A
> +public key hierarchy can thus be constructed.
> address@hidden and signatures} for more information.
> address@hidden deffn
> +
>
> address@hidden unset
> address@hidden unset
> @@ -4640,6 +4736,25 @@ only on PC BIOS platforms.
> address@hidden ignore
>
>
> address@hidden verify_detached
> address@hidden verify_detached
> +
> address@hidden Command verify_detached file signature_file [pubkey_file]
> +Verifies a GPG-style detached signature, where the signed file is
> address@hidden, and the signature itself is in file @var{signature_file}.
> +Optionally, a specific public key to use can be specified using
> address@hidden  Otherwise, public keys from GRUB's trusted keys
> +(@pxref{list_trusted}, @pxref{trust}, and @pxref{distrust}) are
> +tried.  Note that, when @code{check_signatures=enforce}, an explicitly
> +identified @var{pubkey_file} must itself be signed by an
> +already-trusted key.

Same. Not tested, but code explicitly disables all filters when reading
all three files.

You are right.  Corrected.

 

> address@hidden Security and signatures
> address@hidden Security considerations when using digital signatures
> +
> +GRUB's @file{core.img} can optionally provide enforcement that all
> +files subsequently read from disk are covered by a valid digital
> +signature.  This includes GRUB configuration files, the GRUB
> +environment block, GRUB loadable modules and their dependency files,
> +and loaded operating system files such as a Linux kernel.

theme files, font files and whatever else including arguments to hash
checks. Really, "all" means "all", there is no need to explicitly list
what it comprises.

All does not mean arguments to verify_detached if a public key file is explicitly listed, or arguments to trust.  It is probably better to explicitly mention these two exceptions.  Done.

 

>                                                             This
> +document does @strong{not} cover how to ensure that your platform's
> +firmware (e.g., GNU Coreboot or UEFI Secure Boot) validates
> address@hidden
> +
> +GRUB uses GPG-style detached signatures (meaning that a file
> address@hidden will be produced when file @file{foo} is signed), and
> +currently supports the DSA signing algorithm.  Both 2048-bit and
> +3072-bit keys are supported. A signing key can be generated as
> +follows:
> +
> address@hidden
> +gpg --gen-key
> address@hidden example
> +

I think path to keyring is needed here to make it obvious you can have
more than one.

I don't want to get too tangled up in also documenting gpg.  I'm not opposed to this, but prefer to consider it out of scope for the current patch.

 

> +The corresponding public key must be embedded in @file{core.img} when
> +executing the @command{grub-mkimage} command (typically as part of
> address@hidden, @pxref{Invoking grub-install}) utility.

grub-mkimage is internal implementation detail. It should not be
mentioned here.

I tend to agree, but right now it's necessary to understand this.  When grub-install support for --pubkey matures, this can be removed.

 

>                                                                  This
> +can be done using the @code{--pubkey} option to @command{grub-mkimage}
> +and manually specifying that the modules required for signature
> +verification be embedded in @file{core.img}.  For example:
> +
> address@hidden
> +# First, wrap grub-mkimage to include your public key(s).
> +cat <<EOF > /root/grub-mkimage-pubkey.sh
> +#!/bin/sh
> +/usr/bin/grub-mkimage --pubkey=/boot/pubkey.gpg $@@
> +EOF
> +chmod +x /root/grub-mkimage-pubkey.sh
> +# Then, invoke grub-install, explicitly including the `verify'
> +# module and its dependencies (as verify cannot signature-check
> +# itself).
> +grub-install \
> +  --grub-mkimage=/root/grub-mkimage-pubkey.sh \
> +  --modules="verify gcry_rsa gcry_dsa gcry_sha256 hashsum"\
> +"gcry_sha1 mpi echo loadenv" \
> +  /dev/sda
> address@hidden example
> +

Nor should this example really be included.

Same thoughts as above.  This should get dropped as part of some future cleanup, but for the moment I think it's necessary.  It's also already committed so somewhat moot.

 

> +An individual file can be signed as follows:
> +
> address@hidden
> +gpg --detach-sign /path/to/file
> address@hidden example
> +

Should not you specify path to keyring?

I think this falls out of scope due to it not being GRUB's job to document GPG.

 

> +For successful validation of all of GRUB's subcomponents and the
> +loaded OS kernel, they must all be signed.  One way to accomplish this
> +is the following (after having already produced the desired
> address@hidden file, e.g., by running @command{grub-mkconfig}
> +(@pxref{Invoking grub-mkconfig}):
> +
> address@hidden
> address@hidden
> +# Edit /dev/shm/passphrase.txt to contain your signing key's passphrase
> +for i in `find /boot -name "*.cfg" -or -name "*.lst" -or \
> +  -name "*.mod" -or -name "vmlinuz*" -or -name "initrd*" -or \
> +  -name "grubenv"`;
> +do
> +  gpg --batch --detach-sign --passphrase-fd 0 $i < \
> +    /dev/shm/passphrase.txt
> +done
> +shred /dev/shm/passphrase.txt
> address@hidden group
> address@hidden example
> +

Not sure if this belongs here. Either it has to be integrated in
grub-install and documented there or anyone with minimal shell
programming skills can do it. Nor is this example correct, as it does
not mention theme files, font files, locale files and others.

It's already in as-is.  Cleanup would be great but let that be another patch.

 

> +
> address@hidden address@hidden
> +Use @var{program} as @command{grub-mkimage}.  This is primarily useful
> +for advanced users who wish to provide custom arguments to
> address@hidden

For me the main usage is to be able to run grub-install from build
directory without installing it. I do not know if this omitted
intentionally, but this is advanced option not required for normal
usage.

That sounds like developer documentation, but I think we both agree that this is a minor point.  Leaving as-is for now.

Unless there is a strong reason to keep this thread alive, let's move discussing to the PATCHv1 thread that I will start shortly.

Thanks!

-Jon