Message ID | 1323056151.64551.1471055747097.JavaMail.zimbra@raptorengineeringinc.com |
---|---|
State | Superseded |
Headers | show |
On 08/12/2016 11:35 PM, Timothy Pearson wrote: [...] > diff --git a/configure.ac b/configure.ac > index 00a6113..370511b 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -170,6 +170,70 @@ AS_IF( > ] > ) > > +AC_ARG_WITH( > + [signed-boot], > + [AS_HELP_STRING([--with-signed-boot], > + [build kernel signature checking support [default=yes]] > + )], > + [], > + [with_signed_boot=yes] > +) > +AM_CONDITIONAL([WITH_SIGNED_BOOT], [test "x$with_signed_boot" = "xyes"]) > + > +AM_CONDITIONAL( > + [WITH_SIGNED_BOOT], > + [test "x$with_signed_boot" = "xyes"]) Is there any reason to define macro WITH_SIGNED_BOOT twice with different indentation? Or the macros are different and I'm overlooking them?
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 08/16/2016 01:50 PM, Murilo Opsfelder Araújo wrote: > On 08/12/2016 11:35 PM, Timothy Pearson wrote: > [...] >> diff --git a/configure.ac b/configure.ac >> index 00a6113..370511b 100644 >> --- a/configure.ac >> +++ b/configure.ac >> @@ -170,6 +170,70 @@ AS_IF( >> ] >> ) >> >> +AC_ARG_WITH( >> + [signed-boot], >> + [AS_HELP_STRING([--with-signed-boot], >> + [build kernel signature checking support [default=yes]] >> + )], >> + [], >> + [with_signed_boot=yes] >> +) >> +AM_CONDITIONAL([WITH_SIGNED_BOOT], [test "x$with_signed_boot" = "xyes"]) >> + >> +AM_CONDITIONAL( >> + [WITH_SIGNED_BOOT], >> + [test "x$with_signed_boot" = "xyes"]) > > Is there any reason to define macro WITH_SIGNED_BOOT twice with > different indentation? Or the macros are different and I'm overlooking > them? > No, I'm thinking this happened without my knowledge during a rebase. It's exactly the kind of glitch that tends to happen (old line not wrapped, new line wrapped before rebase). - -- 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/ iQEcBAEBAgAGBQJXs4IiAAoJEK+E3vEXDOFb9FcIALHkr4tsqfW8WXzkoXX0i5NS 3UX2S8OMhbEWwUezVEss6krq22I6cY2ExKoMNwlbEUwErWg5+vp+Wj5RCa4q5+9c ZhwodMayYz7LHJ738rD3FfR8hQnKvWXg1EP3nfGBhISvQ8Vyw1jZNg5Z4P27BHl8 NK8X/R9XDzROiCsXiMGFpZfxrdp6TIEbkQp+m3agT9qMyUiShiduuEvvBVmgcago tXPEDLuNigLCWrgwrrY9ATB+3Yjy0f/yIpCfiXqsYgDlFjwAoC1ubODaIystUIAm KJiPor78nVCgHdSvDOKzsmr7YCD6sOicgG++hrVbZliiZgoic7wQYEVbPmHgl0M= =A7DQ -----END PGP SIGNATURE-----
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 08/16/2016 04:14 PM, Timothy Pearson wrote: > On 08/16/2016 01:50 PM, Murilo Opsfelder Araújo wrote: >> Is there any reason to define macro WITH_SIGNED_BOOT twice with >> different indentation? Or the macros are different and I'm overlooking >> them? > > > No, I'm thinking this happened without my knowledge during a rebase. > It's exactly the kind of glitch that tends to happen (old line not > wrapped, new line wrapped before rebase). > Fixed in patch V6. - -- 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/ iQEcBAEBAgAGBQJXs5PEAAoJEK+E3vEXDOFbhCEIAJJucSX5sT+n61zpHxJs1bdE np09Rg9+IzqsVM2qa0M99GJ9ku1tl+KCvd/Yinjs3zr47BFnaRgS08ilj2+9YVPo 6koWNugQ2ryZv3f7almOYFhA0Pmcr92xGGmgRh+GP4nLFvxGvF8ZnZ/s3qNqBN39 R+8Q6dl7YAS32nJb4mS/3rlRBtKpooHfSUrjMZBRkdZz6fsbBANNIT3NytTKu4SR BFoZBaFb73AfT24Lu1DajlQRYGaYK14Tc6zzef8hdqQVKtEKEutdefWYuETpMFkB XDuDkeAI8fJRhdf+RhArwkHLivDqPYo154GYkb5ZpE0Ef+9MyPuCVlhSLAKkd4Q= =sb+Q -----END PGP SIGNATURE-----
On Fri, 2016-08-12 at 21:35 -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. > > Signed-off-by: Timothy Pearson <tpearson@raptorengineering.com> > --- > configure.ac | 64 +++++++ > discover/Makefile.am | 12 +- > discover/boot.c | 155 ++++++++++++++--- > discover/boot.h | 30 ++++ > discover/device-handler.c | 6 + > discover/device-handler.h | 1 + > discover/gpg.c | 381 ++++++++++++++++++++++++++++++++++++++++++ > discover/gpg.h | 73 ++++++++ > discover/grub2/builtins.c | 8 + > discover/kboot-parser.c | 6 + > discover/pxe-parser.c | 7 + > discover/user-event.c | 16 +- > discover/yaboot-parser.c | 7 + > lib/pb-protocol/pb-protocol.c | 13 ++ > lib/types/types.h | 2 + > ui/common/discover-client.c | 1 + > ui/common/discover-client.h | 1 + > ui/ncurses/nc-boot-editor.c | 51 +++++- > ui/ncurses/nc-cui.c | 2 + > 19 files changed, 807 insertions(+), 29 deletions(-) > create mode 100644 discover/gpg.c > create mode 100644 discover/gpg.h > > diff --git a/configure.ac b/configure.ac > index 00a6113..370511b 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -170,6 +170,70 @@ AS_IF( > ] > ) > > +AC_ARG_WITH( > + [signed-boot], > + [AS_HELP_STRING([--with-signed-boot], > + [build kernel signature checking support [default=yes]] > + )], > + [], > + [with_signed_boot=yes] > +) > +AM_CONDITIONAL([WITH_SIGNED_BOOT], [test "x$with_signed_boot" = "xyes"]) > + > +AM_CONDITIONAL( > + [WITH_SIGNED_BOOT], > + [test "x$with_signed_boot" = "xyes"]) > + > +AS_IF( > + [test "x$with_signed_boot" = "xyes"], > + [PKG_CHECK_MODULES( > + [GPGME], > + [gpgme >= 1.0.0], > + [SAVE_LIBS="$LIBS" LIBS="$LIBS $gpgme_LIBS" > + AC_CHECK_LIB( > + [gpgme], > + [gpgme_op_verify], > + [], > + [AC_MSG_FAILURE([--with-signed-boot was given but the test for gpgme failed.])] > + ) > + LIBS="$SAVE_LIBS" > + ], > + [AM_PATH_GPGME([1.0.0], [SAVE_LIBS="$LIBS" LIBS="$LIBS $gpgme_LIBS" > + AC_CHECK_LIB( > + [gpgme], > + [gpgme_op_verify], > + [], > + [AC_MSG_FAILURE([--with-signed-boot was given but the test for gpgme failed.])] > + ) > + LIBS="$SAVE_LIBS"], > + [AC_MSG_RESULT([$gpgme_PKG_ERRORS]) > + AC_MSG_FAILURE([ Consider adjusting PKG_CONFIG_PATH environment variable]) > + ]) > + ] > + )] > +) > + > +AS_IF( > + [test "x$with_signed_boot" = "xyes"], > + [SAVE_CPPFLAGS="$CPPFLAGS" CPPFLAGS="$CPPFLAGS $gpgme_CFLAGS" > + AC_CHECK_HEADERS( > + [gpgme.h], > + [], > + [AC_MSG_FAILURE([ --with-signed-boot given but gpgme.h not found])] > + ) > + CPPFLAGS="$SAVE_CPPFLAGS" > + ] > +) > + > +AM_CONDITIONAL([WITH_GPGME], [test "x$with_signed_boot" = "xyes"]) > + > +AC_ARG_VAR( > + [lockdown_file], > + [Location of authorized signature file [default = "/etc/pb-lockdown"]] > +) > +AS_IF([test "x$lockdown_file" = x], [lockdown_file="/etc/pb-lockdown"]) > +AC_DEFINE_UNQUOTED(LOCKDOWN_FILE, "$lockdown_file", [Lockdown file location]) > + > AC_ARG_ENABLE( > [busybox], > [AS_HELP_STRING( > diff --git a/discover/Makefile.am b/discover/Makefile.am > index 899c9a6..4a74da3 100644 > --- a/discover/Makefile.am > +++ b/discover/Makefile.am > @@ -15,6 +15,12 @@ > sbin_PROGRAMS += discover/pb-discover > noinst_PROGRAMS += discover/platform.ro > > +if WITH_GPGME > +gpg_int_SOURCES = discover/gpg.c > +else > +gpg_int_SOURCES = > +endif > + > discover_pb_discover_SOURCES = \ > discover/boot.c \ > discover/boot.h \ > @@ -52,13 +58,15 @@ discover_pb_discover_SOURCES = \ > discover/user-event.h \ > discover/kboot-parser.c \ > discover/yaboot-parser.c \ > - discover/pxe-parser.c > + discover/pxe-parser.c \ > + $(gpg_int_SOURCES) > > discover_pb_discover_LDADD = \ > discover/grub2/grub2-parser.ro \ > discover/platform.ro \ > $(core_lib) \ > - $(UDEV_LIBS) > + $(UDEV_LIBS) \ > + $(GPGME_LIBS) > > discover_pb_discover_LDFLAGS = \ > $(AM_LDFLAGS) \ > diff --git a/discover/boot.c b/discover/boot.c > index ba6ce25..38be536 100644 > --- a/discover/boot.c > +++ b/discover/boot.c > @@ -25,6 +25,7 @@ > #include "paths.h" > #include "resource.h" > #include "platform.h" > +#include "gpg.h" > > static const char *boot_hook_dir = PKG_SYSCONF_DIR "/boot.d"; > enum { > @@ -32,21 +33,6 @@ enum { > BOOT_HOOK_EXIT_UPDATE = 2, > }; > > -struct boot_task { > - struct load_url_result *image; > - struct load_url_result *initrd; > - struct load_url_result *dtb; > - const char *local_image; > - const char *local_initrd; > - const char *local_dtb; > - const char *args; > - const char *boot_tty; > - boot_status_fn status_fn; > - void *status_arg; > - bool dry_run; > - bool cancelled; > -}; > - > /** > * kexec_load - kexec load helper. > */ > @@ -59,20 +45,33 @@ static int kexec_load(struct boot_task *boot_task) > char *s_dtb = NULL; > char *s_args = NULL; > > + const char* local_initrd = boot_task->local_initrd; > + const char* local_dtb = boot_task->local_dtb; > + const char* local_image = boot_task->local_image; > + > + if ((result = gpg_validate_boot_files(boot_task, &local_initrd, 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. > + &local_dtb, &local_image))) { > + if (result == KEXEC_LOAD_SIGNATURE_FAILURE) { > + pb_log("%s: Aborting kexec due to" > + " signature verification failure\n", __func__); > + goto abort_kexec; > + } > + } > + > p = argv; > *p++ = pb_system_apps.kexec; /* 1 */ > *p++ = "-l"; /* 2 */ > > - if (boot_task->local_initrd) { > + if (local_initrd) { > s_initrd = talloc_asprintf(boot_task, "--initrd=%s", > - boot_task->local_initrd); > + local_initrd); > assert(s_initrd); > *p++ = s_initrd; /* 3 */ > } > > - if (boot_task->local_dtb) { > + if (local_dtb) { > s_dtb = talloc_asprintf(boot_task, "--dtb=%s", > - boot_task->local_dtb); > + local_dtb); > assert(s_dtb); > *p++ = s_dtb; /* 4 */ > } > @@ -84,7 +83,7 @@ static int kexec_load(struct boot_task *boot_task) > *p++ = s_args; /* 5 */ > } > > - *p++ = boot_task->local_image; /* 6 */ > + *p++ = local_image; /* 6 */ nitpick - indentation > *p++ = NULL; /* 7 */ > > result = process_run_simple_argv(boot_task, argv); > @@ -92,6 +91,10 @@ static int kexec_load(struct boot_task *boot_task) > if (result) > pb_log("%s: failed: (%d)\n", __func__, result); > > +abort_kexec: > + gpg_validate_boot_files_cleanup(boot_task, &local_initrd, &local_dtb, > + &local_image); > + > return result; > } > > @@ -378,23 +381,69 @@ static void boot_process(struct load_url_result *result, void *data) > check_load(task, "dtb", task->dtb)) > goto no_load; > > + if (task->verify_signature) { > + if (load_pending(task->image_signature) || > + load_pending(task->initrd_signature) || > + load_pending(task->dtb_signature) || > + load_pending(task->cmdline_signature)) > + return; > + > + if (check_load(task, "kernel image signature", > + task->image_signature) || > + check_load(task, "initrd signature", > + task->initrd_signature) || > + check_load(task, "dtb signature", > + task->dtb_signature) || > + check_load(task, "command line signature", > + task->cmdline_signature)) > + goto no_sig_load; > + } > + > /* we make a copy of the local paths, as the boot hooks might update > * and/or create these */ > task->local_image = task->image ? task->image->local : NULL; > task->local_initrd = task->initrd ? task->initrd->local : NULL; > task->local_dtb = task->dtb ? task->dtb->local : NULL; > > + if (task->verify_signature) { > + task->local_image_signature = task->image_signature ? > + task->image_signature->local : NULL; > + task->local_initrd_signature = task->initrd_signature ? > + task->initrd_signature->local : NULL; > + task->local_dtb_signature = task->dtb_signature ? > + task->dtb_signature->local : NULL; > + task->local_cmdline_signature = task->cmdline_signature ? > + task->cmdline_signature->local : NULL; > + } > + > run_boot_hooks(task); > > update_status(task->status_fn, task->status_arg, BOOT_STATUS_INFO, > _("performing kexec_load")); > > rc = kexec_load(task); > - if (rc) { > + if (rc == KEXEC_LOAD_SIGNATURE_FAILURE) { > + update_status(task->status_fn, task->status_arg, > + BOOT_STATUS_ERROR, > + _("signature verification failed")); > + } > + else if (rc == KEXEC_LOAD_SIG_SETUP_INVALID) { > update_status(task->status_fn, task->status_arg, > - BOOT_STATUS_ERROR, _("kexec load failed")); > + BOOT_STATUS_ERROR, > + _("invalid signature configuration")); > + } > + else if (rc) { > + update_status(task->status_fn, task->status_arg, > + BOOT_STATUS_ERROR, > + _("kexec load failed")); > } > > +no_sig_load: > + cleanup_load(task->image_signature); > + cleanup_load(task->initrd_signature); > + cleanup_load(task->dtb_signature); > + cleanup_load(task->cmdline_signature); > + > no_load: > cleanup_load(task->image); > cleanup_load(task->initrd); > @@ -435,6 +484,8 @@ struct boot_task *boot(void *ctx, struct discover_boot_option *opt, > boot_status_fn status_fn, void *status_arg) > { > struct pb_url *image = NULL, *initrd = NULL, *dtb = NULL; > + struct pb_url *image_sig = NULL, *initrd_sig = NULL, *dtb_sig = NULL, > + *cmdline_sig = NULL; > const struct config *config; > struct boot_task *boot_task; > const char *boot_desc; > @@ -478,6 +529,8 @@ struct boot_task *boot(void *ctx, struct discover_boot_option *opt, > boot_task->status_fn = status_fn; > boot_task->status_arg = status_arg; > > + boot_task->verify_signature = (lockdown_status() == PB_LOCKDOWN_SIGN); > + > if (cmd && cmd->boot_args) { > boot_task->args = talloc_strdup(boot_task, cmd->boot_args); > } else if (opt && opt->option->boot_args) { > @@ -494,11 +547,69 @@ struct boot_task *boot(void *ctx, struct discover_boot_option *opt, > boot_task->boot_tty = config ? config->boot_tty : NULL; > } > > + if (boot_task->verify_signature) { > + if (cmd && cmd->args_sig_file) { > + cmdline_sig = pb_url_parse(opt, cmd->args_sig_file); > + } else if (opt && opt->args_sig_file) { > + cmdline_sig = opt->args_sig_file->url; > + } else { > + pb_log("%s: no command line signature file" > + " specified\n", __func__); > + update_status(status_fn, status_arg, BOOT_STATUS_INFO, > + _("Boot failed: no command line" > + " signature file specified")); > + return NULL; boot_task should probably get freed here. > + } > + } > + > /* start async loads for boot resources */ > rc = start_url_load(boot_task, "kernel image", image, &boot_task->image) > || start_url_load(boot_task, "initrd", initrd, &boot_task->initrd) > || start_url_load(boot_task, "dtb", dtb, &boot_task->dtb); > > + if (boot_task->verify_signature) { > + /* Generate names of associated signature files and load */ > + if (image) { > + image_sig = pb_url_copy(ctx, image); > + talloc_free(image_sig->file); > + image_sig->file = talloc_asprintf(image_sig, > + "%s.sig", image->file); > + talloc_free(image_sig->path); > + image_sig->path = talloc_asprintf(image_sig, > + "%s.sig", image->path); 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) ? > + rc |= start_url_load(boot_task, > + "kernel image signature", image_sig, > + &boot_task->image_signature); > + } > + if (initrd) { > + initrd_sig = pb_url_copy(ctx, initrd); > + talloc_free(initrd_sig->file); > + initrd_sig->file = talloc_asprintf(initrd_sig, > + "%s.sig", initrd->file); > + talloc_free(initrd_sig->path); > + initrd_sig->path = talloc_asprintf(initrd_sig, > + "%s.sig", initrd->path); > + rc |= start_url_load(boot_task, "initrd signature", > + initrd_sig, &boot_task->initrd_signature); > + } > + if (dtb) { > + dtb_sig = pb_url_copy(ctx, dtb); > + talloc_free(dtb_sig->file); > + dtb_sig->file = talloc_asprintf(dtb_sig, > + "%s.sig", dtb->file); > + talloc_free(dtb_sig->path); > + dtb_sig->path = talloc_asprintf(dtb_sig, > + "%s.sig", dtb->path); > + rc |= start_url_load(boot_task, "dtb signature", > + dtb_sig, &boot_task->dtb_signature); > + } > + > + rc |= start_url_load(boot_task, > + "kernel command line signature", cmdline_sig, > + &boot_task->cmdline_signature); > + } > + > + /* If all URLs are local, we may be done. */ > if (rc) { > /* Don't call boot_cancel() to preserve the status update */ > boot_task->cancelled = true; > diff --git a/discover/boot.h b/discover/boot.h > index ec61703..5f6e874 100644 > --- a/discover/boot.h > +++ b/discover/boot.h > @@ -11,4 +11,34 @@ struct boot_task *boot(void *ctx, struct discover_boot_option *opt, > boot_status_fn status_fn, void *status_arg); > > void boot_cancel(struct boot_task *task); > + > +struct boot_task { > + struct load_url_result *image; > + struct load_url_result *initrd; > + struct load_url_result *dtb; > + const char *local_image; > + const char *local_initrd; > + const char *local_dtb; > + const char *args; > + const char *boot_tty; > + boot_status_fn status_fn; > + void *status_arg; > + bool dry_run; > + bool cancelled; > + bool verify_signature; > + struct load_url_result *image_signature; > + struct load_url_result *initrd_signature; > + struct load_url_result *dtb_signature; > + struct load_url_result *cmdline_signature; > + const char *local_image_signature; > + const char *local_initrd_signature; > + const char *local_dtb_signature; > + const char *local_cmdline_signature; > +}; > + > +enum { > + KEXEC_LOAD_SIG_SETUP_INVALID = 253, > + KEXEC_LOAD_SIGNATURE_FAILURE = 254, > +}; > + > #endif /* _BOOT_H */ > diff --git a/discover/device-handler.c b/discover/device-handler.c > index c31fcea..f6b6d22 100644 > --- a/discover/device-handler.c > +++ b/discover/device-handler.c > @@ -613,6 +613,7 @@ static bool __attribute__((used)) boot_option_is_resolved( > return resource_is_resolved(opt->boot_image) && > resource_is_resolved(opt->initrd) && > resource_is_resolved(opt->dtb) && > + resource_is_resolved(opt->args_sig_file) && > resource_is_resolved(opt->icon); > } > > @@ -638,6 +639,8 @@ static bool boot_option_resolve(struct discover_boot_option *opt, > return resource_resolve(opt->boot_image, "boot_image", opt, handler) && > resource_resolve(opt->initrd, "initrd", opt, handler) && > resource_resolve(opt->dtb, "dtb", opt, handler) && > + resource_resolve(opt->args_sig_file, "args_sig_file", opt, > + handler) && > resource_resolve(opt->icon, "icon", opt, handler); > } > > @@ -652,6 +655,7 @@ static void boot_option_finalise(struct device_handler *handler, > assert(!opt->option->dtb_file); > assert(!opt->option->icon_file); > assert(!opt->option->device_id); > + assert(!opt->option->args_sig_file); > > if (opt->boot_image) > opt->option->boot_image_file = opt->boot_image->url->full; > @@ -661,6 +665,8 @@ static void boot_option_finalise(struct device_handler *handler, > opt->option->dtb_file = opt->dtb->url->full; > if (opt->icon) > opt->option->icon_file = opt->icon->url->full; > + if (opt->args_sig_file) > + opt->option->args_sig_file = opt->args_sig_file->url->full; > > opt->option->device_id = opt->device->device->id; > > diff --git a/discover/device-handler.h b/discover/device-handler.h > index b6f9fd5..f785ccc 100644 > --- a/discover/device-handler.h > +++ b/discover/device-handler.h > @@ -48,6 +48,7 @@ struct discover_boot_option { > struct resource *boot_image; > struct resource *initrd; > struct resource *dtb; > + struct resource *args_sig_file; > struct resource *icon; > }; > > diff --git a/discover/gpg.c b/discover/gpg.c > new file mode 100644 > index 0000000..e1c1d04 > --- /dev/null > +++ b/discover/gpg.c > @@ -0,0 +1,381 @@ > +/* > + * Copyright (C) 2016 Raptor Engineering, LLC > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; version 2 of the License. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, write to the Free Software > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA > + */ > + > +#if defined(HAVE_CONFIG_H) > +#include "config.h" > +#endif > + > +#include <stdbool.h> > +#include <stdlib.h> > +#include <assert.h> > +#include <dirent.h> > +#include <string.h> > +#include <fcntl.h> > +#include <sys/types.h> > + > +#include <log/log.h> > +#include <talloc/talloc.h> > +#include <url/url.h> > +#include <util/util.h> > +#include <i18n/i18n.h> > + > +#include "gpg.h" > + > +int copy_file_to_destination(const char * source_file, > + char * destination_file, int max_dest_filename_size) { nitpick - char *destination_file, etc > + int result = 0; > + char template[21] = "/tmp/petitbootXXXXXX"; Don't think you need to explicitly set the array length here. > + FILE *source_handle = fopen(source_file, "r"); > + int destination_fd = mkstemp(template); > + FILE *destination_handle = fdopen(destination_fd, "w"); > + if (!source_handle || !(destination_handle)) { > + // handle open error > + pb_log("%s: failed: unable to open source file '%s'\n", > + __func__, source_file); > + result = 1; Probably best to bail out straight away here. > + } > + > + size_t l1; > + unsigned char buffer[FILE_XFER_BUFFER_SIZE]; talloc() this instead? > + > + /* Copy data */ > + while ((l1 = fread(buffer, 1, sizeof buffer, source_handle)) > 0) { > + size_t l2 = fwrite(buffer, 1, l1, destination_handle); > + if (l2 < l1) { > + if (ferror(destination_handle)) { > + /* General error */ > + result = 1; Change these last few debug errors to -1 > + pb_log("%s: failed: unknown fault\n", __func__); > + } > + else { > + /* No space on destination device */ > + result = 2; > + pb_log("%s: failed: temporary storage full\n", > + __func__); Why not just break; here? It looks like even if there's an error we'll continue to read the source file until finished. > + } > + } > + } > + > + if (result) { > + destination_file[0] = '\0'; > + } > + else { > + ssize_t r; > + char readlink_buffer[MAX_FILENAME_SIZE]; > + snprintf(readlink_buffer, MAX_FILENAME_SIZE, "/proc/self/fd/%d", > + destination_fd); > + r = readlink(readlink_buffer, destination_file, > + max_dest_filename_size); > + if (r < 0) { > + /* readlink failed */ > + result = 3; Debug error > + pb_log("%s: failed: unable to obtain temporary filename" > + "\n", __func__); > + } > + destination_file[r] = '\0'; > + } > + > + fclose(source_handle); > + fclose(destination_handle); > + > + return result; > +} > + > +int verify_file_signature(const char * plaintext_filename, > + const char * signature_filename, FILE * authorized_signatures_handle, > + const char * keyring_path) const char *variable > +{ > + int valid = 0; > + > + if (signature_filename == NULL) > + return -1; > + > + gpgme_signature_t verification_signatures; > + gpgme_verify_result_t verification_result; > + gpgme_data_t plaintext_data; > + gpgme_data_t signature_data; > + gpgme_engine_info_t enginfo; > + gpgme_ctx_t gpg_context; > + gpgme_error_t err; nitpick - put these before the if (signature..) check > + > + /* Initialize gpgme */ > + setlocale (LC_ALL, ""); > + gpgme_check_version(NULL); > + gpgme_set_locale(NULL, LC_CTYPE, setlocale (LC_CTYPE, NULL)); > + err = gpgme_engine_check_version(GPGME_PROTOCOL_OpenPGP); > + if (err != GPG_ERR_NO_ERROR) { > + pb_log("%s: OpenPGP support not available\n", __func__); > + return -1; > + } > + err = gpgme_get_engine_info(&enginfo); > + if (err != GPG_ERR_NO_ERROR) { > + pb_log("%s: GPG engine failed to initialize\n", __func__); > + return -1; > + } > + err = gpgme_new(&gpg_context); > + if (err != GPG_ERR_NO_ERROR) { > + pb_log("%s: GPG context could not be created\n", __func__); > + return -1; > + } > + err = gpgme_set_protocol(gpg_context, GPGME_PROTOCOL_OpenPGP); > + if (err != GPG_ERR_NO_ERROR) { > + pb_log("%s: GPG protocol could not be set\n", __func__); > + return -1; > + } > + if (keyring_path) > + err = gpgme_ctx_set_engine_info (gpg_context, > + GPGME_PROTOCOL_OpenPGP, enginfo->file_name, > + keyring_path); > + else > + err = gpgme_ctx_set_engine_info (gpg_context, > + GPGME_PROTOCOL_OpenPGP, enginfo->file_name, > + enginfo->home_dir); Is there ever a time you expect to not have keyring_path? > + if (err != GPG_ERR_NO_ERROR) { > + pb_log("%s: Could not set GPG engine information\n", __func__); > + return -1; > + } > + err = gpgme_data_new_from_file(&plaintext_data, plaintext_filename, 1); > + if (err != GPG_ERR_NO_ERROR) { > + pb_log("%s: Could not create GPG plaintext data buffer" > + " from file '%s'\n", __func__, plaintext_filename); > + return -1; > + } > + err = gpgme_data_new_from_file(&signature_data, signature_filename, 1); > + if (err != GPG_ERR_NO_ERROR) { > + pb_log("%s: Could not create GPG signature data buffer" > + " from file '%s'\n", __func__, signature_filename); > + return -1; > + } > + > + /* Check signature */ > + err = gpgme_op_verify(gpg_context, signature_data, plaintext_data, > + NULL); > + if (err != GPG_ERR_NO_ERROR) { > + pb_log("%s: Could not verify file using GPG signature '%s'\n", > + __func__, signature_filename); > + return -1; > + } > + verification_result = gpgme_op_verify_result(gpg_context); > + verification_signatures = verification_result->signatures; > + while (verification_signatures) { > + if (verification_signatures->status == GPG_ERR_NO_ERROR) { > + pb_log("%s: Good signature for key ID '%s' ('%s')\n", > + __func__, verification_signatures->fpr, > + signature_filename); > + /* Verify fingerprint is present in > + * authorized signatures file > + */ > + char *auth_sig_line = NULL; > + size_t auth_sig_len = 0; > + ssize_t auth_sig_read; > + rewind(authorized_signatures_handle); > + while ((auth_sig_read = getline(&auth_sig_line, > + &auth_sig_len, > + authorized_signatures_handle)) != -1) { > + auth_sig_len = strlen(auth_sig_line); > + while ((auth_sig_line[auth_sig_len-1] == '\n') > + || (auth_sig_line[auth_sig_len-1] == '\r')) > + auth_sig_len--; > + auth_sig_line[auth_sig_len] = '\0'; > + if (strcmp(auth_sig_line, > + verification_signatures->fpr) == 0) > + valid = 1; 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? > + } > + free(auth_sig_line); > + } > + else { Would it be worth doing if (verification_signatures->status != GPG_ERR_NO_ERROR) { pb_log(...); continue; } first instead to save yourself an indent of the above block? > + pb_log("%s: Signature for key ID '%s' ('%s') invalid." > + " Status: %08x\n", __func__, > + verification_signatures->fpr, > + signature_filename, > + verification_signatures->status); > + } > + verification_signatures = verification_signatures->next; > + } > + > + /* Clean up */ > + gpgme_data_release(plaintext_data); > + gpgme_data_release(signature_data); > + gpgme_release(gpg_context); > + > + if (!valid) { > + pb_log("%s: Incorrect GPG signature\n", __func__); > + return -1; > + } > + else { Don't need this else since we return above > + pb_log("%s: GPG signature '%s' for file '%s' verified\n", > + __func__, signature_filename, plaintext_filename); > + } > + > + return 0; > +} > + > +int gpg_validate_boot_files(struct boot_task *boot_task, > + const char** local_initrd, const char** local_dtb, > + const char** local_image) { Assume I nitpick the rest of these too :) > + int result = 0; > + > + const char* local_initrd_signature = (boot_task->verify_signature) ? > + boot_task->local_initrd_signature : NULL; > + const char* local_dtb_signature = (boot_task->verify_signature) ? > + boot_task->local_dtb_signature : NULL; > + const char* local_image_signature = (boot_task->verify_signature) ? > + boot_task->local_image_signature : NULL; > + const char* local_cmdline_signature = (boot_task->verify_signature) ? > + boot_task->local_cmdline_signature : NULL; > + > + if (boot_task->verify_signature) { 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. > + char kernel_filename[MAX_FILENAME_SIZE]; > + char initrd_filename[MAX_FILENAME_SIZE]; > + char dtb_filename[MAX_FILENAME_SIZE]; > + > + kernel_filename[0] = '\0'; > + initrd_filename[0] = '\0'; > + dtb_filename[0] = '\0'; > + > + FILE *authorized_signatures_handle = NULL; > + > + /* Load authorized signatures file */ > + authorized_signatures_handle = fopen(LOCKDOWN_FILE, "r"); > + if (!authorized_signatures_handle) { > + pb_log("%s: unable to read lockdown file\n", __func__); > + result = KEXEC_LOAD_SIG_SETUP_INVALID; > + return result; Can just do return KEXEC_LOAD_SIG_SETUP_INVALID; > + } > + > + /* Copy files to temporary directory for verification / boot */ > + result = copy_file_to_destination(boot_task->local_image, > + kernel_filename, MAX_FILENAME_SIZE); 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? > + if (result) { > + pb_log("%s: image copy failed: (%d)\n", > + __func__, result); > + return result; > + } > + if (boot_task->local_initrd) { > + result = copy_file_to_destination( > + boot_task->local_initrd, > + initrd_filename, MAX_FILENAME_SIZE); > + if (result) { > + pb_log("%s: initrd copy failed: (%d)\n", > + __func__, result); > + unlink(*local_image); > + return result; 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. > + } > + } > + if (boot_task->local_dtb) { > + result = copy_file_to_destination(boot_task->local_dtb, > + dtb_filename, MAX_FILENAME_SIZE); > + if (result) { > + pb_log("%s: dtb copy failed: (%d)\n", > + __func__, result); > + unlink(*local_image); > + if (*local_initrd) > + unlink(*local_initrd); > + return result; > + } > + } > + *local_image = talloc_strdup(boot_task, > + kernel_filename); > + if (boot_task->local_initrd) > + *local_initrd = talloc_strdup(boot_task, > + initrd_filename); > + if (boot_task->local_dtb) > + *local_dtb = talloc_strdup(boot_task, > + dtb_filename); > + > + /* Write command line to temporary file for verification */ > + char cmdline_template[21] = "/tmp/petitbootXXXXXX"; > + int cmdline_fd = mkstemp(cmdline_template); > + FILE *cmdline_handle = NULL; (especially if we change to if (!verify_signature) as above), declare these variables at the start of the function. > + if (cmdline_fd < 0) { > + // handle mkstemp error style nitpick - use /* blah */ instead of // blah > + pb_log("%s: failed: unable to create command line" > + " temporary file for verification\n", > + __func__); > + result = -1; > + } > + else { > + cmdline_handle = fdopen(cmdline_fd, "w"); > + } > + if (!cmdline_handle) { > + // handle open error > + pb_log("%s: failed: unable to write command line" > + " temporary file for verification\n", > + __func__); > + result = -1; > + } > + else { > + fwrite(boot_task->args, sizeof(char), > + strlen(boot_task->args), cmdline_handle); > + fflush(cmdline_handle); > + } > + > + /* Check signatures */ > + if (verify_file_signature(kernel_filename, > + local_image_signature, > + authorized_signatures_handle, "/etc/gpg")) > + result = KEXEC_LOAD_SIGNATURE_FAILURE; > + if (verify_file_signature(cmdline_template, > + local_cmdline_signature, > + authorized_signatures_handle, "/etc/gpg")) > + result = KEXEC_LOAD_SIGNATURE_FAILURE; > + if (boot_task->local_initrd_signature) > + if (verify_file_signature(initrd_filename, > + local_initrd_signature, > + authorized_signatures_handle, "/etc/gpg")) > + result = KEXEC_LOAD_SIGNATURE_FAILURE; > + if (boot_task->local_dtb_signature) > + if (verify_file_signature(dtb_filename, > + local_dtb_signature, > + authorized_signatures_handle, "/etc/gpg")) > + result = KEXEC_LOAD_SIGNATURE_FAILURE; > + > + /* Clean up */ > + if (cmdline_handle) { > + fclose(cmdline_handle); > + unlink(cmdline_template); > + } > + fclose(authorized_signatures_handle); > + } > + > + return result; > +} > + > +void gpg_validate_boot_files_cleanup(struct boot_task *boot_task, > + const char** local_initrd, const char** local_dtb, > + const char** local_image) { > + if (boot_task->verify_signature) { > + unlink(*local_image); > + if (*local_initrd) > + unlink(*local_initrd); > + if (*local_dtb) > + unlink(*local_dtb); > + > + talloc_free((char*)*local_image); > + if (*local_initrd) > + talloc_free((char*)*local_initrd); > + if (*local_dtb) > + talloc_free((char*)*local_dtb); > + } Related to a comment below, neater if these aren't const chars and the casts can be dropped. > +} > + > +int lockdown_status() { > + if (access(LOCKDOWN_FILE, F_OK) == -1) > + return PB_LOCKDOWN_NONE; > + else > + return PB_LOCKDOWN_SIGN; > +} > \ No newline at end of file > diff --git a/discover/gpg.h b/discover/gpg.h > new file mode 100644 > index 0000000..9b452b8 > --- /dev/null > +++ b/discover/gpg.h > @@ -0,0 +1,73 @@ > +/* > + * Copyright (C) 2016 Raptor Engineering, LLC > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; version 2 of the License. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, write to the Free Software > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA > + */ > + > +#ifndef _PB_GPG_H > +#define _PB_GPG_H > + > +#define MAX_FILENAME_SIZE 8192 > +#define FILE_XFER_BUFFER_SIZE 8192 > + > +#include "boot.h" > + > +enum { > + PB_LOCKDOWN_NONE = 0, > + PB_LOCKDOWN_SIGN = 1, > +}; > + > +#if defined(HAVE_LIBGPGME) > +#include <gpgme.h> > +#endif /* HAVE_LIBGPGME */ > + > +int lockdown_status(void); > + > +int copy_file_to_destination(const char * source_file, > + char * destination_file, int max_dest_filename_size); > + > +int verify_file_signature(const char * plaintext_filename, > + const char * signature_filename, FILE * authorized_signatures_handle, > + const char * keyring_path); > + > +int gpg_validate_boot_files(struct boot_task *boot_task, > + const char** local_initrd, const char** local_dtb, > + const char** local_image); > + > +void gpg_validate_boot_files_cleanup(struct boot_task *boot_task, > + const char** local_initrd, const char** local_dtb, > + const char** local_image); > + > +#if !defined(HAVE_LIBGPGME) > + > +int lockdown_status(void) { return PB_LOCKDOWN_NONE; } > + > +int copy_file_to_destination(const char * source_file, > + char * destination_file, int max_dest_filename_size) { return -1; } > + > +int verify_file_signature(const char * plaintext_filename, > + const char * signature_filename, FILE * authorized_signatures_handle, > + const char * keyring_path) { return -1; } > + > +int gpg_validate_boot_files(struct boot_task *boot_task, > + const char** local_initrd, const char** local_dtb, > + const char** local_image) { return 0; } > + > +void gpg_validate_boot_files_cleanup(struct boot_task *boot_task, > + const char** local_initrd, const char** local_dtb, > + const char** local_image) { } > + > +#endif /* HAVE_LIBGPGME */ > + > +#endif /* _PB_GPG_H */ > \ No newline at end of file > diff --git a/discover/grub2/builtins.c b/discover/grub2/builtins.c > index 8bff732..c16b639 100644 > --- a/discover/grub2/builtins.c > +++ b/discover/grub2/builtins.c > @@ -6,7 +6,9 @@ > #include <types/types.h> > #include <talloc/talloc.h> > #include <util/util.h> > +#include <url/url.h> > > +#include "discover/resource.h" > #include "discover/parser.h" > #include "grub2.h" > > @@ -69,6 +71,12 @@ static int builtin_linux(struct grub2_script *script, > opt->option->boot_args = talloc_asprintf_append( > opt->option->boot_args, > " %s", argv[i]); > + > + char* args_sigfile_default = talloc_asprintf(opt, > + "%s.cmdline.sig", argv[1]); > + opt->args_sig_file = create_grub2_resource(opt, script->ctx->device, > + root, args_sigfile_default); > + talloc_free(args_sigfile_default); > return 0; > } > > diff --git a/discover/kboot-parser.c b/discover/kboot-parser.c > index cebe787..f7f75e0 100644 > --- a/discover/kboot-parser.c > +++ b/discover/kboot-parser.c > @@ -96,6 +96,12 @@ out_add: > d_opt->boot_image = create_devpath_resource(d_opt, > conf->dc->device, value); > > + char* args_sigfile_default = talloc_asprintf(d_opt, > + "%s.cmdline.sig", value); > + d_opt->args_sig_file = create_devpath_resource(d_opt, > + conf->dc->device, args_sigfile_default); > + talloc_free(args_sigfile_default); > + > if (root) { > opt->boot_args = talloc_asprintf(opt, "root=%s %s", root, args); > talloc_free(args); > diff --git a/discover/pxe-parser.c b/discover/pxe-parser.c > index 4481e5d..4aae8b1 100644 > --- a/discover/pxe-parser.c > +++ b/discover/pxe-parser.c > @@ -166,6 +166,13 @@ static void pxe_process_pair(struct conf_context *ctx, > url = pxe_url_join(ctx->dc, ctx->dc->conf_url, value); > opt->boot_image = create_url_resource(opt, url); > > + char* args_sigfile_default = talloc_asprintf(opt, > + "%s.cmdline.sig", value); > + url = pxe_url_join(ctx->dc, ctx->dc->conf_url, > + args_sigfile_default); > + opt->args_sig_file = create_url_resource(opt, url); > + talloc_free(args_sigfile_default); > + > } else if (streq(name, "INITRD")) { > url = pxe_url_join(ctx->dc, ctx->dc->conf_url, value); > opt->initrd = create_url_resource(opt, url); > diff --git a/discover/user-event.c b/discover/user-event.c > index 7350b6c..6ea754f 100644 > --- a/discover/user-event.c > +++ b/discover/user-event.c > @@ -82,7 +82,7 @@ static void user_event_print_event(struct event __attribute__((unused)) *event) > } > > static struct resource *user_event_resource(struct discover_boot_option *opt, > - struct event *event) > + struct event *event, bool gen_boot_args_sigfile) 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? > { > const char *siaddr, *boot_file; > struct resource *res; > @@ -101,7 +101,16 @@ static struct resource *user_event_resource(struct discover_boot_option *opt, > return NULL; > } > > - url_str = talloc_asprintf(opt, "%s%s/%s", "tftp://", siaddr, boot_file); > + if (gen_boot_args_sigfile) { > + char* args_sigfile_default = talloc_asprintf(opt, > + "%s.cmdline.sig", boot_file); > + url_str = talloc_asprintf(opt, "%s%s/%s", "tftp://", siaddr, > + args_sigfile_default); > + talloc_free(args_sigfile_default); > + } > + else > + url_str = talloc_asprintf(opt, "%s%s/%s", "tftp://", siaddr, > + boot_file); > url = pb_url_parse(opt, url_str); > talloc_free(url_str); > > @@ -143,12 +152,13 @@ static int parse_user_event(struct discover_context *ctx, struct event *event) > opt->id = talloc_asprintf(opt, "%s#%s", dev->id, val); > opt->name = talloc_strdup(opt, val); > > - d_opt->boot_image = user_event_resource(d_opt, event); > + d_opt->boot_image = user_event_resource(d_opt, event, false); > if (!d_opt->boot_image) { > pb_log("%s: no boot image found for %s!\n", __func__, > opt->name); > goto fail_opt; > } > + d_opt->args_sig_file = user_event_resource(d_opt, event, true); > > val = event_get_param(event, "rootpath"); > if (val) { > diff --git a/discover/yaboot-parser.c b/discover/yaboot-parser.c > index aa99392..b62f39d 100644 > --- a/discover/yaboot-parser.c > +++ b/discover/yaboot-parser.c > @@ -114,6 +114,13 @@ static void yaboot_finish(struct conf_context *conf) > /* populate the boot option from state data */ > state->opt->boot_image = create_yaboot_devpath_resource(state, > conf, state->boot_image); > + > + char* args_sigfile_default = talloc_asprintf(opt, > + "%s.cmdline.sig", state->boot_image); > + state->opt->args_sig_file = create_yaboot_devpath_resource(state, > + conf, args_sigfile_default); > + talloc_free(args_sigfile_default); > + > if (state->initrd) { > state->opt->initrd = create_yaboot_devpath_resource(state, > conf, state->initrd); > diff --git a/lib/pb-protocol/pb-protocol.c b/lib/pb-protocol/pb-protocol.c > index 7887fb0..ad7798e 100644 > --- a/lib/pb-protocol/pb-protocol.c > +++ b/lib/pb-protocol/pb-protocol.c > @@ -37,6 +37,7 @@ > * 4-byte len, initrd_file > * 4-byte len, dtb_file > * 4-byte len, boot_args > + * 4-byte len, args_sig_file > * > * action = 0x2: device remove message > * payload: > @@ -49,6 +50,7 @@ > * 4-byte len, initrd_file > * 4-byte len, dtb_file > * 4-byte len, boot_args > + * 4-byte len, args_sig_file > * > */ > > @@ -72,6 +74,7 @@ void pb_protocol_dump_device(const struct device *dev, const char *text, > fprintf(stream, "%s\t\tinit: %s\n", text, opt->initrd_file); > fprintf(stream, "%s\t\tdtb: %s\n", text, opt->dtb_file); > fprintf(stream, "%s\t\targs: %s\n", text, opt->boot_args); > + fprintf(stream, "%s\t\tasig: %s\n", text, opt->args_sig_file); > } > } > > @@ -197,6 +200,7 @@ int pb_protocol_boot_option_len(const struct boot_option *opt) > 4 + optional_strlen(opt->initrd_file) + > 4 + optional_strlen(opt->dtb_file) + > 4 + optional_strlen(opt->boot_args) + > + 4 + optional_strlen(opt->args_sig_file) + > sizeof(opt->is_default); > } > > @@ -207,6 +211,7 @@ int pb_protocol_boot_len(const struct boot_command *boot) > 4 + optional_strlen(boot->initrd_file) + > 4 + optional_strlen(boot->dtb_file) + > 4 + optional_strlen(boot->boot_args) + > + 4 + optional_strlen(boot->args_sig_file) + > 4 + optional_strlen(boot->tty); > } > > @@ -360,6 +365,7 @@ int pb_protocol_serialise_boot_option(const struct boot_option *opt, > pos += pb_protocol_serialise_string(pos, opt->initrd_file); > pos += pb_protocol_serialise_string(pos, opt->dtb_file); > pos += pb_protocol_serialise_string(pos, opt->boot_args); > + pos += pb_protocol_serialise_string(pos, opt->args_sig_file); > > *(bool *)pos = opt->is_default; > pos += sizeof(bool); > @@ -380,6 +386,7 @@ int pb_protocol_serialise_boot_command(const struct boot_command *boot, > pos += pb_protocol_serialise_string(pos, boot->initrd_file); > pos += pb_protocol_serialise_string(pos, boot->dtb_file); > pos += pb_protocol_serialise_string(pos, boot->boot_args); > + pos += pb_protocol_serialise_string(pos, boot->args_sig_file); > pos += pb_protocol_serialise_string(pos, boot->tty); > > assert(pos <= buf + buf_len); > @@ -750,6 +757,9 @@ int pb_protocol_deserialise_boot_option(struct boot_option *opt, > if (read_string(opt, &pos, &len, &opt->boot_args)) > goto out; > > + if (read_string(opt, &pos, &len, &opt->args_sig_file)) > + goto out; > + > if (len < sizeof(bool)) > goto out; > opt->is_default = *(bool *)(pos); > @@ -785,6 +795,9 @@ int pb_protocol_deserialise_boot_command(struct boot_command *cmd, > if (read_string(cmd, &pos, &len, &cmd->boot_args)) > goto out; > > + if (read_string(cmd, &pos, &len, &cmd->args_sig_file)) > + goto out; > + > if (read_string(cmd, &pos, &len, &cmd->tty)) > goto out; > > diff --git a/lib/types/types.h b/lib/types/types.h > index 5c5f6ed..6b607cd 100644 > --- a/lib/types/types.h > +++ b/lib/types/types.h > @@ -52,6 +52,7 @@ struct boot_option { > char *initrd_file; > char *dtb_file; > char *boot_args; > + char *args_sig_file; > bool is_default; > > struct list_item list; > @@ -65,6 +66,7 @@ struct boot_command { > char *initrd_file; > char *dtb_file; > char *boot_args; > + char *args_sig_file; > char *tty; > }; > > diff --git a/ui/common/discover-client.c b/ui/common/discover-client.c > index 6247dd0..5dbd99b 100644 > --- a/ui/common/discover-client.c > +++ b/ui/common/discover-client.c > @@ -312,6 +312,7 @@ static void create_boot_command(struct boot_command *command, > command->initrd_file = data->initrd; > command->dtb_file = data->dtb; > command->boot_args = data->args; > + command->args_sig_file = data->args_sig_file; > command->tty = ttyname(STDIN_FILENO); > } > > diff --git a/ui/common/discover-client.h b/ui/common/discover-client.h > index 542a275..59d2df9 100644 > --- a/ui/common/discover-client.h > +++ b/ui/common/discover-client.h > @@ -11,6 +11,7 @@ struct pb_boot_data { > char *initrd; > char *dtb; > char *args; > + char *args_sig_file; > }; > > /** > diff --git a/ui/ncurses/nc-boot-editor.c b/ui/ncurses/nc-boot-editor.c > index 4012ec5..9788fef 100644 > --- a/ui/ncurses/nc-boot-editor.c > +++ b/ui/ncurses/nc-boot-editor.c > @@ -63,6 +63,8 @@ struct boot_editor { > struct nc_widget_textbox *dtb_f; > struct nc_widget_label *args_l; > struct nc_widget_textbox *args_f; > + struct nc_widget_label *args_sig_file_l; > + struct nc_widget_textbox *args_sig_file_f; > struct nc_widget_button *ok_b; > struct nc_widget_button *help_b; > struct nc_widget_button *cancel_b; > @@ -73,6 +75,9 @@ struct boot_editor { > char *initrd; > char *dtb; > char *args; > + char *args_sig_file; > + > + bool use_signature_files; > }; > > extern const struct help_text boot_editor_help_text; > @@ -198,6 +203,12 @@ static struct pb_boot_data *boot_editor_prepare_data( > s = widget_textbox_get_value(boot_editor->widgets.args_f); > bd->args = *s ? talloc_strdup(bd, s) : NULL; > > + if (boot_editor->use_signature_files) { > + s = widget_textbox_get_value( > + boot_editor->widgets.args_sig_file_f); > + bd->args_sig_file = conditional_prefix(bd, prefix, s); > + } > + > return bd; > } > > @@ -323,6 +334,12 @@ static void boot_editor_layout_widgets(struct boot_editor *boot_editor) > y += layout_pair(boot_editor, y, boot_editor->widgets.args_l, > boot_editor->widgets.args_f); > > + if (boot_editor->use_signature_files) { > + y += layout_pair(boot_editor, y, > + boot_editor->widgets.args_sig_file_l, > + boot_editor->widgets.args_sig_file_f); > + } > + > > y++; > widget_move(widget_button_base(boot_editor->widgets.ok_b), y, > @@ -445,6 +462,11 @@ static void boot_editor_find_device(struct boot_editor *boot_editor, > if (bd->dtb && !path_on_device(bd_info, bd->dtb)) > return; > > + if (boot_editor->use_signature_files) > + if (bd->args_sig_file && !path_on_device(bd_info, > + bd->args_sig_file)) > + return; > + > /* ok, we match; preselect the device option, and remove the common > * prefix */ > boot_editor->selected_device = bd_info->name; > @@ -454,6 +476,9 @@ static void boot_editor_find_device(struct boot_editor *boot_editor, > boot_editor->initrd += len; > if (boot_editor->dtb) > boot_editor->dtb += len; > + if (boot_editor->use_signature_files) > + if (boot_editor->args_sig_file) > + boot_editor->args_sig_file += len; > } > > static void boot_editor_setup_widgets(struct boot_editor *boot_editor, > @@ -501,6 +526,13 @@ static void boot_editor_setup_widgets(struct boot_editor *boot_editor, > boot_editor->widgets.args_f = widget_new_textbox(set, 0, 0, > field_size, boot_editor->args); > > + if (boot_editor->use_signature_files) { > + boot_editor->widgets.args_sig_file_l = widget_new_label(set, > + 0, 0, _("Argument signature file:")); > + boot_editor->widgets.args_sig_file_f = widget_new_textbox(set, > + 0, 0, field_size, boot_editor->args_sig_file); > + } > + > boot_editor->widgets.ok_b = widget_new_button(set, 0, 0, 10, > _("OK"), ok_click, boot_editor); > boot_editor->widgets.help_b = widget_new_button(set, 0, 0, 10, > @@ -553,6 +585,15 @@ struct boot_editor *boot_editor_init(struct cui *cui, > if (!boot_editor) > return NULL; > > +#if defined(HAVE_LIBGPGME) > + if (access(LOCKDOWN_FILE, F_OK) == -1) > + boot_editor->use_signature_files = false; > + else > + boot_editor->use_signature_files = true; > +#else > + boot_editor->use_signature_files = false; > +#endif > + > talloc_set_destructor(boot_editor, boot_editor_destructor); > boot_editor->cui = cui; > boot_editor->item = item; > @@ -563,9 +604,10 @@ struct boot_editor *boot_editor_init(struct cui *cui, > > int ncols1 = strncols(_("Device tree:")); > int ncols2 = strncols(_("Boot arguments:")); > + int ncols3 = strncols(_("Argument signature file:")); > > boot_editor->label_x = 1; > - boot_editor->field_x = 2 + max(ncols1, ncols2); > + boot_editor->field_x = 2 + max(max(ncols1, ncols2), ncols3); > > nc_scr_init(&boot_editor->scr, pb_boot_editor_sig, 0, > cui, boot_editor_process_key, > @@ -584,10 +626,15 @@ struct boot_editor *boot_editor_init(struct cui *cui, > boot_editor->initrd = bd->initrd; > boot_editor->dtb = bd->dtb; > boot_editor->args = bd->args; > + if (boot_editor->use_signature_files) > + boot_editor->args_sig_file = bd->args_sig_file; > + else > + boot_editor->args_sig_file = ""; > boot_editor_find_device(boot_editor, bd, sysinfo); > } else { > boot_editor->image = boot_editor->initrd = > - boot_editor->dtb = boot_editor->args = ""; > + boot_editor->dtb = boot_editor->args = > + boot_editor->args_sig_file = ""; > } > > boot_editor->pad = newpad( > diff --git a/ui/ncurses/nc-cui.c b/ui/ncurses/nc-cui.c > index 0c355cc..09b63b0 100644 > --- a/ui/ncurses/nc-cui.c > +++ b/ui/ncurses/nc-cui.c > @@ -543,6 +543,7 @@ static int cui_boot_option_add(struct device *dev, struct boot_option *opt, > cod->bd->initrd = talloc_strdup(cod->bd, opt->initrd_file); > cod->bd->dtb = talloc_strdup(cod->bd, opt->dtb_file); > cod->bd->args = talloc_strdup(cod->bd, opt->boot_args); > + cod->bd->args_sig_file = talloc_strdup(cod->bd, opt->args_sig_file); > > /* This disconnects items array from menu. */ > result = set_menu_items(cui->main->ncm, NULL); > @@ -566,6 +567,7 @@ static int cui_boot_option_add(struct device *dev, struct boot_option *opt, > pb_log(" image '%s'\n", cod->bd->image); > pb_log(" initrd '%s'\n", cod->bd->initrd); > pb_log(" args '%s'\n", cod->bd->args); > + pb_log(" argsig '%s'\n", cod->bd->args_sig_file); > > /* Re-attach the items array. */ > result = set_menu_items(cui->main->ncm, cui->main->items);
-----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-----
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@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/petitboot
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 08/18/2016 08:32 PM, Samuel Mendoza-Jonas wrote: > 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. It's possible, though in general I'd prefer the parsing to remain consistent from line to line. If this is the only nit to be picked can this series be merged? :-) Thanks! - -- 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/ iQEcBAEBAgAGBQJXt25BAAoJEK+E3vEXDOFbakcH/R0msehoYQUmd+0avHU6t9O/ ojimES1+ejwB4hK1RrCT63mVfuszWmwUxhuh7CFfTfcTgN/DNlIJ5lsWPMzofbbc ikTGL2ywC8rmGOMdsySp9IvarkmKYrKlX4wQnlUEqmn4MtUbLUMsbMKKD33Xnw1Q GjL6ZOVPF9Xl7VVG1kKbBi+9TMXM8stu9xVzoLmP/Q3q1iF7ESGHhVMABwVh9WuF qJJyTR++J8UAOf2ncvR4l/CbADBLTwcmksA58TMyC3XsH8wjyyyQFYywbmtfy2GV +78PYpNRaHNo1JKjC2PVVKOXaKQsB+2SpCL2fTsVbU55IB+uwRajpYgAJHPTebI= =b5gq -----END PGP SIGNATURE-----
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 08/19/2016 03:38 PM, Timothy Pearson wrote: > On 08/18/2016 08:32 PM, Samuel Mendoza-Jonas wrote: >> 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. > > It's possible, though in general I'd prefer the parsing to remain > consistent from line to line. > > If this is the only nit to be picked can this series be merged? :-) > > Thanks! > I just wanted to follow up on this patch series. Thanks! - -- 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/ iQEcBAEBAgAGBQJXvMd3AAoJEK+E3vEXDOFbFDQH/RpRHeUuW1nw3JDuOfijuUBO dO+PS6FcXWKPBUZDEIjVDhNcDW2EqP5eMilFX9rVyOSFg4L9+dPcQmcuqxry2017 34bBVyHFN46fssGkqekyCKMOFJeGZH6WkrmzFZMrQ9IOF72JrJE/KUdAtoTtJZst Aga5kPQzRuuvbDRQBhtr5dtXDmwYT6EwckhSDlK2L7zCrwLvoZ+rCN31ekpsT3bP lYIEAVOngjkZxGE/d9ar35G5fyyWfyjb8cpH9rxxRdwGnHdmjQ+Qn761tcgIPNja s+R8gab3uC6vaX2kd6Y71M3vXG26r9X4YTYOTT6esYTpZ56yYaCD8qqTtVgdboo= =TH2L -----END PGP SIGNATURE-----
On Tue, 2016-08-23 at 17:00 -0500, Timothy Pearson wrote: > On 08/19/2016 03:38 PM, Timothy Pearson wrote: > > > > On 08/18/2016 08:32 PM, Samuel Mendoza-Jonas wrote: > > > > > > 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. > > > > It's possible, though in general I'd prefer the parsing to remain > > consistent from line to line. > > > > If this is the only nit to be picked can this series be merged? :-) > > > > Thanks! > > > > I just wanted to follow up on this patch series. > > Thanks! Hi! Excuse me not following up, got caught up putting out some other fires :) I'm pretty happy to merge this I think. I've tested with buildroot with only a moderate amount of hair-pulling, and everything acts as expected. One, maybe two nitpicks, but I can handle those myself when I merge, being: - I'll change --with-signed-boot to default=no - I might add a comment to a file or commit message to stress that it's probably best to hold off using the word 'secure' too much unless this is used in conjunction with a proper trusted-boot implementation so that you can trust the integrity of the initramfs. If you like I can send that to you to bikeshed as well :) Cheers, Sam
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 08/23/2016 07:40 PM, Samuel Mendoza-Jonas wrote: > Hi! Excuse me not following up, got caught up putting out some other > fires :) No problem, it happens. > I'm pretty happy to merge this I think. I've tested with buildroot with > only a moderate amount of hair-pulling, and everything acts as expected. > One, maybe two nitpicks, but I can handle those myself when I merge, > being: > - I'll change --with-signed-boot to default=no OK. > - I might add a comment to a file or commit message to stress that it's > probably best to hold off using the word 'secure' too much unless this > is used in conjunction with a proper trusted-boot implementation so that > you can trust the integrity of the initramfs. If you like I can send > that to you to bikeshed as well :) Yeah, this is probably a true statement. Essentially the entire contents of NOR flash, including petitboot, become the CRTM. While this is definitely more secure than the existing boot methods (and in line with some commercial x86 offerings) I wouldn't call it complete until the TPM is being used and the CRTM is as small as possible, preferably in the first 4K of Flash (write protected) or, even better, in the on-die nonvolatile memory for OpenPOWER systems. Not sure if that's even possible at this time, but it would definitely be interesting. Thanks! - -- 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/ iQEcBAEBAgAGBQJXvcMwAAoJEK+E3vEXDOFbnKAH/0b8Dw/nl27Y1eYZLBrGSlVw txIxOILDrFXyMqyr+xOm/mWH2VpGqeg6uk0sUMFZaKMW7iKTzh9fT1J4Sbq7gZiC ZuinV5wWgoWkhcUx9Jgp7VonJx4c9bBK3tmRKs97KyO2nXH7Wx06v8TGZ+Cv4wfq TE6M3/xBm/64nFsSWloloM5iJwFtLFnJQs3/FtfAi6vHfY7tlLX4C0rJBUY1MvFf cIJyOl0a6E1Po1rIFeWBRU6Spm3LvFy9RkbATaxRRtXGVSM2HDjUW5rUmnx7e7N+ sdblUU6K4QVQXq/HZkInOLAULbxhUsyuXWcJQV5GJUrQyYY7oUeB3gFhdGve5yw= =Z7Xz -----END PGP SIGNATURE-----
On Wed, 2016-08-24 at 10:54 -0500, Timothy Pearson wrote: > On 08/23/2016 07:40 PM, Samuel Mendoza-Jonas wrote: > > > > Hi! Excuse me not following up, got caught up putting out some other > > fires :) > > No problem, it happens. > > > > > I'm pretty happy to merge this I think. I've tested with buildroot with > > only a moderate amount of hair-pulling, and everything acts as expected. > > One, maybe two nitpicks, but I can handle those myself when I merge, > > being: > > - I'll change --with-signed-boot to default=no > > OK. > > > > > - I might add a comment to a file or commit message to stress that it's > > probably best to hold off using the word 'secure' too much unless this > > is used in conjunction with a proper trusted-boot implementation so that > > you can trust the integrity of the initramfs. If you like I can send > > that to you to bikeshed as well :) > > Yeah, this is probably a true statement. Essentially the entire > contents of NOR flash, including petitboot, become the CRTM. While this > is definitely more secure than the existing boot methods (and in line > with some commercial x86 offerings) I wouldn't call it complete until > the TPM is being used and the CRTM is as small as possible, preferably > in the first 4K of Flash (write protected) or, even better, in the > on-die nonvolatile memory for OpenPOWER systems. Not sure if that's > even possible at this time, but it would definitely be interesting. > > Thanks! > With some build-related fixes and the comment mentioned above, series merged as ccb478ac. Thanks!
diff --git a/configure.ac b/configure.ac index 00a6113..370511b 100644 --- a/configure.ac +++ b/configure.ac @@ -170,6 +170,70 @@ AS_IF( ] ) +AC_ARG_WITH( + [signed-boot], + [AS_HELP_STRING([--with-signed-boot], + [build kernel signature checking support [default=yes]] + )], + [], + [with_signed_boot=yes] +) +AM_CONDITIONAL([WITH_SIGNED_BOOT], [test "x$with_signed_boot" = "xyes"]) + +AM_CONDITIONAL( + [WITH_SIGNED_BOOT], + [test "x$with_signed_boot" = "xyes"]) + +AS_IF( + [test "x$with_signed_boot" = "xyes"], + [PKG_CHECK_MODULES( + [GPGME], + [gpgme >= 1.0.0], + [SAVE_LIBS="$LIBS" LIBS="$LIBS $gpgme_LIBS" + AC_CHECK_LIB( + [gpgme], + [gpgme_op_verify], + [], + [AC_MSG_FAILURE([--with-signed-boot was given but the test for gpgme failed.])] + ) + LIBS="$SAVE_LIBS" + ], + [AM_PATH_GPGME([1.0.0], [SAVE_LIBS="$LIBS" LIBS="$LIBS $gpgme_LIBS" + AC_CHECK_LIB( + [gpgme], + [gpgme_op_verify], + [], + [AC_MSG_FAILURE([--with-signed-boot was given but the test for gpgme failed.])] + ) + LIBS="$SAVE_LIBS"], + [AC_MSG_RESULT([$gpgme_PKG_ERRORS]) + AC_MSG_FAILURE([ Consider adjusting PKG_CONFIG_PATH environment variable]) + ]) + ] + )] +) + +AS_IF( + [test "x$with_signed_boot" = "xyes"], + [SAVE_CPPFLAGS="$CPPFLAGS" CPPFLAGS="$CPPFLAGS $gpgme_CFLAGS" + AC_CHECK_HEADERS( + [gpgme.h], + [], + [AC_MSG_FAILURE([ --with-signed-boot given but gpgme.h not found])] + ) + CPPFLAGS="$SAVE_CPPFLAGS" + ] +) + +AM_CONDITIONAL([WITH_GPGME], [test "x$with_signed_boot" = "xyes"]) + +AC_ARG_VAR( + [lockdown_file], + [Location of authorized signature file [default = "/etc/pb-lockdown"]] +) +AS_IF([test "x$lockdown_file" = x], [lockdown_file="/etc/pb-lockdown"]) +AC_DEFINE_UNQUOTED(LOCKDOWN_FILE, "$lockdown_file", [Lockdown file location]) + AC_ARG_ENABLE( [busybox], [AS_HELP_STRING( diff --git a/discover/Makefile.am b/discover/Makefile.am index 899c9a6..4a74da3 100644 --- a/discover/Makefile.am +++ b/discover/Makefile.am @@ -15,6 +15,12 @@ sbin_PROGRAMS += discover/pb-discover noinst_PROGRAMS += discover/platform.ro +if WITH_GPGME +gpg_int_SOURCES = discover/gpg.c +else +gpg_int_SOURCES = +endif + discover_pb_discover_SOURCES = \ discover/boot.c \ discover/boot.h \ @@ -52,13 +58,15 @@ discover_pb_discover_SOURCES = \ discover/user-event.h \ discover/kboot-parser.c \ discover/yaboot-parser.c \ - discover/pxe-parser.c + discover/pxe-parser.c \ + $(gpg_int_SOURCES) discover_pb_discover_LDADD = \ discover/grub2/grub2-parser.ro \ discover/platform.ro \ $(core_lib) \ - $(UDEV_LIBS) + $(UDEV_LIBS) \ + $(GPGME_LIBS) discover_pb_discover_LDFLAGS = \ $(AM_LDFLAGS) \ diff --git a/discover/boot.c b/discover/boot.c index ba6ce25..38be536 100644 --- a/discover/boot.c +++ b/discover/boot.c @@ -25,6 +25,7 @@ #include "paths.h" #include "resource.h" #include "platform.h" +#include "gpg.h" static const char *boot_hook_dir = PKG_SYSCONF_DIR "/boot.d"; enum { @@ -32,21 +33,6 @@ enum { BOOT_HOOK_EXIT_UPDATE = 2, }; -struct boot_task { - struct load_url_result *image; - struct load_url_result *initrd; - struct load_url_result *dtb; - const char *local_image; - const char *local_initrd; - const char *local_dtb; - const char *args; - const char *boot_tty; - boot_status_fn status_fn; - void *status_arg; - bool dry_run; - bool cancelled; -}; - /** * kexec_load - kexec load helper. */ @@ -59,20 +45,33 @@ static int kexec_load(struct boot_task *boot_task) char *s_dtb = NULL; char *s_args = NULL; + const char* local_initrd = boot_task->local_initrd; + const char* local_dtb = boot_task->local_dtb; + const char* local_image = boot_task->local_image; + + if ((result = gpg_validate_boot_files(boot_task, &local_initrd, + &local_dtb, &local_image))) { + if (result == KEXEC_LOAD_SIGNATURE_FAILURE) { + pb_log("%s: Aborting kexec due to" + " signature verification failure\n", __func__); + goto abort_kexec; + } + } + p = argv; *p++ = pb_system_apps.kexec; /* 1 */ *p++ = "-l"; /* 2 */ - if (boot_task->local_initrd) { + if (local_initrd) { s_initrd = talloc_asprintf(boot_task, "--initrd=%s", - boot_task->local_initrd); + local_initrd); assert(s_initrd); *p++ = s_initrd; /* 3 */ } - if (boot_task->local_dtb) { + if (local_dtb) { s_dtb = talloc_asprintf(boot_task, "--dtb=%s", - boot_task->local_dtb); + local_dtb); assert(s_dtb); *p++ = s_dtb; /* 4 */ } @@ -84,7 +83,7 @@ static int kexec_load(struct boot_task *boot_task) *p++ = s_args; /* 5 */ } - *p++ = boot_task->local_image; /* 6 */ + *p++ = local_image; /* 6 */ *p++ = NULL; /* 7 */ result = process_run_simple_argv(boot_task, argv); @@ -92,6 +91,10 @@ static int kexec_load(struct boot_task *boot_task) if (result) pb_log("%s: failed: (%d)\n", __func__, result); +abort_kexec: + gpg_validate_boot_files_cleanup(boot_task, &local_initrd, &local_dtb, + &local_image); + return result; } @@ -378,23 +381,69 @@ static void boot_process(struct load_url_result *result, void *data) check_load(task, "dtb", task->dtb)) goto no_load; + if (task->verify_signature) { + if (load_pending(task->image_signature) || + load_pending(task->initrd_signature) || + load_pending(task->dtb_signature) || + load_pending(task->cmdline_signature)) + return; + + if (check_load(task, "kernel image signature", + task->image_signature) || + check_load(task, "initrd signature", + task->initrd_signature) || + check_load(task, "dtb signature", + task->dtb_signature) || + check_load(task, "command line signature", + task->cmdline_signature)) + goto no_sig_load; + } + /* we make a copy of the local paths, as the boot hooks might update * and/or create these */ task->local_image = task->image ? task->image->local : NULL; task->local_initrd = task->initrd ? task->initrd->local : NULL; task->local_dtb = task->dtb ? task->dtb->local : NULL; + if (task->verify_signature) { + task->local_image_signature = task->image_signature ? + task->image_signature->local : NULL; + task->local_initrd_signature = task->initrd_signature ? + task->initrd_signature->local : NULL; + task->local_dtb_signature = task->dtb_signature ? + task->dtb_signature->local : NULL; + task->local_cmdline_signature = task->cmdline_signature ? + task->cmdline_signature->local : NULL; + } + run_boot_hooks(task); update_status(task->status_fn, task->status_arg, BOOT_STATUS_INFO, _("performing kexec_load")); rc = kexec_load(task); - if (rc) { + if (rc == KEXEC_LOAD_SIGNATURE_FAILURE) { + update_status(task->status_fn, task->status_arg, + BOOT_STATUS_ERROR, + _("signature verification failed")); + } + else if (rc == KEXEC_LOAD_SIG_SETUP_INVALID) { update_status(task->status_fn, task->status_arg, - BOOT_STATUS_ERROR, _("kexec load failed")); + BOOT_STATUS_ERROR, + _("invalid signature configuration")); + } + else if (rc) { + update_status(task->status_fn, task->status_arg, + BOOT_STATUS_ERROR, + _("kexec load failed")); } +no_sig_load: + cleanup_load(task->image_signature); + cleanup_load(task->initrd_signature); + cleanup_load(task->dtb_signature); + cleanup_load(task->cmdline_signature); + no_load: cleanup_load(task->image); cleanup_load(task->initrd); @@ -435,6 +484,8 @@ struct boot_task *boot(void *ctx, struct discover_boot_option *opt, boot_status_fn status_fn, void *status_arg) { struct pb_url *image = NULL, *initrd = NULL, *dtb = NULL; + struct pb_url *image_sig = NULL, *initrd_sig = NULL, *dtb_sig = NULL, + *cmdline_sig = NULL; const struct config *config; struct boot_task *boot_task; const char *boot_desc; @@ -478,6 +529,8 @@ struct boot_task *boot(void *ctx, struct discover_boot_option *opt, boot_task->status_fn = status_fn; boot_task->status_arg = status_arg; + boot_task->verify_signature = (lockdown_status() == PB_LOCKDOWN_SIGN); + if (cmd && cmd->boot_args) { boot_task->args = talloc_strdup(boot_task, cmd->boot_args); } else if (opt && opt->option->boot_args) { @@ -494,11 +547,69 @@ struct boot_task *boot(void *ctx, struct discover_boot_option *opt, boot_task->boot_tty = config ? config->boot_tty : NULL; } + if (boot_task->verify_signature) { + if (cmd && cmd->args_sig_file) { + cmdline_sig = pb_url_parse(opt, cmd->args_sig_file); + } else if (opt && opt->args_sig_file) { + cmdline_sig = opt->args_sig_file->url; + } else { + pb_log("%s: no command line signature file" + " specified\n", __func__); + update_status(status_fn, status_arg, BOOT_STATUS_INFO, + _("Boot failed: no command line" + " signature file specified")); + return NULL; + } + } + /* start async loads for boot resources */ rc = start_url_load(boot_task, "kernel image", image, &boot_task->image) || start_url_load(boot_task, "initrd", initrd, &boot_task->initrd) || start_url_load(boot_task, "dtb", dtb, &boot_task->dtb); + if (boot_task->verify_signature) { + /* Generate names of associated signature files and load */ + if (image) { + image_sig = pb_url_copy(ctx, image); + talloc_free(image_sig->file); + image_sig->file = talloc_asprintf(image_sig, + "%s.sig", image->file); + talloc_free(image_sig->path); + image_sig->path = talloc_asprintf(image_sig, + "%s.sig", image->path); + rc |= start_url_load(boot_task, + "kernel image signature", image_sig, + &boot_task->image_signature); + } + if (initrd) { + initrd_sig = pb_url_copy(ctx, initrd); + talloc_free(initrd_sig->file); + initrd_sig->file = talloc_asprintf(initrd_sig, + "%s.sig", initrd->file); + talloc_free(initrd_sig->path); + initrd_sig->path = talloc_asprintf(initrd_sig, + "%s.sig", initrd->path); + rc |= start_url_load(boot_task, "initrd signature", + initrd_sig, &boot_task->initrd_signature); + } + if (dtb) { + dtb_sig = pb_url_copy(ctx, dtb); + talloc_free(dtb_sig->file); + dtb_sig->file = talloc_asprintf(dtb_sig, + "%s.sig", dtb->file); + talloc_free(dtb_sig->path); + dtb_sig->path = talloc_asprintf(dtb_sig, + "%s.sig", dtb->path); + rc |= start_url_load(boot_task, "dtb signature", + dtb_sig, &boot_task->dtb_signature); + } + + rc |= start_url_load(boot_task, + "kernel command line signature", cmdline_sig, + &boot_task->cmdline_signature); + } + + /* If all URLs are local, we may be done. */ if (rc) { /* Don't call boot_cancel() to preserve the status update */ boot_task->cancelled = true; diff --git a/discover/boot.h b/discover/boot.h index ec61703..5f6e874 100644 --- a/discover/boot.h +++ b/discover/boot.h @@ -11,4 +11,34 @@ struct boot_task *boot(void *ctx, struct discover_boot_option *opt, boot_status_fn status_fn, void *status_arg); void boot_cancel(struct boot_task *task); + +struct boot_task { + struct load_url_result *image; + struct load_url_result *initrd; + struct load_url_result *dtb; + const char *local_image; + const char *local_initrd; + const char *local_dtb; + const char *args; + const char *boot_tty; + boot_status_fn status_fn; + void *status_arg; + bool dry_run; + bool cancelled; + bool verify_signature; + struct load_url_result *image_signature; + struct load_url_result *initrd_signature; + struct load_url_result *dtb_signature; + struct load_url_result *cmdline_signature; + const char *local_image_signature; + const char *local_initrd_signature; + const char *local_dtb_signature; + const char *local_cmdline_signature; +}; + +enum { + KEXEC_LOAD_SIG_SETUP_INVALID = 253, + KEXEC_LOAD_SIGNATURE_FAILURE = 254, +}; + #endif /* _BOOT_H */ diff --git a/discover/device-handler.c b/discover/device-handler.c index c31fcea..f6b6d22 100644 --- a/discover/device-handler.c +++ b/discover/device-handler.c @@ -613,6 +613,7 @@ static bool __attribute__((used)) boot_option_is_resolved( return resource_is_resolved(opt->boot_image) && resource_is_resolved(opt->initrd) && resource_is_resolved(opt->dtb) && + resource_is_resolved(opt->args_sig_file) && resource_is_resolved(opt->icon); } @@ -638,6 +639,8 @@ static bool boot_option_resolve(struct discover_boot_option *opt, return resource_resolve(opt->boot_image, "boot_image", opt, handler) && resource_resolve(opt->initrd, "initrd", opt, handler) && resource_resolve(opt->dtb, "dtb", opt, handler) && + resource_resolve(opt->args_sig_file, "args_sig_file", opt, + handler) && resource_resolve(opt->icon, "icon", opt, handler); } @@ -652,6 +655,7 @@ static void boot_option_finalise(struct device_handler *handler, assert(!opt->option->dtb_file); assert(!opt->option->icon_file); assert(!opt->option->device_id); + assert(!opt->option->args_sig_file); if (opt->boot_image) opt->option->boot_image_file = opt->boot_image->url->full; @@ -661,6 +665,8 @@ static void boot_option_finalise(struct device_handler *handler, opt->option->dtb_file = opt->dtb->url->full; if (opt->icon) opt->option->icon_file = opt->icon->url->full; + if (opt->args_sig_file) + opt->option->args_sig_file = opt->args_sig_file->url->full; opt->option->device_id = opt->device->device->id; diff --git a/discover/device-handler.h b/discover/device-handler.h index b6f9fd5..f785ccc 100644 --- a/discover/device-handler.h +++ b/discover/device-handler.h @@ -48,6 +48,7 @@ struct discover_boot_option { struct resource *boot_image; struct resource *initrd; struct resource *dtb; + struct resource *args_sig_file; struct resource *icon; }; diff --git a/discover/gpg.c b/discover/gpg.c new file mode 100644 index 0000000..e1c1d04 --- /dev/null +++ b/discover/gpg.c @@ -0,0 +1,381 @@ +/* + * Copyright (C) 2016 Raptor Engineering, LLC + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + */ + +#if defined(HAVE_CONFIG_H) +#include "config.h" +#endif + +#include <stdbool.h> +#include <stdlib.h> +#include <assert.h> +#include <dirent.h> +#include <string.h> +#include <fcntl.h> +#include <sys/types.h> + +#include <log/log.h> +#include <talloc/talloc.h> +#include <url/url.h> +#include <util/util.h> +#include <i18n/i18n.h> + +#include "gpg.h" + +int copy_file_to_destination(const char * source_file, + char * destination_file, int max_dest_filename_size) { + int result = 0; + char template[21] = "/tmp/petitbootXXXXXX"; + FILE *source_handle = fopen(source_file, "r"); + int destination_fd = mkstemp(template); + FILE *destination_handle = fdopen(destination_fd, "w"); + if (!source_handle || !(destination_handle)) { + // handle open error + pb_log("%s: failed: unable to open source file '%s'\n", + __func__, source_file); + result = 1; + } + + size_t l1; + unsigned char buffer[FILE_XFER_BUFFER_SIZE]; + + /* Copy data */ + while ((l1 = fread(buffer, 1, sizeof buffer, source_handle)) > 0) { + size_t l2 = fwrite(buffer, 1, l1, destination_handle); + if (l2 < l1) { + if (ferror(destination_handle)) { + /* General error */ + result = 1; + pb_log("%s: failed: unknown fault\n", __func__); + } + else { + /* No space on destination device */ + result = 2; + pb_log("%s: failed: temporary storage full\n", + __func__); + } + } + } + + if (result) { + destination_file[0] = '\0'; + } + else { + ssize_t r; + char readlink_buffer[MAX_FILENAME_SIZE]; + snprintf(readlink_buffer, MAX_FILENAME_SIZE, "/proc/self/fd/%d", + destination_fd); + r = readlink(readlink_buffer, destination_file, + max_dest_filename_size); + if (r < 0) { + /* readlink failed */ + result = 3; + pb_log("%s: failed: unable to obtain temporary filename" + "\n", __func__); + } + destination_file[r] = '\0'; + } + + fclose(source_handle); + fclose(destination_handle); + + return result; +} + +int verify_file_signature(const char * plaintext_filename, + const char * signature_filename, FILE * authorized_signatures_handle, + const char * keyring_path) +{ + int valid = 0; + + if (signature_filename == NULL) + return -1; + + gpgme_signature_t verification_signatures; + gpgme_verify_result_t verification_result; + gpgme_data_t plaintext_data; + gpgme_data_t signature_data; + gpgme_engine_info_t enginfo; + gpgme_ctx_t gpg_context; + gpgme_error_t err; + + /* Initialize gpgme */ + setlocale (LC_ALL, ""); + gpgme_check_version(NULL); + gpgme_set_locale(NULL, LC_CTYPE, setlocale (LC_CTYPE, NULL)); + err = gpgme_engine_check_version(GPGME_PROTOCOL_OpenPGP); + if (err != GPG_ERR_NO_ERROR) { + pb_log("%s: OpenPGP support not available\n", __func__); + return -1; + } + err = gpgme_get_engine_info(&enginfo); + if (err != GPG_ERR_NO_ERROR) { + pb_log("%s: GPG engine failed to initialize\n", __func__); + return -1; + } + err = gpgme_new(&gpg_context); + if (err != GPG_ERR_NO_ERROR) { + pb_log("%s: GPG context could not be created\n", __func__); + return -1; + } + err = gpgme_set_protocol(gpg_context, GPGME_PROTOCOL_OpenPGP); + if (err != GPG_ERR_NO_ERROR) { + pb_log("%s: GPG protocol could not be set\n", __func__); + return -1; + } + if (keyring_path) + err = gpgme_ctx_set_engine_info (gpg_context, + GPGME_PROTOCOL_OpenPGP, enginfo->file_name, + keyring_path); + else + err = gpgme_ctx_set_engine_info (gpg_context, + GPGME_PROTOCOL_OpenPGP, enginfo->file_name, + enginfo->home_dir); + if (err != GPG_ERR_NO_ERROR) { + pb_log("%s: Could not set GPG engine information\n", __func__); + return -1; + } + err = gpgme_data_new_from_file(&plaintext_data, plaintext_filename, 1); + if (err != GPG_ERR_NO_ERROR) { + pb_log("%s: Could not create GPG plaintext data buffer" + " from file '%s'\n", __func__, plaintext_filename); + return -1; + } + err = gpgme_data_new_from_file(&signature_data, signature_filename, 1); + if (err != GPG_ERR_NO_ERROR) { + pb_log("%s: Could not create GPG signature data buffer" + " from file '%s'\n", __func__, signature_filename); + return -1; + } + + /* Check signature */ + err = gpgme_op_verify(gpg_context, signature_data, plaintext_data, + NULL); + if (err != GPG_ERR_NO_ERROR) { + pb_log("%s: Could not verify file using GPG signature '%s'\n", + __func__, signature_filename); + return -1; + } + verification_result = gpgme_op_verify_result(gpg_context); + verification_signatures = verification_result->signatures; + while (verification_signatures) { + if (verification_signatures->status == GPG_ERR_NO_ERROR) { + pb_log("%s: Good signature for key ID '%s' ('%s')\n", + __func__, verification_signatures->fpr, + signature_filename); + /* Verify fingerprint is present in + * authorized signatures file + */ + char *auth_sig_line = NULL; + size_t auth_sig_len = 0; + ssize_t auth_sig_read; + rewind(authorized_signatures_handle); + while ((auth_sig_read = getline(&auth_sig_line, + &auth_sig_len, + authorized_signatures_handle)) != -1) { + auth_sig_len = strlen(auth_sig_line); + while ((auth_sig_line[auth_sig_len-1] == '\n') + || (auth_sig_line[auth_sig_len-1] == '\r')) + auth_sig_len--; + auth_sig_line[auth_sig_len] = '\0'; + if (strcmp(auth_sig_line, + verification_signatures->fpr) == 0) + valid = 1; + } + free(auth_sig_line); + } + else { + pb_log("%s: Signature for key ID '%s' ('%s') invalid." + " Status: %08x\n", __func__, + verification_signatures->fpr, + signature_filename, + verification_signatures->status); + } + verification_signatures = verification_signatures->next; + } + + /* Clean up */ + gpgme_data_release(plaintext_data); + gpgme_data_release(signature_data); + gpgme_release(gpg_context); + + if (!valid) { + pb_log("%s: Incorrect GPG signature\n", __func__); + return -1; + } + else { + pb_log("%s: GPG signature '%s' for file '%s' verified\n", + __func__, signature_filename, plaintext_filename); + } + + return 0; +} + +int gpg_validate_boot_files(struct boot_task *boot_task, + const char** local_initrd, const char** local_dtb, + const char** local_image) { + int result = 0; + + const char* local_initrd_signature = (boot_task->verify_signature) ? + boot_task->local_initrd_signature : NULL; + const char* local_dtb_signature = (boot_task->verify_signature) ? + boot_task->local_dtb_signature : NULL; + const char* local_image_signature = (boot_task->verify_signature) ? + boot_task->local_image_signature : NULL; + const char* local_cmdline_signature = (boot_task->verify_signature) ? + boot_task->local_cmdline_signature : NULL; + + if (boot_task->verify_signature) { + char kernel_filename[MAX_FILENAME_SIZE]; + char initrd_filename[MAX_FILENAME_SIZE]; + char dtb_filename[MAX_FILENAME_SIZE]; + + kernel_filename[0] = '\0'; + initrd_filename[0] = '\0'; + dtb_filename[0] = '\0'; + + FILE *authorized_signatures_handle = NULL; + + /* Load authorized signatures file */ + authorized_signatures_handle = fopen(LOCKDOWN_FILE, "r"); + if (!authorized_signatures_handle) { + pb_log("%s: unable to read lockdown file\n", __func__); + result = KEXEC_LOAD_SIG_SETUP_INVALID; + return result; + } + + /* Copy files to temporary directory for verification / boot */ + result = copy_file_to_destination(boot_task->local_image, + kernel_filename, MAX_FILENAME_SIZE); + if (result) { + pb_log("%s: image copy failed: (%d)\n", + __func__, result); + return result; + } + if (boot_task->local_initrd) { + result = copy_file_to_destination( + boot_task->local_initrd, + initrd_filename, MAX_FILENAME_SIZE); + if (result) { + pb_log("%s: initrd copy failed: (%d)\n", + __func__, result); + unlink(*local_image); + return result; + } + } + if (boot_task->local_dtb) { + result = copy_file_to_destination(boot_task->local_dtb, + dtb_filename, MAX_FILENAME_SIZE); + if (result) { + pb_log("%s: dtb copy failed: (%d)\n", + __func__, result); + unlink(*local_image); + if (*local_initrd) + unlink(*local_initrd); + return result; + } + } + *local_image = talloc_strdup(boot_task, + kernel_filename); + if (boot_task->local_initrd) + *local_initrd = talloc_strdup(boot_task, + initrd_filename); + if (boot_task->local_dtb) + *local_dtb = talloc_strdup(boot_task, + dtb_filename); + + /* Write command line to temporary file for verification */ + char cmdline_template[21] = "/tmp/petitbootXXXXXX"; + int cmdline_fd = mkstemp(cmdline_template); + FILE *cmdline_handle = NULL; + if (cmdline_fd < 0) { + // handle mkstemp error + pb_log("%s: failed: unable to create command line" + " temporary file for verification\n", + __func__); + result = -1; + } + else { + cmdline_handle = fdopen(cmdline_fd, "w"); + } + if (!cmdline_handle) { + // handle open error + pb_log("%s: failed: unable to write command line" + " temporary file for verification\n", + __func__); + result = -1; + } + else { + fwrite(boot_task->args, sizeof(char), + strlen(boot_task->args), cmdline_handle); + fflush(cmdline_handle); + } + + /* Check signatures */ + if (verify_file_signature(kernel_filename, + local_image_signature, + authorized_signatures_handle, "/etc/gpg")) + result = KEXEC_LOAD_SIGNATURE_FAILURE; + if (verify_file_signature(cmdline_template, + local_cmdline_signature, + authorized_signatures_handle, "/etc/gpg")) + result = KEXEC_LOAD_SIGNATURE_FAILURE; + if (boot_task->local_initrd_signature) + if (verify_file_signature(initrd_filename, + local_initrd_signature, + authorized_signatures_handle, "/etc/gpg")) + result = KEXEC_LOAD_SIGNATURE_FAILURE; + if (boot_task->local_dtb_signature) + if (verify_file_signature(dtb_filename, + local_dtb_signature, + authorized_signatures_handle, "/etc/gpg")) + result = KEXEC_LOAD_SIGNATURE_FAILURE; + + /* Clean up */ + if (cmdline_handle) { + fclose(cmdline_handle); + unlink(cmdline_template); + } + fclose(authorized_signatures_handle); + } + + return result; +} + +void gpg_validate_boot_files_cleanup(struct boot_task *boot_task, + const char** local_initrd, const char** local_dtb, + const char** local_image) { + if (boot_task->verify_signature) { + unlink(*local_image); + if (*local_initrd) + unlink(*local_initrd); + if (*local_dtb) + unlink(*local_dtb); + + talloc_free((char*)*local_image); + if (*local_initrd) + talloc_free((char*)*local_initrd); + if (*local_dtb) + talloc_free((char*)*local_dtb); + } +} + +int lockdown_status() { + if (access(LOCKDOWN_FILE, F_OK) == -1) + return PB_LOCKDOWN_NONE; + else + return PB_LOCKDOWN_SIGN; +} \ No newline at end of file diff --git a/discover/gpg.h b/discover/gpg.h new file mode 100644 index 0000000..9b452b8 --- /dev/null +++ b/discover/gpg.h @@ -0,0 +1,73 @@ +/* + * Copyright (C) 2016 Raptor Engineering, LLC + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + */ + +#ifndef _PB_GPG_H +#define _PB_GPG_H + +#define MAX_FILENAME_SIZE 8192 +#define FILE_XFER_BUFFER_SIZE 8192 + +#include "boot.h" + +enum { + PB_LOCKDOWN_NONE = 0, + PB_LOCKDOWN_SIGN = 1, +}; + +#if defined(HAVE_LIBGPGME) +#include <gpgme.h> +#endif /* HAVE_LIBGPGME */ + +int lockdown_status(void); + +int copy_file_to_destination(const char * source_file, + char * destination_file, int max_dest_filename_size); + +int verify_file_signature(const char * plaintext_filename, + const char * signature_filename, FILE * authorized_signatures_handle, + const char * keyring_path); + +int gpg_validate_boot_files(struct boot_task *boot_task, + const char** local_initrd, const char** local_dtb, + const char** local_image); + +void gpg_validate_boot_files_cleanup(struct boot_task *boot_task, + const char** local_initrd, const char** local_dtb, + const char** local_image); + +#if !defined(HAVE_LIBGPGME) + +int lockdown_status(void) { return PB_LOCKDOWN_NONE; } + +int copy_file_to_destination(const char * source_file, + char * destination_file, int max_dest_filename_size) { return -1; } + +int verify_file_signature(const char * plaintext_filename, + const char * signature_filename, FILE * authorized_signatures_handle, + const char * keyring_path) { return -1; } + +int gpg_validate_boot_files(struct boot_task *boot_task, + const char** local_initrd, const char** local_dtb, + const char** local_image) { return 0; } + +void gpg_validate_boot_files_cleanup(struct boot_task *boot_task, + const char** local_initrd, const char** local_dtb, + const char** local_image) { } + +#endif /* HAVE_LIBGPGME */ + +#endif /* _PB_GPG_H */ \ No newline at end of file diff --git a/discover/grub2/builtins.c b/discover/grub2/builtins.c index 8bff732..c16b639 100644 --- a/discover/grub2/builtins.c +++ b/discover/grub2/builtins.c @@ -6,7 +6,9 @@ #include <types/types.h> #include <talloc/talloc.h> #include <util/util.h> +#include <url/url.h> +#include "discover/resource.h" #include "discover/parser.h" #include "grub2.h" @@ -69,6 +71,12 @@ static int builtin_linux(struct grub2_script *script, opt->option->boot_args = talloc_asprintf_append( opt->option->boot_args, " %s", argv[i]); + + char* args_sigfile_default = talloc_asprintf(opt, + "%s.cmdline.sig", argv[1]); + opt->args_sig_file = create_grub2_resource(opt, script->ctx->device, + root, args_sigfile_default); + talloc_free(args_sigfile_default); return 0; } diff --git a/discover/kboot-parser.c b/discover/kboot-parser.c index cebe787..f7f75e0 100644 --- a/discover/kboot-parser.c +++ b/discover/kboot-parser.c @@ -96,6 +96,12 @@ out_add: d_opt->boot_image = create_devpath_resource(d_opt, conf->dc->device, value); + char* args_sigfile_default = talloc_asprintf(d_opt, + "%s.cmdline.sig", value); + d_opt->args_sig_file = create_devpath_resource(d_opt, + conf->dc->device, args_sigfile_default); + talloc_free(args_sigfile_default); + if (root) { opt->boot_args = talloc_asprintf(opt, "root=%s %s", root, args); talloc_free(args); diff --git a/discover/pxe-parser.c b/discover/pxe-parser.c index 4481e5d..4aae8b1 100644 --- a/discover/pxe-parser.c +++ b/discover/pxe-parser.c @@ -166,6 +166,13 @@ static void pxe_process_pair(struct conf_context *ctx, url = pxe_url_join(ctx->dc, ctx->dc->conf_url, value); opt->boot_image = create_url_resource(opt, url); + char* args_sigfile_default = talloc_asprintf(opt, + "%s.cmdline.sig", value); + url = pxe_url_join(ctx->dc, ctx->dc->conf_url, + args_sigfile_default); + opt->args_sig_file = create_url_resource(opt, url); + talloc_free(args_sigfile_default); + } else if (streq(name, "INITRD")) { url = pxe_url_join(ctx->dc, ctx->dc->conf_url, value); opt->initrd = create_url_resource(opt, url); diff --git a/discover/user-event.c b/discover/user-event.c index 7350b6c..6ea754f 100644 --- a/discover/user-event.c +++ b/discover/user-event.c @@ -82,7 +82,7 @@ static void user_event_print_event(struct event __attribute__((unused)) *event) } static struct resource *user_event_resource(struct discover_boot_option *opt, - struct event *event) + struct event *event, bool gen_boot_args_sigfile) { const char *siaddr, *boot_file; struct resource *res; @@ -101,7 +101,16 @@ static struct resource *user_event_resource(struct discover_boot_option *opt, return NULL; } - url_str = talloc_asprintf(opt, "%s%s/%s", "tftp://", siaddr, boot_file); + if (gen_boot_args_sigfile) { + char* args_sigfile_default = talloc_asprintf(opt, + "%s.cmdline.sig", boot_file); + url_str = talloc_asprintf(opt, "%s%s/%s", "tftp://", siaddr, + args_sigfile_default); + talloc_free(args_sigfile_default); + } + else + url_str = talloc_asprintf(opt, "%s%s/%s", "tftp://", siaddr, + boot_file); url = pb_url_parse(opt, url_str); talloc_free(url_str); @@ -143,12 +152,13 @@ static int parse_user_event(struct discover_context *ctx, struct event *event) opt->id = talloc_asprintf(opt, "%s#%s", dev->id, val); opt->name = talloc_strdup(opt, val); - d_opt->boot_image = user_event_resource(d_opt, event); + d_opt->boot_image = user_event_resource(d_opt, event, false); if (!d_opt->boot_image) { pb_log("%s: no boot image found for %s!\n", __func__, opt->name); goto fail_opt; } + d_opt->args_sig_file = user_event_resource(d_opt, event, true); val = event_get_param(event, "rootpath"); if (val) { diff --git a/discover/yaboot-parser.c b/discover/yaboot-parser.c index aa99392..b62f39d 100644 --- a/discover/yaboot-parser.c +++ b/discover/yaboot-parser.c @@ -114,6 +114,13 @@ static void yaboot_finish(struct conf_context *conf) /* populate the boot option from state data */ state->opt->boot_image = create_yaboot_devpath_resource(state, conf, state->boot_image); + + char* args_sigfile_default = talloc_asprintf(opt, + "%s.cmdline.sig", state->boot_image); + state->opt->args_sig_file = create_yaboot_devpath_resource(state, + conf, args_sigfile_default); + talloc_free(args_sigfile_default); + if (state->initrd) { state->opt->initrd = create_yaboot_devpath_resource(state, conf, state->initrd); diff --git a/lib/pb-protocol/pb-protocol.c b/lib/pb-protocol/pb-protocol.c index 7887fb0..ad7798e 100644 --- a/lib/pb-protocol/pb-protocol.c +++ b/lib/pb-protocol/pb-protocol.c @@ -37,6 +37,7 @@ * 4-byte len, initrd_file * 4-byte len, dtb_file * 4-byte len, boot_args + * 4-byte len, args_sig_file * * action = 0x2: device remove message * payload: @@ -49,6 +50,7 @@ * 4-byte len, initrd_file * 4-byte len, dtb_file * 4-byte len, boot_args + * 4-byte len, args_sig_file * */ @@ -72,6 +74,7 @@ void pb_protocol_dump_device(const struct device *dev, const char *text, fprintf(stream, "%s\t\tinit: %s\n", text, opt->initrd_file); fprintf(stream, "%s\t\tdtb: %s\n", text, opt->dtb_file); fprintf(stream, "%s\t\targs: %s\n", text, opt->boot_args); + fprintf(stream, "%s\t\tasig: %s\n", text, opt->args_sig_file); } } @@ -197,6 +200,7 @@ int pb_protocol_boot_option_len(const struct boot_option *opt) 4 + optional_strlen(opt->initrd_file) + 4 + optional_strlen(opt->dtb_file) + 4 + optional_strlen(opt->boot_args) + + 4 + optional_strlen(opt->args_sig_file) + sizeof(opt->is_default); } @@ -207,6 +211,7 @@ int pb_protocol_boot_len(const struct boot_command *boot) 4 + optional_strlen(boot->initrd_file) + 4 + optional_strlen(boot->dtb_file) + 4 + optional_strlen(boot->boot_args) + + 4 + optional_strlen(boot->args_sig_file) + 4 + optional_strlen(boot->tty); } @@ -360,6 +365,7 @@ int pb_protocol_serialise_boot_option(const struct boot_option *opt, pos += pb_protocol_serialise_string(pos, opt->initrd_file); pos += pb_protocol_serialise_string(pos, opt->dtb_file); pos += pb_protocol_serialise_string(pos, opt->boot_args); + pos += pb_protocol_serialise_string(pos, opt->args_sig_file); *(bool *)pos = opt->is_default; pos += sizeof(bool); @@ -380,6 +386,7 @@ int pb_protocol_serialise_boot_command(const struct boot_command *boot, pos += pb_protocol_serialise_string(pos, boot->initrd_file); pos += pb_protocol_serialise_string(pos, boot->dtb_file); pos += pb_protocol_serialise_string(pos, boot->boot_args); + pos += pb_protocol_serialise_string(pos, boot->args_sig_file); pos += pb_protocol_serialise_string(pos, boot->tty); assert(pos <= buf + buf_len); @@ -750,6 +757,9 @@ int pb_protocol_deserialise_boot_option(struct boot_option *opt, if (read_string(opt, &pos, &len, &opt->boot_args)) goto out; + if (read_string(opt, &pos, &len, &opt->args_sig_file)) + goto out; + if (len < sizeof(bool)) goto out; opt->is_default = *(bool *)(pos); @@ -785,6 +795,9 @@ int pb_protocol_deserialise_boot_command(struct boot_command *cmd, if (read_string(cmd, &pos, &len, &cmd->boot_args)) goto out; + if (read_string(cmd, &pos, &len, &cmd->args_sig_file)) + goto out; + if (read_string(cmd, &pos, &len, &cmd->tty)) goto out; diff --git a/lib/types/types.h b/lib/types/types.h index 5c5f6ed..6b607cd 100644 --- a/lib/types/types.h +++ b/lib/types/types.h @@ -52,6 +52,7 @@ struct boot_option { char *initrd_file; char *dtb_file; char *boot_args; + char *args_sig_file; bool is_default; struct list_item list; @@ -65,6 +66,7 @@ struct boot_command { char *initrd_file; char *dtb_file; char *boot_args; + char *args_sig_file; char *tty; }; diff --git a/ui/common/discover-client.c b/ui/common/discover-client.c index 6247dd0..5dbd99b 100644 --- a/ui/common/discover-client.c +++ b/ui/common/discover-client.c @@ -312,6 +312,7 @@ static void create_boot_command(struct boot_command *command, command->initrd_file = data->initrd; command->dtb_file = data->dtb; command->boot_args = data->args; + command->args_sig_file = data->args_sig_file; command->tty = ttyname(STDIN_FILENO); } diff --git a/ui/common/discover-client.h b/ui/common/discover-client.h index 542a275..59d2df9 100644 --- a/ui/common/discover-client.h +++ b/ui/common/discover-client.h @@ -11,6 +11,7 @@ struct pb_boot_data { char *initrd; char *dtb; char *args; + char *args_sig_file; }; /** diff --git a/ui/ncurses/nc-boot-editor.c b/ui/ncurses/nc-boot-editor.c index 4012ec5..9788fef 100644 --- a/ui/ncurses/nc-boot-editor.c +++ b/ui/ncurses/nc-boot-editor.c @@ -63,6 +63,8 @@ struct boot_editor { struct nc_widget_textbox *dtb_f; struct nc_widget_label *args_l; struct nc_widget_textbox *args_f; + struct nc_widget_label *args_sig_file_l; + struct nc_widget_textbox *args_sig_file_f; struct nc_widget_button *ok_b; struct nc_widget_button *help_b; struct nc_widget_button *cancel_b; @@ -73,6 +75,9 @@ struct boot_editor { char *initrd; char *dtb; char *args; + char *args_sig_file; + + bool use_signature_files; }; extern const struct help_text boot_editor_help_text; @@ -198,6 +203,12 @@ static struct pb_boot_data *boot_editor_prepare_data( s = widget_textbox_get_value(boot_editor->widgets.args_f); bd->args = *s ? talloc_strdup(bd, s) : NULL; + if (boot_editor->use_signature_files) { + s = widget_textbox_get_value( + boot_editor->widgets.args_sig_file_f); + bd->args_sig_file = conditional_prefix(bd, prefix, s); + } + return bd; } @@ -323,6 +334,12 @@ static void boot_editor_layout_widgets(struct boot_editor *boot_editor) y += layout_pair(boot_editor, y, boot_editor->widgets.args_l, boot_editor->widgets.args_f); + if (boot_editor->use_signature_files) { + y += layout_pair(boot_editor, y, + boot_editor->widgets.args_sig_file_l, + boot_editor->widgets.args_sig_file_f); + } + y++; widget_move(widget_button_base(boot_editor->widgets.ok_b), y, @@ -445,6 +462,11 @@ static void boot_editor_find_device(struct boot_editor *boot_editor, if (bd->dtb && !path_on_device(bd_info, bd->dtb)) return; + if (boot_editor->use_signature_files) + if (bd->args_sig_file && !path_on_device(bd_info, + bd->args_sig_file)) + return; + /* ok, we match; preselect the device option, and remove the common * prefix */ boot_editor->selected_device = bd_info->name; @@ -454,6 +476,9 @@ static void boot_editor_find_device(struct boot_editor *boot_editor, boot_editor->initrd += len; if (boot_editor->dtb) boot_editor->dtb += len; + if (boot_editor->use_signature_files) + if (boot_editor->args_sig_file) + boot_editor->args_sig_file += len; } static void boot_editor_setup_widgets(struct boot_editor *boot_editor, @@ -501,6 +526,13 @@ static void boot_editor_setup_widgets(struct boot_editor *boot_editor, boot_editor->widgets.args_f = widget_new_textbox(set, 0, 0, field_size, boot_editor->args); + if (boot_editor->use_signature_files) { + boot_editor->widgets.args_sig_file_l = widget_new_label(set, + 0, 0, _("Argument signature file:")); + boot_editor->widgets.args_sig_file_f = widget_new_textbox(set, + 0, 0, field_size, boot_editor->args_sig_file); + } + boot_editor->widgets.ok_b = widget_new_button(set, 0, 0, 10, _("OK"), ok_click, boot_editor); boot_editor->widgets.help_b = widget_new_button(set, 0, 0, 10, @@ -553,6 +585,15 @@ struct boot_editor *boot_editor_init(struct cui *cui, if (!boot_editor) return NULL; +#if defined(HAVE_LIBGPGME) + if (access(LOCKDOWN_FILE, F_OK) == -1) + boot_editor->use_signature_files = false; + else + boot_editor->use_signature_files = true; +#else + boot_editor->use_signature_files = false; +#endif + talloc_set_destructor(boot_editor, boot_editor_destructor); boot_editor->cui = cui; boot_editor->item = item; @@ -563,9 +604,10 @@ struct boot_editor *boot_editor_init(struct cui *cui, int ncols1 = strncols(_("Device tree:")); int ncols2 = strncols(_("Boot arguments:")); + int ncols3 = strncols(_("Argument signature file:")); boot_editor->label_x = 1; - boot_editor->field_x = 2 + max(ncols1, ncols2); + boot_editor->field_x = 2 + max(max(ncols1, ncols2), ncols3); nc_scr_init(&boot_editor->scr, pb_boot_editor_sig, 0, cui, boot_editor_process_key, @@ -584,10 +626,15 @@ struct boot_editor *boot_editor_init(struct cui *cui, boot_editor->initrd = bd->initrd; boot_editor->dtb = bd->dtb; boot_editor->args = bd->args; + if (boot_editor->use_signature_files) + boot_editor->args_sig_file = bd->args_sig_file; + else + boot_editor->args_sig_file = ""; boot_editor_find_device(boot_editor, bd, sysinfo); } else { boot_editor->image = boot_editor->initrd = - boot_editor->dtb = boot_editor->args = ""; + boot_editor->dtb = boot_editor->args = + boot_editor->args_sig_file = ""; } boot_editor->pad = newpad( diff --git a/ui/ncurses/nc-cui.c b/ui/ncurses/nc-cui.c index 0c355cc..09b63b0 100644 --- a/ui/ncurses/nc-cui.c +++ b/ui/ncurses/nc-cui.c @@ -543,6 +543,7 @@ static int cui_boot_option_add(struct device *dev, struct boot_option *opt, cod->bd->initrd = talloc_strdup(cod->bd, opt->initrd_file); cod->bd->dtb = talloc_strdup(cod->bd, opt->dtb_file); cod->bd->args = talloc_strdup(cod->bd, opt->boot_args); + cod->bd->args_sig_file = talloc_strdup(cod->bd, opt->args_sig_file); /* This disconnects items array from menu. */ result = set_menu_items(cui->main->ncm, NULL); @@ -566,6 +567,7 @@ static int cui_boot_option_add(struct device *dev, struct boot_option *opt, pb_log(" image '%s'\n", cod->bd->image); pb_log(" initrd '%s'\n", cod->bd->initrd); pb_log(" args '%s'\n", cod->bd->args); + pb_log(" argsig '%s'\n", cod->bd->args_sig_file); /* Re-attach the items array. */ result = set_menu_items(cui->main->ncm, cui->main->items);
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. Signed-off-by: Timothy Pearson <tpearson@raptorengineering.com> --- configure.ac | 64 +++++++ discover/Makefile.am | 12 +- discover/boot.c | 155 ++++++++++++++--- discover/boot.h | 30 ++++ discover/device-handler.c | 6 + discover/device-handler.h | 1 + discover/gpg.c | 381 ++++++++++++++++++++++++++++++++++++++++++ discover/gpg.h | 73 ++++++++ discover/grub2/builtins.c | 8 + discover/kboot-parser.c | 6 + discover/pxe-parser.c | 7 + discover/user-event.c | 16 +- discover/yaboot-parser.c | 7 + lib/pb-protocol/pb-protocol.c | 13 ++ lib/types/types.h | 2 + ui/common/discover-client.c | 1 + ui/common/discover-client.h | 1 + ui/ncurses/nc-boot-editor.c | 51 +++++- ui/ncurses/nc-cui.c | 2 + 19 files changed, 807 insertions(+), 29 deletions(-) create mode 100644 discover/gpg.c create mode 100644 discover/gpg.h