[PATCH 1/2] Add support for GPG signature enforcement on booted

[Samuel Mendoza-Jonas]

On Tue, 2016-08-02 at 22:00 -0500, Timothy Pearson wrote:
> On 08/01/2016 11:00 PM, Samuel Mendoza-Jonas wrote:
> > 
> > On Mon, 2016-08-01 at 12:09 -0500, Timothy Pearson wrote:
> > > 
> > >  kernels and related blobs
> > > 
> > > This can be used to implement a form of organization-controlled secure boot,
> > > whereby kernels may be loaded from a variety of sources but they will only
> > > boot if a valid signature file is found for each component, and only if the
> > > signature is listed in the /etc/pb-lockdown file.
> > 
> > Cool! This all looks pretty interesting - I have some general comments
> > lower down, but in particular:
> > 
> > - Some work with Secure Boot is also being done by Nayna Jain, but with
> > a different approach. I don't think either version steps on the other's
> > toes but something to look at.
> > (http://patchwork.ozlabs.org/patch/629157/ etc)
> 
> Is there a more relevant link?  The patch linked to does not seem
> related to secure boot.

Andrew posted below a link to the full series, but AFAIK that's only a
partial implementation of some remote attestation - I'll see if there is
some more useful links around.

> 
> > 
> > - It looks like you've based your changes on and older/separate version
> > of Petitboot. Current upstream (and especially for OpenPOWER) can be
> > found here: http://git.ozlabs.org/?p=petitboot, and also at
> > https://github.com/open-power/petitboot
> 
> I will update this once the general approach / implementation is
> approved.  When this patchset was created I was unaware that upstream
> was hosted outside of the Linux kernel GIT.
> 
> 
> > 
> > It's not immediately clear to me why the boot files need to be copied to
> > tmp before verifying them. Is there some quirk of gpgme that requires it?
> 
> A previous poster is correct -- this is done to prevent timing attacks
> on the signed files.
> 
> 
> > 
> > Do you mean to return here?
> 
> Definitely.  These arrors (and others) have been corrected in the V2
> patches sent just now.
> 
> > 
> > Any particular reason for this size?
> 
> No, it's mostly a first-pass balance between slow read / write and
> memory use.
> 
> > 
> > '0' or '\0'?
> 
> Corrected.
> 
> > 
> > I think there's officially enough code depending on libgpgme that you
> > could justify moving it into it's own .c file instead of interspersing
> > it within boot.c. Then instead from wrapping everything in #if defines,
> > you could decide somewhere further up whether to boot normally or with
> > signatures (eg. by stat-ing /etc/pb-lockdown). Thoughts?
> 
> I removed some of the #if guards that weren't needed in the final
> version.  Is this something you would need done before a merge?

I think so - my main concern is that if Petitboot supports both this and
some version of Nayna's work, boot.c will become very messy. Even if each
approach isolated the bulk of its logic and called out to it as needed it
would help to make it more readable / maintainable.

> 
> > 
> > I noticed in a few places increasing numbers are returned for errors, but
> > it doesn't look like any callers check for specific numbers - do these
> > have any particular meaning?
> 
> None whatsoever.  They were used during development to determine the
> exact bailout point of a failure.
> 
> 
> > 
> > Style - we order variable declarations in decreasing size for clarity, eg:
> > 
> > 	gpgme_signature_t verification_signatures;
> > 	gpgme_verify_result_t verification_result;
> > 	gpgme_engine_info_t enginfo;
> > 	etc...
> 
> Done.
> 
> > 
> > return 1; ?
> 
> Fixed.
> 
> > 
> > Use talloc_strndup() instead of strdup()
> 
> Done.
> 
> > 
> > In other spots too, but keep to an 80-character limit
> 
> Fixed.
> 
> > 
> > The problem here is that while the "Exit to shell" button is replaced,
> > users can still exit with 'x' or potentially some other key combination
> > that causes ncurses to exit.
> > Additionally it looks like the "Reboot" button just calls the exit
> > callback anyway :)
> 
> Good point.  The script we were launching petitboot from issued a hard
> machine reset if petitboot-nc exited for any reason and as a result this
> was overlooked.
> 
> Version 2 of this patch implements the following additional features:
>  * Linux reboot on UI termination
>  * Signature verification of Linux arguments
>