Message-ID: <Yf2pE4BxpaBQhaJ9@casper.infradead.org>
Date:   Fri, 4 Feb 2022 22:30:43 +0000
From:   Matthew Wilcox <willy@...radead.org>
To:     Masahiro Yamada <masahiroy@...nel.org>
Cc:     Linux Kbuild mailing list <linux-kbuild@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        efi@...ts.einval.com,
        debian-kernel <debian-kernel@...ts.debian.org>,
        linux-efi <linux-efi@...r.kernel.org>,
        Ard Biesheuvel <ardb@...nel.org>,
        David Woodhouse <dwmw2@...radead.org>,
        David Howells <dhowells@...hat.com>, keyrings@...r.kernel.org
Subject: Re: [PATCH v2] builddeb: Support signing kernels with the module
 signing key

On Wed, Jan 05, 2022 at 12:39:57AM +0900, Masahiro Yamada wrote:
> +CC the maintainers of CERTIFICATE HANDLING
> M:      David Howells <dhowells@...hat.com>
> M:      David Woodhouse <dwmw2@...radead.org>
> L:      keyrings@...r.kernel.org

Davids, can one of you respond to this?

> On Sat, Dec 18, 2021 at 12:11 PM Matthew Wilcox (Oracle)
> <willy@...radead.org> wrote:
> >
> > If the config file specifies a signing key, use it to sign
> > the kernel so that machines with SecureBoot enabled can boot.
> > See https://wiki.debian.org/SecureBoot
> >
> > Signed-off-by: Matthew Wilcox (Oracle) <willy@...radead.org>
> > ---
> > v2:
> >  - Handle private keys stored in the pem file as well as adjacent to the
> >    certificate
> >  - Handle certificate paths specified relative to both dsttree and srctree
> >    (as well as absolute)
> >  - Only try to sign the executable if EFI_STUB is enabled
> >  - Only try to execute sbsign if it's in $PATH
> >
> >  scripts/package/builddeb | 25 ++++++++++++++++++++++++-
> >  1 file changed, 24 insertions(+), 1 deletion(-)
> >
> > diff --git a/scripts/package/builddeb b/scripts/package/builddeb
> > index 91a502bb97e8..9dd92fd02b12 100755
> > --- a/scripts/package/builddeb
> > +++ b/scripts/package/builddeb
> > @@ -147,7 +147,30 @@ else
> >         cp System.map "$tmpdir/boot/System.map-$version"
> >         cp $KCONFIG_CONFIG "$tmpdir/boot/config-$version"
> >  fi
> > -cp "$($MAKE -s -f $srctree/Makefile image_name)" "$tmpdir/$installed_image_path"
> > +
> > +vmlinux=$($MAKE -s -f $srctree/Makefile image_name)
> > +key=
> > +if is_enabled CONFIG_EFI_STUB && is_enabled CONFIG_MODULE_SIG; then
> > +       cert=$(grep ^CONFIG_MODULE_SIG_KEY= include/config/auto.conf | cut -d\" -f2)
> > +       if [ ! -f $cert ]; then
> > +               cert=$srctree/$cert
> > +       fi
> > +
> > +       key=${cert%pem}priv
> > +       if [ ! -f $key ]; then
> > +               key=$cert
> > +       fi
> 
> 
> I still do not understand this part.
> 
> It is true that the Debian document you referred to creates separate files
> for the key and the certificate:
>   # openssl req -new -x509 -newkey rsa:2048 -keyout MOK.priv -outform
> DER -out MOK.der -days 36500 -subj "/CN=My Name/" -nodes
> 
> but, is such a use-case possible in Kbuild?

I don't think it matters whether *Kbuild* can generate one file or
two.  If somebody follows the *Debian* document, they will have
two files.  It would surely be desirable that if somebody has followed
the Debian instructions that we would then sign the kernel using the
keys they previously generated.

> In the old days, yes, the key and the certificate were stored in separate files.
> (the key in *.priv and the certificate in *.x509)
> 
> 
> Please read this commit:
> 
> 
> commit fb1179499134bc718dc7557c7a6a95dc72f224cb
> Author: David Woodhouse <David.Woodhouse@...el.com>
> Date:   Mon Jul 20 21:16:30 2015 +0100
> 
>     modsign: Use single PEM file for autogenerated key
> 
>     The current rule for generating signing_key.priv and signing_key.x509 is
>     a classic example of a bad rule which has a tendency to break parallel
>     make. When invoked to create *either* target, it generates the other
>     target as a side-effect that make didn't predict.
> 
>     So let's switch to using a single file signing_key.pem which contains
>     both key and certificate. That matches what we do in the case of an
>     external key specified by CONFIG_MODULE_SIG_KEY anyway, so it's also
>     slightly cleaner.
> 
>     Signed-off-by: David Woodhouse <David.Woodhouse@...el.com>
>     Signed-off-by: David Howells <dhowells@...hat.com>
> 
> 
> 
> 
> Since then, both key and certificate are stored in a single *.pem file.

I did read that commit.  I think it's a terrible idea.  If the
secret key & the certificate are stored in the same file, it's
no better than a symmetric cipher.  Not even SSH does this!

> The motivation for this change is still questionable to me;
> the commit description sounds like they merged *.priv and *.x509
> into *.pem just because they could not write a correct Makefile.
> (If requested, I can write a correct Makefile that works in parallel build)
> 
> But, anyway, as long as I read the current code, we never
> have a separate *.priv file.
> 
> 
> The help message of the config option supports my view.
> 
> 
> config MODULE_SIG_KEY
>         string "File name or PKCS#11 URI of module signing key"
>         default "certs/signing_key.pem"
>         depends on MODULE_SIG || (IMA_APPRAISE_MODSIG && MODULES)
>         help
>          Provide the file name of a private key/certificate in PEM format,
>          or a PKCS#11 URI according to RFC7512. The file should contain, or
>          the URI should identify, both the certificate and its corresponding
>                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>          private key.
>          ^^^^^^^^^^^
> 
> 
> 
> I CC'ed  David Howells, David Woodhouse, keyrings@...r.kernel.org
> in case I understood wrong.
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> > +       if ! command -v sbsign >/dev/null; then
> > +               key=
> > +       fi
> > +fi
> > +
> > +if [ -n "$key" ]; then
> > +       sbsign --key $key --cert $cert "$vmlinux" --output "$tmpdir/$installed_image_path"
> > +else
> > +       cp "$vmlinux" "$tmpdir/$installed_image_path"
> > +fi
> >
> >  if is_enabled CONFIG_OF_EARLY_FLATTREE; then
> >         # Only some architectures with OF support have this target
> > --
> > 2.33.0
> >
> 
> 
> --
> Best Regards
> Masahiro Yamada

Powered by blists - more mailing lists