[PATCH] discover/platform: platform boot args append after verification
Samuel Mendoza-Jonas
sam at mendozajonas.com
Tue Jul 3 16:17:00 AEST 2018
On Tue, 2018-07-03 at 10:19 +1000, Brett Grandbois wrote:
>
> On 02/07/18 11:38, Samuel Mendoza-Jonas wrote:
> > On Mon, 2018-06-11 at 08:24 +1000, Brett Grandbois wrote:
> > > In a signed-boot environment, the cmdline options are verified along
> > > with the kernel, which is to be expected. However this can cause
> > > problems in environments where the system needs to make a runtime
> > > adjustment to the cmdline args as now the signature will fail. There
> > > is currently boot hook support for adjusting args but that happens
> > > before verification and will therefore fail, and also having a callout
> > > to a possible modified script/app seems to be too much of a security
> > > hole. After some experimentation it appears the best solution to this
> > > issue is to add a new platform callout that is invoked after
> > > verification as part of the kexec cmdline args append. Since the
> > > platform(s) are compiled-in they can't be (easily) altered on the system
> > > and the default NULL behavior of the platform system means that it must
> > > be explicitly invoked by a platform designer, which would be an implicit
> > > acknowledgement that the possible security hole is acceptable.
> > > Runtime update of dtb's in a signed-boot environment will also face this
> > > issue but that will be addressed separately.
> >
> > Hi Brett,
> >
> > This is a pretty clean solution to the problem, however modifying boot
> > arguments at all has been something we've tried to stay away from in
> > general, let alone modifying signed arguments :)
> > I already have an idea of what the aim is here but would you mind giving
> > a bit of a description of your use case so we can think about it in
> > context?
> >
> > Cheers,
> > Sam
>
> Sure Sam,
>
> So the basic idea is that this is a replacement mechanism for the boot
> hooks in a signed boot environment, specifically in this case for boot args.
>
> In all situations, the boot hooks run right before kexec_load(), where
> they are able to modify the args, dtb, even the image or initrd. While
> still can be run in a signed-boot environment, any modifications done by
> the hooks will almost certainly cause a signature failure as those are
> performed in kexec_load(), which again as noted above is run after the
> hooks. This is a problem in environments where signed-boot is enabled
> but system modifications are still necessary. As an example, for our
> platform we have signed-boot enabled and also a requirement for
> persistent storage and runtime configuration of the kernel console
> settings. This is an x86/ACPI environment so we don't have dtb's, the
> only way to configure this at runtime is appending a 'console=XX' to the
> kernel boot args. But since the cmdline args as specified in the conf
> files are signed, appending the console configuration will violate the
> signature and the boot will fail. I believe the powerpc platform has
> something similar but achieves it via dtb, which would also fail in a
> signed-boot environment. We have another boot arg flag we need to pass
> that has to do with our updater (RAUC slot if you're interested) but I
> think the console is a good simple example to highlight the issue.
One idea that came up while discussing this was possibly allowing
multiple signature files; if you know that there's a finite set of
'console=$tty' or 'rauc_param=foo' parameters that are allowable, could
we have a group of signatures that match each permutation? Then we can
still "actually" validate the signature when booting.
The platform callout is probably the least scary way of doing this but it
means taking very close care in that callout to make sure Petitboot can't
be tricked into appending something unintended, otherwise we'll
"validate" something that could be malicious.
>
> So I did quite a bit of experimentation and determined the least of all
> the evils is to allow the system to append boot args after the signature
> check. I admit it is opening up a security hole, but it is implemented
> as a platform callout. By default undefined platform callouts are not
> run so nothing happens in the default case. In situations like ours we
> have to explicitly define the platform callout to do this, and doing so
> have acknowledged that this functionality is a higher priority than the
> possible security hole we are opening. Hmm, maybe a good future feature
> is a configure option to specify which platform source files are to be
> included? If anyone is keen on that I'm happy to take it on.
>
> A more locked-down approach would be to have an explicit list of args
> that could be set by the system and appended directly, like for example
> the global 'boot_console' if defined is always appended as a 'console=',
> but that lacks flexibility and I think would just cause continual
> requests for new args to be added.
>
> >
> > >
> > > Signed-off-by: Brett Grandbois <brett.grandbois at opengear.com>
> > > ---
> > > discover/boot.c | 24 ++++++++++++++++++++----
> > > discover/platform.c | 7 +++++++
> > > discover/platform.h | 4 ++++
> > > 3 files changed, 31 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/discover/boot.c b/discover/boot.c
> > > index 2a0d333..c66264a 100644
> > > --- a/discover/boot.c
> > > +++ b/discover/boot.c
> > > @@ -62,6 +62,7 @@ static int kexec_load(struct boot_task *boot_task)
> > > struct process *process;
> > > char *s_initrd = NULL;
> > > char *s_args = NULL;
> > > + char *p_args = NULL;
> > > const char *argv[7];
> > > char *s_dtb = NULL;
> > > const char **p;
> > > @@ -121,10 +122,19 @@ static int kexec_load(struct boot_task *boot_task)
> > > *p++ = s_dtb; /* 4 */
> > > }
> > >
> > > - s_args = talloc_asprintf(boot_task, "--append=%s",
> > > - boot_task->args ?: "\"\"");
> > > - assert(s_args);
> > > - *p++ = s_args; /* 5 */
> > > + p_args = platform_boot_args_append(boot_task);
> > > +
> > > + if (p_args || (boot_task->args && strlen(boot_task->args))) {
> > > + if (p_args && (boot_task->args && strlen(boot_task->args)))
> > > + s_args = talloc_asprintf(boot_task, "--append=%s %s",
> > > + boot_task->args, p_args);
> > > + else
> > > + s_args = talloc_asprintf(boot_task, "--append=%s",
> > > + p_args ?: boot_task->args);
> > > +
> > > + assert(s_args);
> > > + *p++ = s_args; /* 5 */
> > > + }
> > >
> > > *p++ = local_image; /* 6 */
> > > *p++ = NULL; /* 7 */
> > > @@ -581,6 +591,12 @@ struct boot_task *boot(void *ctx, struct discover_boot_option *opt,
> > > talloc_free(boot_task);
> > > return NULL;
> > > }
> > > + /*
> > > + * no args is valid but the expectation is that the sigfile
> > > + * will have hashed ""
> > > + */
> > > + if (!boot_task->args)
> > > + boot_task->args = talloc_strdup(boot_task, "");
> > > }
> > >
> > > image_res = add_boot_resource(boot_task, _("kernel image"), image,
> > > diff --git a/discover/platform.c b/discover/platform.c
> > > index cc6306f..93cc4b7 100644
> > > --- a/discover/platform.c
> > > +++ b/discover/platform.c
> > > @@ -209,6 +209,13 @@ int platform_get_sysinfo(struct system_info *info)
> > > return -1;
> > > }
> > >
> > > +char *platform_boot_args_append(const struct boot_task *task)
> > > +{
> > > + if (platform && platform->boot_args_append)
> > > + return platform->boot_args_append(platform, task);
> > > + return NULL;
> > > +}
> > > +
> > > int config_set(struct config *newconfig)
> > > {
> > > int rc;
> > > diff --git a/discover/platform.h b/discover/platform.h
> > > index 5aa8e3f..9960c7d 100644
> > > --- a/discover/platform.h
> > > +++ b/discover/platform.h
> > > @@ -2,6 +2,7 @@
> > > #define PLATFORM_H
> > >
> > > #include <types/types.h>
> > > +#include "boot.h"
> > >
> > > struct platform {
> > > const char *name;
> > > @@ -11,6 +12,8 @@ struct platform {
> > > void (*pre_boot)(struct platform *,
> > > const struct config *);
> > > int (*get_sysinfo)(struct platform *, struct system_info *);
> > > + char* (*boot_args_append)(struct platform *,
> > > + const struct boot_task*);
> > > uint16_t dhcp_arch_id;
> > > void *platform_data;
> > > };
> > > @@ -20,6 +23,7 @@ int platform_fini(void);
> > > const struct platform *platform_get(void);
> > > int platform_get_sysinfo(struct system_info *info);
> > > void platform_pre_boot(void);
> > > +char *platform_boot_args_append(const struct boot_task*);
> > >
> > > /* configuration interface */
> > > const struct config *config_get(void);
More information about the Petitboot
mailing list