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

[Timothy Pearson]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

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.

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

- -- 
Timothy Pearson
Raptor Engineering
+1 (415) 727-8645 (direct line)
+1 (512) 690-0200 (switchboard)
https://www.raptorengineering.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEbBAEBAgAGBQJXtYRlAAoJEK+E3vEXDOFbuvQH+Kj6D2zqWwhqEOVdIQM6B0JY
aENPYGKrg+9fud4WGaf6kQg3UfKRYPefcVOVVMRUsRyzgXZDN+hI15hUDBBPR/aO
GgD3FNPjtGM8X/Kkar+wa27+/MTtVtz1oPlsHlDDbEnWOTET1Y/wUoMgCPTT0w5J
sm6LRv7J21kqBf1WieX65T8zFAG4/BCxtz2i+b1WgYhf7JFT/HgmbPXWWSLYl27B
yviCuM+XAKm8g9+CfCl/diBOodo0N9j54IzBRasGPJW0xCEBoKf3ZktNd92109aM
hnCVLGfcDRRhSxJ+48tcLtq8d3jT4b9MiJC3lTmoezGN5lsl/MmoqkkBjQL66g==
=B8V9
-----END PGP SIGNATURE-----