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

[Samuel Mendoza-Jonas]

On Thu, 2016-08-18 at 04:48 -0500, Timothy Pearson wrote:
> On 08/16/2016 10:46 PM, Samuel Mendoza-Jonas wrote:
> 
> > 
> > Since we pass boot_task here, is there any need to define the const
> > chars above? gpg_validate_boot_files can just use boot_task->local_image
> > etc, and then local_image/etc can be normal char pointers that are
> > allocated by gpg_validate_boot_files.
> 
> Fixed in patch V7.
> 
> > 
> > nitpick - indentation
> 
> Fixed in patch V7.
> 
> > 
> > boot_task should probably get freed here.
> 
> Fixed in patch V7.
> 
> > 
> > This block is the same for each image/initrd/dtb, could it be worth it
> > to add a function to gpg.c like
> > 
> > 			image_sig = gpg_set_signature(ctx, image) ?
> 
> Fixed in patch V7.
> 
> > 
> > Probably best to bail out straight away here.
> 
> Fixed in patch V7.
> 
> > 
> > talloc() this instead?
> 
> Fixed in patch V7.
> 
> > 
> > Change these last few debug errors to -1
> 
> Fixed in patch V7.
> 
> > 
> > Why not just break; here? It looks like even if there's an error we'll
> > continue to read the source file until finished.
> 
> Fixed in patch V7.
> 
> > 
> > Debug error
> 
> Not entirely sure what this means, but the retcode was change to -1 in
> patch V7.
> 
> > 
> > const char *variable
> 
> Fixed in patch V7.
> 
> > 
> > nitpick - put these before the if (signature..) check
> 
> Fixed in patch V7.
> 
> > 
> > If I'm reading this right, could you instead do
> > 	if (strncmp(auth_sig_line, verfication_signatures->fpr,
> > 		    	strlen(verification_signatures->fpr) == 0)
> > and skip the while loop?
> 
> No, this would allow essentially a substring match and could create a
> security hole.  Truncating only newline / carriage return characters
> from the end of the line does not have this side-effect.

Sure, it makes sense to err on the side of caution here. The other case
where you check for the string "ENCRYPTED" would be a case for strncmp
thought I imagine, since the length of the string is static.

> 
> > 
> > first instead to save yourself an indent of the above block?
> 
> Fixed in patch V7.
> 
> > 
> > Don't need this else since we return above
> 
> Right.  Fixed in patch V7.
> 
> > 
> > Assume I nitpick the rest of these too :)
> 
> Should all be fixed in patch V7, though I imagine some may have still
> snuck through.
> 
> > 
> > Since the else case here is do nothing, it looks neater to just do
> > 
> > 	if (!boot_task->verify_signature)
> > 		return result;
> > 
> > And save an indent on the rest of the function.
> 
> Fixed in patch V7.
> 
> > 
> > Can just do
> > 	return KEXEC_LOAD_SIG_SETUP_INVALID;
> 
> Fixed in patch V7.
> 
> > 
> > I wonder if it might not be nicer to just pass a pointer and have
> > copy_file_to_destination() talloc_strdup() the resulting filename. Then
> > we don't need to allocate 8*3K for all the names and can drop
> > MAX_FILENAME_SIZE. Thoughts?
> 
> Agreed.  Fixed in patch V7.
> 
> > 
> > If gpg_validate_boot_files() fails we'll call
> > gpg_validate_boot_files_cleanup() afterwards, so I think we're safe to
> > skip unlink()-ing local_image and local_initrd at this point.
> 
> I'm somewhat ambivalent on this, so it's been changed in patch V7 to
> behave as described.
> 
> > 
> > (especially if we change to if (!verify_signature) as above), declare
> > these variables at the start of the function.
> 
> Fixed in patch V7.
> 
> > 
> > style nitpick - use /* blah */ instead of // blah
> 
> Yeah, that snuck in accidentally.  Fixed in patch V7.
> 
> > 
> > Related to a comment below, neater if these aren't const chars and
> > the casts can be dropped.
> 
> Fixed in patch V7.
> 
> > 
> > I'm unsure about the changes in user-event. On changing the prototype,
> > would we be better off adding a new event param rather than adding
> > gen_boot_args_sigfile.
> > Since the security model as it stands appears to depend on the user not
> > accessing the shell, does it even make sense to handle signatures in
> > user events?
> 
> I did this to maintain consistency with the rest of the boot options as
> handled elsewhere in petitboot.  Even if there is no legitimate use case
> at this time, eventually a limited console might be desirable, in which
> case this becomes usable again.
> 
> As an aside, Gerrit and/or GitHub really offer a nicer mechanism for
> these sorts of complex reviews.  This is especially true when modified
> patches keep getting caught in mailing list moderation.

Changing the review process is fairly low on my TODO list, but I have
cracked and changed the list's maximum file size limit to save everyone's
sanity :)

> 
> _______________________________________________
> Petitboot mailing list
> Petitboot at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/petitboot