[PATCH] discover/platform: platform boot args append after verification
Samuel Mendoza-Jonas
sam at mendozajonas.com
Mon Jul 9 11:19:39 AEST 2018
On Wed, 2018-07-04 at 08:32 +1000, Brett Grandbois wrote:
>
> On 03/07/18 16:17, Samuel Mendoza-Jonas wrote:
> > 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.
>
> That can get messy very quickly. For the very simple case of 2 RAUC
> slot + 2 console options is 4 different cmdline signatures. Start
> adding in data rate, partiy, etc, and the number of combinations start
> exploding.
I have been imagining the scenario slightly differently in that while all these
combinations exist, in a given environment only a subset would actually be
allowed. If you need that kind of flexibility combined with signed boot then
that is definitely going to get messy. I guess my question is, does it make
sense to have that kind of flexibility in the signed boot scenario?
> Then there's the question of how the signature is actually
> selected. In this case you could probably get away with a new boot hook
> parameter rather than a platform callout to do some sort of runtime
> cmdline sig file update. For a very small amount of options that would
> probably be sufficient but limits future flexibility as new options are
> needed. This is of course the hardest part of security: where do you
> draw the line between security and flexibility? IMHO a compiled-in
> in-house developed platform callout is by definition an acceptance of
> the flexibility tradeoff over the hard signature security. Would a
> configure enable option help alleviate concerns? In that case the
> callout has to be defined and the call explicitly enabled at build time?
I reckon the patch as it is currently implemented is fine - nothing will
happen unless a platform has defined that function - but your idea below
about having an option to selectively build platform files would definitely
add an extra layer of reassurance :)
Cheers,
Sam
>
> >
> > 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