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

[Timothy Pearson]

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.

> - 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 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

-- 
Timothy Pearson
Raptor Engineering
+1 (415) 727-8645 (direct line)
+1 (512) 690-0200 (switchboard)
https://www.raptorengineering.com