<div dir="ltr">Hi!<br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Aug 1, 2016 at 9:00 PM, Samuel Mendoza-Jonas <span dir="ltr"><<a href="mailto:sam@ozlabs.au.ibm.com" target="_blank">sam@ozlabs.au.ibm.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><span class="gmail-">On Mon, 2016-08-01 at 12:09 -0500, Timothy Pearson wrote:<br>
> kernels and related blobs<br>
><br>
> This can be used to implement a form of organization-controlled secure boot,<br>
> whereby kernels may be loaded from a variety of sources but they will only<br>
> boot if a valid signature file is found for each component, and only if the<br>
> signature is listed in the /etc/pb-lockdown file.<br>
<br>
</span>Cool! This all looks pretty interesting - I have some general comments<br>
lower down, but in particular:<br>
<br>
- Some work with Secure Boot is also being done by Nayna Jain, but with<br>
a different approach. I don't think either version steps on the other's<br>
toes but something to look at.<br>
(<a href="http://patchwork.ozlabs.org/patch/629157/" rel="noreferrer" target="_blank">http://patchwork.ozlabs.org/patch/629157/</a> etc)<br></blockquote><div><br></div><div>I think it's also worth noting that GRUB2 has some OpenPGP signature verification support:</div><div><a href="https://github.com/jonmccune/grub2/blob/master/grub-core/commands/verify.c">https://github.com/jonmccune/grub2/blob/master/grub-core/commands/verify.c</a><br></div><div><a href="https://github.com/jonmccune/grub2/blob/master/grub-core/commands/loadenv.c">https://github.com/jonmccune/grub2/blob/master/grub-core/commands/loadenv.c</a><br></div><div><a href="https://github.com/jonmccune/grub2/blob/master/docs/grub.texi">https://github.com/jonmccune/grub2/blob/master/docs/grub.texi</a> (search for 'detached')<br></div><div>(I used a <a href="http://github.com">github.com</a> link b/c the <a href="http://git.savannah.gnu.org/">http://git.savannah.gnu.org/</a> was reporting errors.)</div><div><br></div><div>The implementation in these patches doesn't seem to consider the mechanism by which Petitboot discovers its boot options (which is actually pretty cute), but that seems to restrict use to kernels with their entire kernel command-line compiled in (or allowing perhaps unexpected things like init=/bin/bash). Have you considered integrating with any bootloader configuration file format?</div><div><br></div><div>I wasn't aware of Nayna Jain's efforts, and haven't looked at them yet, but I might be interested in following up on work like the present patch series by updating the Petitboot parser for GRUB-style configuration to support some of the related commands that GRUB2 offers.</div><div><br></div><div>Thanks!</div><div>-Jon</div><div><br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<br>
- It looks like you've based your changes on and older/separate version<br>
of Petitboot. Current upstream (and especially for OpenPOWER) can be<br>
found here: <a href="http://git.ozlabs.org/?p=petitboot" rel="noreferrer" target="_blank">http://git.ozlabs.org/?p=petitboot</a>, and also at<br>
<a href="https://github.com/open-power/petitboot" rel="noreferrer" target="_blank">https://github.com/open-power/petitboot</a><br>
<div><div class="gmail-h5"><br>
><br>
> This patch also disables direct command line access when the /etc/pb-lockdown<br>
> file is present.<br>
><br>
> Signed-off-by: Timothy Pearson <<a href="mailto:tpearson@raptorengineering.com">tpearson@raptorengineering.com</a>><br>
> ---<br>
> <a href="http://configure.ac" rel="noreferrer" target="_blank">configure.ac</a> | 62 ++++++++<br>
> discover/Makefile.am | 3 +-<br>
> discover/boot.c | 381 +++++++++++++++++++++++++++++++++++++++++++++++++-<br>
> lib/url/url.c | 2 +-<br>
> lib/url/url.h | 1 +<br>
> ui/ncurses/nc-cui.c | 16 ++-<br>
> 6 files changed, 454 insertions(+), 11 deletions(-)<br>
><br>
> diff --git a/<a href="http://configure.ac" rel="noreferrer" target="_blank">configure.ac</a> b/<a href="http://configure.ac" rel="noreferrer" target="_blank">configure.ac</a><br>
> index b540819..f6d23c2 100644<br>
> --- a/<a href="http://configure.ac" rel="noreferrer" target="_blank">configure.ac</a><br>
> +++ b/<a href="http://configure.ac" rel="noreferrer" target="_blank">configure.ac</a><br>
> @@ -150,6 +150,68 @@ AS_IF(<br>
> ]<br>
> )<br>
><br>
> +AC_ARG_WITH(<br>
> + [signed-boot],<br>
> + [AS_HELP_STRING([--with-signed-boot],<br>
> + [build kernel signature checking support [default=yes]]<br>
> + )],<br>
> + [],<br>
> + [with_signed_boot=yes]<br>
> +)<br>
> +AM_CONDITIONAL([WITH_SIGNED_BOOT], [test "x$with_signed_boot" = "xyes"])<br>
> +<br>
> +AM_CONDITIONAL(<br>
> + [WITH_SIGNED_BOOT],<br>
> + [test "x$with_signed_boot" = "xyes"])<br>
> +<br>
> +AS_IF(<br>
> + [test "x$with_signed_boot" = "xyes"],<br>
> + [PKG_CHECK_MODULES(<br>
> + [GPGME],<br>
> + [gpgme >= 1.0.0],<br>
> + [SAVE_LIBS="$LIBS" LIBS="$LIBS $gpgme_LIBS"<br>
> + AC_CHECK_LIB(<br>
> + [gpgme],<br>
> + [gpgme_op_verify],<br>
> + [],<br>
> + [AC_MSG_FAILURE([--with-signed-boot was given but the test for gpgme failed.])]<br>
> + )<br>
> + LIBS="$SAVE_LIBS"<br>
> + ],<br>
> + [AM_PATH_GPGME([1.0.0], [SAVE_LIBS="$LIBS" LIBS="$LIBS $gpgme_LIBS"<br>
> + AC_CHECK_LIB(<br>
> + [gpgme],<br>
> + [gpgme_op_verify],<br>
> + [],<br>
> + [AC_MSG_FAILURE([--with-signed-boot was given but the test for gpgme failed.])]<br>
> + )<br>
> + LIBS="$SAVE_LIBS"],<br>
> + [AC_MSG_RESULT([$gpgme_PKG_ERRORS])<br>
> + AC_MSG_FAILURE([ Consider adjusting PKG_CONFIG_PATH environment variable])<br>
> + ])<br>
> + ]<br>
> + )]<br>
> +)<br>
> +<br>
> +AS_IF(<br>
> + [test "x$with_signed_boot" = "xyes"],<br>
> + [SAVE_CPPFLAGS="$CPPFLAGS" CPPFLAGS="$CPPFLAGS $gpgme_CFLAGS"<br>
> + AC_CHECK_HEADERS(<br>
> + [gpgme.h],<br>
> + [],<br>
> + [AC_MSG_FAILURE([ --with-signed-boot given but gpgme.h not found])]<br>
> + )<br>
> + CPPFLAGS="$SAVE_CPPFLAGS"<br>
> + ]<br>
> +)<br>
> +<br>
> +AC_ARG_VAR(<br>
> + [lockdown_file],<br>
> + [Location of authorized signature file [default = "/etc/pb-lockdown"]]<br>
> +)<br>
> +AS_IF([test "x$lockdown_file" = x], [lockdown_file="/etc/pb-lockdown"])<br>
> +AC_DEFINE_UNQUOTED(LOCKDOWN_FILE, "$lockdown_file", [Lockdown file location])<br>
> +<br>
> AC_ARG_ENABLE(<br>
> [busybox],<br>
> [AS_HELP_STRING(<br>
> diff --git a/discover/Makefile.am b/discover/Makefile.am<br>
> index 5d0f6e2..44ef5e5 100644<br>
> --- a/discover/Makefile.am<br>
> +++ b/discover/Makefile.am<br>
> @@ -56,7 +56,8 @@ discover_pb_discover_LDADD = \<br>
> discover/grub2/<a href="http://grub2-parser.ro" rel="noreferrer" target="_blank">grub2-parser.ro</a> \<br>
> discover/<a href="http://platform.ro" rel="noreferrer" target="_blank">platform.ro</a> \<br>
> $(core_lib) \<br>
> - $(UDEV_LIBS)<br>
> + $(UDEV_LIBS) \<br>
> + $(GPGME_LIBS)<br>
><br>
> discover_pb_discover_CPPFLAGS = \<br>
> $(AM_CPPFLAGS) \<br>
> diff --git a/discover/boot.c b/discover/boot.c<br>
> index 6e7fda6..eb65f31 100644<br>
> --- a/discover/boot.c<br>
> +++ b/discover/boot.c<br>
> @@ -20,6 +20,10 @@<br>
> #include <util/util.h><br>
> #include <i18n/i18n.h><br>
><br>
> +#if defined(HAVE_LIBGPGME)<br>
> +#include <gpgme.h><br>
> +#endif<br>
> +<br>
> #include "device-handler.h"<br>
> #include "boot.h"<br>
> #include "paths.h"<br>
> @@ -31,6 +35,11 @@ enum {<br>
> BOOT_HOOK_EXIT_UPDATE = 2,<br>
> };<br>
><br>
> +enum {<br>
> + KEXEC_LOAD_SIG_SETUP_INVALID = 253,<br>
> + KEXEC_LOAD_SIGNATURE_FAILURE = 254,<br>
> +};<br>
> +<br>
> struct boot_task {<br>
> struct load_url_result *image;<br>
> struct load_url_result *initrd;<br>
> @@ -43,8 +52,189 @@ struct boot_task {<br>
> void *status_arg;<br>
> bool dry_run;<br>
> bool cancelled;<br>
> +#if defined(HAVE_LIBGPGME)<br>
> + bool verify_signature;<br>
> + struct load_url_result *image_signature;<br>
> + struct load_url_result *initrd_signature;<br>
> + struct load_url_result *dtb_signature;<br>
> + const char *local_image_signature;<br>
> + const char *local_initrd_signature;<br>
> + const char *local_dtb_signature;<br>
> +#endif<br>
> };<br>
><br>
> +static int copy_file_to_destination(const char * source_file, char * destination_file, int max_dest_filename_size) {<br>
<br>
</div></div>It's not immediately clear to me why the boot files need to be copied to<br>
tmp before verifying them. Is there some quirk of gpgme that requires it?<br></blockquote><div><br></div><div>I don't know the author's intention but copying to a ramdisk helps to address a time-of-check, time-of-use attack if the disk firmware (or network mount...) is malicious, and feeds one set of files for signature verification and a distinct set for the actual kexec.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<span class="gmail-"><br>
> + int result = 0;<br>
> + char template[21] = "/tmp/petitbootXXXXXX";<br>
> + FILE *source_handle = fopen(source_file, "r");<br>
> + int destination_fd = mkstemp(template);<br>
> + FILE *destination_handle = fdopen(destination_fd, "w");<br>
> + if(!source_handle || !(destination_handle)) {<br>
> + // handle open error<br>
> + result = 1;<br>
<br>
</span>Do you mean to return here?<br>
<span class="gmail-"><br>
> + pb_log("%s: failed: unable to open source file '%s'\n", __func__, source_file);<br>
> + }<br>
> +<br>
> + size_t l1;<br>
> + unsigned char buffer[8192];<br>
<br>
</span>Any particular reason for this size?<br>
<span class="gmail-"><br>
> +<br>
> + /* Copy data */<br>
> + while ((l1 = fread(buffer, 1, sizeof buffer, source_handle)) > 0) {<br>
> + size_t l2 = fwrite(buffer, 1, l1, destination_handle);<br>
> + if (l2 < l1) {<br>
> + if (ferror(destination_handle)) {<br>
> + /* General error */<br>
> + result = 1;<br>
> + pb_log("%s: failed: unknown fault\n", __func__);<br>
> + }<br>
> + else {<br>
> + /* No space on destination device */<br>
> + result = 2;<br>
> + pb_log("%s: failed: temporary storage full\n", __func__);<br>
> + }<br>
> + }<br>
> + }<br>
> +<br>
> + if (result) {<br>
> + destination_file[0] = 0;<br>
<br>
</span>'0' or '\0'?<br>
<span class="gmail-"><br>
> + }<br>
> + else {<br>
> + ssize_t r;<br>
> + char readlink_buffer[8192];<br>
> + snprintf(readlink_buffer, 8192, "/proc/self/fd/%d", destination_fd);<br>
<br>
</span>Style stuff - unless there's a particular reason for it we generally<br>
prefer that all variables are defined at the start of a function.<br>
Additionally you could probably flip this around and have<br>
if (result) {<br>
...<br>
return result;<br>
}<br>
to lose the indent for this block.<br>
<span class="gmail-"><br>
> + r = readlink(readlink_buffer, destination_file, max_dest_filename_size);<br>
> + if (r < 0) {<br>
> + /* readlink failed */<br>
> + result = 3;<br>
> + pb_log("%s: failed: unable to obtain temporary filename\n", __func__);<br>
> + }<br>
> + destination_file[r] = 0;<br>
> + }<br>
> +<br>
> + fclose(source_handle);<br>
> + fclose(destination_handle);<br>
> +<br>
> + return result;<br>
> +}<br>
> +<br>
> +#if defined(HAVE_LIBGPGME)<br>
<br>
</span>I think there's officially enough code depending on libgpgme that you<br>
could justify moving it into it's own .c file instead of interspersing<br>
it within boot.c. Then instead from wrapping everything in #if defines,<br>
you could decide somewhere further up whether to boot normally or with<br>
signatures (eg. by stat-ing /etc/pb-lockdown). Thoughts?<br>
<span class="gmail-"><br>
> +static int verify_file_signature(const char * plaintext_filename, const char * signature_filename, FILE * authorized_signatures_handle, const char * keyring_path)<br>
> +{<br>
> + int result = 0;<br>
> + int valid = 0;<br>
> +<br>
> + if (signature_filename == NULL)<br>
> + return 10;<br>
<br>
</span>I noticed in a few places increasing numbers are returned for errors, but<br>
it doesn't look like any callers check for specific numbers - do these<br>
have any particular meaning?<br>
<span class="gmail-"><br>
> +<br>
> + gpgme_ctx_t gpg_context;<br>
> + gpgme_error_t err;<br>
> + gpgme_data_t plaintext_data;<br>
> + gpgme_data_t signature_data;<br>
> + gpgme_engine_info_t enginfo;<br>
> + gpgme_verify_result_t verification_result;<br>
> + gpgme_signature_t verification_signatures;<br>
<br>
</span>Style - we order variable declarations in decreasing size for clarity, eg:<br>
<br>
gpgme_signature_t verification_signatures;<br>
gpgme_verify_result_t verification_result;<br>
gpgme_engine_info_t enginfo;<br>
etc...<br>
<span class="gmail-"><br>
> +<br>
> + /* Initialize gpgme */<br>
> + setlocale (LC_ALL, "");<br>
> + gpgme_check_version(NULL);<br>
> + gpgme_set_locale(NULL, LC_CTYPE, setlocale (LC_CTYPE, NULL));<br>
> + err = gpgme_engine_check_version(GPGME_PROTOCOL_OpenPGP);<br>
> + if (err != GPG_ERR_NO_ERROR) {<br>
> + pb_log("%s: OpenPGP support not available\n", __func__);<br>
> + result = 1;<br>
> + return result;<br>
<br>
</span>return 1; ?<br>
<div><div class="gmail-h5"><br>
> + }<br>
> + err = gpgme_get_engine_info(&enginfo);<br>
> + if (err != GPG_ERR_NO_ERROR) {<br>
> + pb_log("%s: GPG engine failed to initialize\n", __func__);<br>
> + result = 2;<br>
> + return result;<br>
> + }<br>
> + err = gpgme_new(&gpg_context);<br>
> + if (err != GPG_ERR_NO_ERROR) {<br>
> + pb_log("%s: GPG context could not be created\n", __func__);<br>
> + result = 3;<br>
> + return result;<br>
> + }<br>
> + err = gpgme_set_protocol(gpg_context, GPGME_PROTOCOL_OpenPGP);<br>
> + if (err != GPG_ERR_NO_ERROR) {<br>
> + pb_log("%s: GPG protocol could not be set\n", __func__);<br>
> + result = 4;<br>
> + return result;<br>
> + }<br>
> + if (keyring_path)<br>
> + err = gpgme_ctx_set_engine_info (gpg_context, GPGME_PROTOCOL_OpenPGP, enginfo->file_name, keyring_path);<br>
> + else<br>
> + err = gpgme_ctx_set_engine_info (gpg_context, GPGME_PROTOCOL_OpenPGP, enginfo->file_name, enginfo->home_dir);<br>
> + if (err != GPG_ERR_NO_ERROR) {<br>
> + pb_log("%s: Could not set GPG engine information\n", __func__);<br>
> + result = 5;<br>
> + return result;<br>
> + }<br>
> + err = gpgme_data_new_from_file(&plaintext_data, plaintext_filename, 1);<br>
> + if (err != GPG_ERR_NO_ERROR) {<br>
> + pb_log("%s: Could not create GPG plaintext data buffer from file '%s'\n", __func__, plaintext_filename);<br>
> + result = 6;<br>
> + return result;<br>
> + }<br>
> + err = gpgme_data_new_from_file(&signature_data, signature_filename, 1);<br>
> + if (err != GPG_ERR_NO_ERROR) {<br>
> + pb_log("%s: Could not create GPG signature data buffer from file '%s'\n", __func__, signature_filename);<br>
> + result = 7;<br>
> + return result;<br>
> + }<br>
> +<br>
> + /* Check signature */<br>
> + err = gpgme_op_verify(gpg_context, signature_data, plaintext_data, NULL);<br>
> + if (err != GPG_ERR_NO_ERROR) {<br>
> + pb_log("%s: Could not verify GPG signature\n", __func__);<br>
> + result = 8;<br>
> + return result;<br>
> + }<br>
> + verification_result = gpgme_op_verify_result(gpg_context);<br>
> + verification_signatures = verification_result->signatures;<br>
> + while (verification_signatures) {<br>
> + if (verification_signatures->status == GPG_ERR_NO_ERROR) {<br>
> + pb_log("%s: Good signature for key ID '%s' ('%s')\n", __func__, verification_signatures->fpr, signature_filename);<br>
> + /* Verify fingerprint is present in authorized signatures file */<br>
> + char *auth_sig_line = NULL;<br>
> + size_t auth_sig_len = 0;<br>
> + ssize_t auth_sig_read;<br>
> + rewind(authorized_signatures_handle);<br>
> + while ((auth_sig_read = getline(&auth_sig_line, &auth_sig_len, authorized_signatures_handle)) != -1) {<br>
> + auth_sig_len = strlen(auth_sig_line);<br>
> + while ((auth_sig_line[auth_sig_len-1] == '\n') || (auth_sig_line[auth_sig_len-1] == '\r'))<br>
> + auth_sig_len--;<br>
> + auth_sig_line[auth_sig_len] = 0;<br>
> + if (strcmp(auth_sig_line, verification_signatures->fpr) == 0)<br>
> + valid = 1;<br>
> + }<br>
> + free(auth_sig_line);<br>
> + }<br>
> + else {<br>
> + pb_log("%s: Signature for key ID '%s' ('%s') invalid. Status: %08x\n", __func__, verification_signatures->fpr, signature_filename, verification_signatures->status);<br>
> + }<br>
> + verification_signatures = verification_signatures->next;<br>
> + }<br>
> +<br>
> + /* Clean up */<br>
> + gpgme_data_release(plaintext_data);<br>
> + gpgme_data_release(signature_data);<br>
> + gpgme_release(gpg_context);<br>
> +<br>
> + if (!valid) {<br>
> + pb_log("%s: Incorrect GPG signature\n", __func__);<br>
> + result = 9;<br>
> + return result;<br>
> + }<br>
> + else {<br>
> + pb_log("%s: GPG signature '%s' for file '%s' verified\n", __func__, signature_filename, plaintext_filename);<br>
> + }<br>
> +<br>
> + return result;<br>
> +}<br>
> +#endif<br>
> +<br>
> /**<br>
> * kexec_load - kexec load helper.<br>
> */<br>
> @@ -57,20 +247,98 @@ static int kexec_load(struct boot_task *boot_task)<br>
> char *s_dtb = NULL;<br>
> char *s_args = NULL;<br>
><br>
> + const char* local_initrd = boot_task->local_initrd;<br>
> + const char* local_dtb = boot_task->local_dtb;<br>
> + const char* local_image = boot_task->local_image;<br>
> +<br>
> +#if defined(HAVE_LIBGPGME)<br>
> + const char* local_initrd_signature = (boot_task->verify_signature) ? boot_task->local_initrd_signature : NULL;<br>
> + const char* local_dtb_signature = (boot_task->verify_signature) ? boot_task->local_dtb_signature : NULL;<br>
> + const char* local_image_signature = (boot_task->verify_signature) ? boot_task->local_image_signature : NULL;<br>
> +<br>
> + if (boot_task->verify_signature) {<br>
> + int max_filename_size = 8192;<br>
> + char kernel_filename[max_filename_size];<br>
> + char initrd_filename[max_filename_size];<br>
> + char dtb_filename[max_filename_size];<br>
> +<br>
> + kernel_filename[0] = 0;<br>
> + initrd_filename[0] = 0;<br>
> + dtb_filename[0] = 0;<br>
> +<br>
> + FILE *authorized_signatures_handle = NULL;<br>
> +<br>
> + /* Load authorized signatures file */<br>
> + authorized_signatures_handle = fopen(LOCKDOWN_FILE, "r");<br>
> + if (!authorized_signatures_handle) {<br>
> + pb_log("%s: unable to read lockdown file\n", __func__);<br>
> + result = KEXEC_LOAD_SIG_SETUP_INVALID;<br>
> + return result;<br>
> + }<br>
> +<br>
> + /* Copy files to temporary directory for verification / boot */<br>
> + result = copy_file_to_destination(boot_task->local_image, kernel_filename, max_filename_size);<br>
> + if (result) {<br>
> + pb_log("%s: image copy failed: (%d)\n", __func__, result);<br>
> + return result;<br>
> + }<br>
> + if (boot_task->local_initrd) {<br>
> + result = copy_file_to_destination(boot_task->local_initrd, initrd_filename, max_filename_size);<br>
> + if (result) {<br>
> + pb_log("%s: initrd copy failed: (%d)\n", __func__, result);<br>
> + unlink(local_image);<br>
> + return result;<br>
> + }<br>
> + }<br>
> + if (boot_task->local_dtb) {<br>
> + result = copy_file_to_destination(boot_task->local_dtb, dtb_filename, max_filename_size);<br>
> + if (result) {<br>
> + pb_log("%s: dtb copy failed: (%d)\n", __func__, result);<br>
> + unlink(local_image);<br>
> + if (local_initrd)<br>
> + unlink(local_initrd);<br>
> + return result;<br>
> + }<br>
> + }<br>
> + local_image = strdup(kernel_filename);<br>
<br>
</div></div>Use talloc_strndup() instead of strdup()<br>
<div><div class="gmail-h5"><br>
> + if (boot_task->local_initrd)<br>
> + local_initrd = strdup(initrd_filename);<br>
> + if (boot_task->local_dtb)<br>
> + local_dtb = strdup(dtb_filename);<br>
> +<br>
> + /* Check signatures */<br>
> + if (verify_file_signature(kernel_filename, local_image_signature, authorized_signatures_handle, "/etc/gpg"))<br>
> + result = KEXEC_LOAD_SIGNATURE_FAILURE;<br>
> + if (boot_task->local_initrd_signature)<br>
> + if (verify_file_signature(initrd_filename, local_initrd_signature, authorized_signatures_handle, "/etc/gpg"))<br>
> + result = KEXEC_LOAD_SIGNATURE_FAILURE;<br>
> + if (boot_task->local_dtb_signature)<br>
> + if (verify_file_signature(dtb_filename, local_dtb_signature, authorized_signatures_handle, "/etc/gpg"))<br>
> + result = KEXEC_LOAD_SIGNATURE_FAILURE;<br>
> +<br>
> + fclose(authorized_signatures_handle);<br>
> +<br>
> + if (result == KEXEC_LOAD_SIGNATURE_FAILURE) {<br>
> + pb_log("%s: Aborting kexec due to signature verification failure\n", __func__);<br>
> + goto abort_kexec;<br>
> + }<br>
> + }<br>
> +#endif<br>
> +<br>
> p = argv;<br>
> *p++ = pb_system_apps.kexec; /* 1 */<br>
> *p++ = "-l"; /* 2 */<br>
><br>
> - if (boot_task->local_initrd) {<br>
> + if (local_initrd) {<br>
> s_initrd = talloc_asprintf(boot_task, "--initrd=%s",<br>
> - boot_task->local_initrd);<br>
> + local_initrd);<br>
> assert(s_initrd);<br>
> *p++ = s_initrd; /* 3 */<br>
> }<br>
><br>
> - if (boot_task->local_dtb) {<br>
> + if (local_dtb) {<br>
> s_dtb = talloc_asprintf(boot_task, "--dtb=%s",<br>
> - boot_task->local_dtb);<br>
> + local_dtb);<br>
> assert(s_dtb);<br>
> *p++ = s_dtb; /* 4 */<br>
> }<br>
> @@ -82,7 +350,7 @@ static int kexec_load(struct boot_task *boot_task)<br>
> *p++ = s_args; /* 5 */<br>
> }<br>
><br>
> - *p++ = boot_task->local_image; /* 6 */<br>
> + *p++ = local_image; /* 6 */<br>
> *p++ = NULL; /* 7 */<br>
><br>
> result = process_run_simple_argv(boot_task, argv);<br>
> @@ -90,6 +358,23 @@ static int kexec_load(struct boot_task *boot_task)<br>
> if (result)<br>
> pb_log("%s: failed: (%d)\n", __func__, result);<br>
><br>
> +#if defined(HAVE_LIBGPGME)<br>
> +abort_kexec:<br>
> + if (boot_task->verify_signature) {<br>
> + unlink(local_image);<br>
> + if (local_initrd)<br>
> + unlink(local_initrd);<br>
> + if (local_dtb)<br>
> + unlink(local_dtb);<br>
> +<br>
> + free((char*)local_image);<br>
> + if (local_initrd)<br>
> + free((char*)local_initrd);<br>
> + if (local_dtb)<br>
> + free((char*)local_dtb);<br>
> + }<br>
> +#endif<br>
> +<br>
> return result;<br>
> }<br>
><br>
> @@ -373,23 +658,53 @@ static void boot_process(struct load_url_result *result, void *data)<br>
> check_load(task, "dtb", task->dtb))<br>
> goto no_load;<br>
><br>
> +#if defined(HAVE_LIBGPGME)<br>
> + if (task->verify_signature) {<br>
> + if (check_load(task, "kernel image signature", task->image_signature) ||<br>
> + check_load(task, "initrd signature", task->initrd_signature) ||<br>
> + check_load(task, "dtb signature", task->dtb_signature))<br>
> + goto no_sig_load;<br>
> + }<br>
> +#endif<br>
> +<br>
> /* we make a copy of the local paths, as the boot hooks might update<br>
> * and/or create these */<br>
> task->local_image = task->image ? task->image->local : NULL;<br>
> task->local_initrd = task->initrd ? task->initrd->local : NULL;<br>
> task->local_dtb = task->dtb ? task->dtb->local : NULL;<br>
><br>
> +#if defined(HAVE_LIBGPGME)<br>
> + if (task->verify_signature) {<br>
> + task->local_image_signature = task->image_signature ? task->image_signature->local : NULL;<br>
> + task->local_initrd_signature = task->initrd_signature ? task->initrd_signature->local : NULL;<br>
> + task->local_dtb_signature = task->dtb_signature ? task->dtb_signature->local : NULL;<br>
> + }<br>
> +#endif<br>
> +<br>
> run_boot_hooks(task);<br>
><br>
> update_status(task->status_fn, task->status_arg, BOOT_STATUS_INFO,<br>
> _("performing kexec_load"));<br>
><br>
> rc = kexec_load(task);<br>
> - if (rc) {<br>
> + if (rc == KEXEC_LOAD_SIGNATURE_FAILURE) {<br>
> + update_status(task->status_fn, task->status_arg,<br>
> + BOOT_STATUS_ERROR, _("signature verification failed"));<br>
> + }<br>
> + else if (rc == KEXEC_LOAD_SIG_SETUP_INVALID) {<br>
> + update_status(task->status_fn, task->status_arg,<br>
> + BOOT_STATUS_ERROR, _("invalid signature configuration"));<br>
> + }<br>
> + else if (rc) {<br>
> update_status(task->status_fn, task->status_arg,<br>
> BOOT_STATUS_ERROR, _("kexec load failed"));<br>
> }<br>
><br>
> +no_sig_load:<br>
> + cleanup_load(task->image_signature);<br>
> + cleanup_load(task->initrd_signature);<br>
> + cleanup_load(task->dtb_signature);<br>
> +<br>
> no_load:<br>
> cleanup_load(task->image);<br>
> cleanup_load(task->initrd);<br>
> @@ -434,6 +749,19 @@ struct boot_task *boot(void *ctx, struct discover_boot_option *opt,<br>
> const char *boot_desc;<br>
> int rc;<br>
><br>
> +#if defined(HAVE_LIBGPGME)<br>
> + struct pb_url *image_sig = NULL, *initrd_sig = NULL, *dtb_sig = NULL;<br>
> +<br>
> + int max_filename_size = 8192;<br>
> + char kernel_signature_filename[max_filename_size];<br>
> + char initrd_signature_filename[max_filename_size];<br>
> + char dtb_signature_filename[max_filename_size];<br>
> +<br>
> + kernel_signature_filename[0] = 0;<br>
> + initrd_signature_filename[0] = 0;<br>
> + dtb_signature_filename[0] = 0;<br>
> +#endif<br>
> +<br>
> if (opt && opt->option->name)<br>
> boot_desc = opt->option->name;<br>
> else if (cmd && cmd->boot_image_file)<br>
> @@ -471,6 +799,12 @@ struct boot_task *boot(void *ctx, struct discover_boot_option *opt,<br>
> boot_task->dry_run = dry_run;<br>
> boot_task->status_fn = status_fn;<br>
> boot_task->status_arg = status_arg;<br>
> +#if defined(HAVE_LIBGPGME)<br>
> + if (access(LOCKDOWN_FILE, F_OK) == -1)<br>
> + boot_task->verify_signature = false;<br>
> + else<br>
> + boot_task->verify_signature = true;<br>
> +#endif<br>
><br>
> if (cmd && cmd->boot_args) {<br>
> boot_task->args = talloc_strdup(boot_task, cmd->boot_args);<br>
> @@ -485,6 +819,41 @@ struct boot_task *boot(void *ctx, struct discover_boot_option *opt,<br>
> rc = start_url_load(boot_task, "kernel image", image, &boot_task->image)<br>
> || start_url_load(boot_task, "initrd", initrd, &boot_task->initrd)<br>
> || start_url_load(boot_task, "dtb", dtb, &boot_task->dtb);<br>
> +#if defined(HAVE_LIBGPGME)<br>
> + if (boot_task->verify_signature) {<br>
> + /* Generate names of associated signature files and load */<br>
> + if (image) {<br>
> + snprintf(kernel_signature_filename, max_filename_size, "%s%s", image->file, ".sig");<br>
> + image_sig = pb_url_copy(ctx, image);<br>
> + talloc_free(image_sig->file);<br>
> + image_sig->file = talloc_strdup(image_sig, kernel_signature_filename);<br>
> + snprintf(kernel_signature_filename, max_filename_size, "%s%s", image->path, ".sig");<br>
> + talloc_free(image_sig->path);<br>
> + image_sig->path = talloc_strdup(image_sig, kernel_signature_filename);<br>
> + rc |= start_url_load(boot_task, "kernel image signature", image_sig, &boot_task->image_signature);<br>
<br>
</div></div>In other spots too, but keep to an 80-character limit<br>
<div><div class="gmail-h5"><br>
> + }<br>
> + if (initrd) {<br>
> + snprintf(initrd_signature_filename, max_filename_size, "%s%s", initrd->file, ".sig");<br>
> + initrd_sig = pb_url_copy(ctx, initrd);<br>
> + talloc_free(initrd_sig->file);<br>
> + initrd_sig->file = talloc_strdup(initrd_sig, initrd_signature_filename);<br>
> + snprintf(initrd_signature_filename, max_filename_size, "%s%s", initrd->path, ".sig");<br>
> + talloc_free(initrd_sig->path);<br>
> + initrd_sig->path = talloc_strdup(initrd_sig, initrd_signature_filename);<br>
> + rc |= start_url_load(boot_task, "initrd signature", initrd_sig, &boot_task->initrd_signature);<br>
> + }<br>
> + if (dtb) {<br>
> + snprintf(dtb_signature_filename, max_filename_size, "%s%s", dtb->file, ".sig");<br>
> + dtb_sig = pb_url_copy(ctx, dtb);<br>
> + talloc_free(dtb_sig->file);<br>
> + dtb_sig->file = talloc_strdup(dtb_sig, dtb_signature_filename);<br>
> + snprintf(dtb_signature_filename, max_filename_size, "%s%s", dtb->path, ".sig");<br>
> + talloc_free(dtb_sig->path);<br>
> + dtb_sig->path = talloc_strdup(dtb_sig, dtb_signature_filename);<br>
> + rc |= start_url_load(boot_task, "dtb signature", dtb_sig, &boot_task->dtb_signature);<br>
> + }<br>
> + }<br>
> +#endif<br>
><br>
> /* If all URLs are local, we may be done. */<br>
> if (rc) {<br>
> diff --git a/lib/url/url.c b/lib/url/url.c<br>
> index 7202f49..6eeced3 100644<br>
> --- a/lib/url/url.c<br>
> +++ b/lib/url/url.c<br>
> @@ -246,7 +246,7 @@ static void pb_url_update_full(struct pb_url *url)<br>
> url->full = pb_url_to_string(url);<br>
> }<br>
><br>
> -static struct pb_url *pb_url_copy(void *ctx, const struct pb_url *url)<br>
> +struct pb_url *pb_url_copy(void *ctx, const struct pb_url *url)<br>
> {<br>
> struct pb_url *new_url;<br>
><br>
> diff --git a/lib/url/url.h b/lib/url/url.h<br>
> index 25e1ad8..9043615 100644<br>
> --- a/lib/url/url.h<br>
> +++ b/lib/url/url.h<br>
> @@ -62,6 +62,7 @@ struct pb_url {<br>
><br>
> bool is_url(const char *str);<br>
> struct pb_url *pb_url_parse(void *ctx, const char *url_str);<br>
> +struct pb_url *pb_url_copy(void *ctx, const struct pb_url *url);<br>
> struct pb_url *pb_url_join(void *ctx, const struct pb_url *url, const char *s);<br>
> char *pb_url_to_string(struct pb_url *url);<br>
><br>
> diff --git a/ui/ncurses/nc-cui.c b/ui/ncurses/nc-cui.c<br>
> index 3f1e0a2..0d027a8 100644<br>
> --- a/ui/ncurses/nc-cui.c<br>
> +++ b/ui/ncurses/nc-cui.c<br>
> @@ -746,6 +746,9 @@ static struct pmenu *main_menu_init(struct cui *cui)<br>
> struct pmenu_item *i;<br>
> struct pmenu *m;<br>
> int result;<br>
> + bool lockdown = false;<br>
> + if (access(LOCKDOWN_FILE, F_OK) != -1)<br>
> + lockdown = true;<br>
><br>
> m = pmenu_init(cui, 7, cui_on_exit);<br>
> if (!m) {<br>
> @@ -789,9 +792,16 @@ static struct pmenu *main_menu_init(struct cui *cui)<br>
> i->on_execute = menu_add_url_execute;<br>
> pmenu_item_insert(m, i, 5);<br>
><br>
> - i = pmenu_item_create(m, _("Exit to shell"));<br>
> - i->on_execute = pmenu_exit_cb;<br>
> - pmenu_item_insert(m, i, 6);<br>
> + if (lockdown) {<br>
> + i = pmenu_item_create(m, _("Reboot"));<br>
> + i->on_execute = pmenu_exit_cb;<br>
> + pmenu_item_insert(m, i, 6);<br>
> + }<br>
> + else {<br>
> + i = pmenu_item_create(m, _("Exit to shell"));<br>
> + i->on_execute = pmenu_exit_cb;<br>
> + pmenu_item_insert(m, i, 6);<br>
> + }<br>
<br>
</div></div>The problem here is that while the "Exit to shell" button is replaced,<br>
users can still exit with 'x' or potentially some other key combination<br>
that causes ncurses to exit.<br>
Additionally it looks like the "Reboot" button just calls the exit<br>
callback anyway :)<br>
<br>
><br>
> result = pmenu_setup(m);<br>
<div class="gmail-HOEnZb"><div class="gmail-h5">><br>
<br>
_______________________________________________<br>
Petitboot mailing list<br>
<a href="mailto:Petitboot@lists.ozlabs.org">Petitboot@lists.ozlabs.org</a><br>
<a href="https://lists.ozlabs.org/listinfo/petitboot" rel="noreferrer" target="_blank">https://lists.ozlabs.org/listinfo/petitboot</a><br>
</div></div></blockquote></div><br></div></div>