There are several problems with this patch.
* The description of the vulnerability is inaccurate. There are boot
loaders beyond legacy GRUB and GRUB2. Any boot loader that doesn't behave
like legacy GRUB will have this vulnerability--not just GRUB2. iPXE seems
to behave like legacy GRUB. I don't know about SYSLINUX. The patch should
point out that boot loaders other than GRUB2 may be vulnerable.
* The patched code does not match either the comments or the commit log
message. With this patch, tboot removes the first command line argument in
the non-legacy-GRUB case, not the GRUB2 case. Therefore it breaks iPXE
multiboot configuration scripts: it's now necessary to add a dummy argument
to each multiboot line, since tboot now drops the first argument.
* The code is questionable. With this patch, tboot decides that the boot
loader is legacy GRUB if its name string begins with "GNU GRUB 0". In GRUB,
this name string is a compile-time configuration parameter (PACKAGE_STRING,
set by configure), not a hardcoded value. If you configure a GRUB2
bootloader as version "GNU GRUB 02.00" would be treated as legacy GRUB by
tboot, and thus still suffer from the vulnerability. Completely defeating
the security of tboot seems a high price to pay for an unlucky choice of
name.
* Fundamentally, the patch does not actually fix the vulnerability: tboot
can still exclude the first command line argument from measurement. True,
the patch makes it much less likely to affect typical systems running GRUB2
today. But it still tries to guess what certain boot loaders might do (add
an extra argument or not) based on incomplete information (the name
string), which breaks some configurations (like iPXE), and leaves others
vulnerable (those that happen to be named starting with "GNU GRUB 0").
In my opinion, tboot shouldn't try to be clever. It should just measure the
command line that the boot loader passes to it--end of story. If that means
users need to figure out whether their boot loader passes the name of the
module as an argument on the module command line in order to pre-compute
the PCRs accurately, so be it. In this case, less is more.
--Ed
On Wed, Jul 23, 2014 at 6:45 PM, Gang Wei <gang....@intel.com> wrote:
> changeset 0efdaf7c5348 in /hg/p/tboot/code
> details:
> http://hg.code.sf.net/p/tboot/code/code?cmd=changeset;node=0efdaf7c5348
> description:
> Security Fix: TBOOT Argument Measurement Vulnerability for GRUB2 +
> ELF Kernels
>
> Remove the first cmdline argument in GRUB2 and ELF kernel case, to
> avoid
> passing unmeasured argument to ELF kernel like Xen. Below are the
> detailed
> flaw report from James Blake.
>
> One essential function TBOOT performs as part of a measured and
> verified
> launch includes measuring the arguments passed to GRUB modules.
> However,
> current versions of TBOOT used on systems loading an ELF kernel
> have a
> vulnerability that allows the first argument to any GRUB module to
> go
> unmeasured, which may result in undetected system compromise.
>
> This vulnerability stems from TBOOT's official workaround for
> accommodating
> GRUB2 multiboot behavior. Specifically, from the TBOOT README:
>
> GRUB2 does not pass the file name in the command line field of the
> multiboot
> entry (module_t::string). Since the tboot code is expecting the
> file name as
> the first part of the string, it tries to remove it to determine
> the command
> line arguments, which will cause a verification error. The
> "official"
> workaround for kernels/etc. that depend on the file name is to
> duplicate the
> file name in the grub.config file like below:
>
> menuentry 'Xen w/ Intel(R) Trusted Execution Technology' {
> recordfail
> insmod part_msdos
> insmod ext2
> set root='(/dev/sda,msdos5)'
> search --no-floppy --fs-uuid --set=root
> 4efb64c6-7e11-482e-8bab-07034a52de39
> multiboot /tboot.gz /tboot.gz logging=vga,memory,serial
> module /xen.gz /xen.gz iommu=required dom0_mem=524288
> com1=115200,8n1
> module /vmlinuz-2.6.18-xen /vmlinuz-2.6.18-xen
> root=/dev/VolGroup...
> module /initrd-2.6.18-xen.img /initrd-2.6.18-xen.img
> module /Q35_SINIT_17.BIN
> }
>
> To illustrate the severity of the bug, consider that on affected
> distributions,
> it would be possible to edit a GRUB command line from:
>
> module /vmlinuz /vmlinuz normal-arguments
>
> to:
>
> module /vmlinuz single normal-arguments
>
> Where 'single' replaces the typical placeholder argument. This
> modification goes
> undetected by TBOOT and consequently the assertion that the system
> has been
> measured and verified is undermined. Namely, the final measurement
> shown in the
> TPM PCR-18 does not change to reflect the modification.
>
> Reported-by: James Blake <bla...@ainfosec.com>
> Signed-off-by: Gang Wei <gang....@intel.com>
>
> diffstat:
>
> tboot/common/loader.c | 40 ++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 40 insertions(+), 0 deletions(-)
>
> diffs (64 lines):
>
> diff -r 6f931b98ea43 -r 0efdaf7c5348 tboot/common/loader.c
> --- a/tboot/common/loader.c Sun Jun 22 20:12:20 2014 -0700
> +++ b/tboot/common/loader.c Wed Jul 23 18:40:35 2014 -0700
> @@ -54,6 +54,7 @@
> #include <txt/txt.h>
> #include <mle.h>
> #include <txt/acmod.h>
> +#include <cmdline.h>
>
> /* copy of kernel/VMM command line so that can append 'tboot=0x1234' */
> static char *new_cmdline = (char *)TBOOT_KERNEL_CMDLINE_ADDR;
> @@ -952,6 +953,39 @@
> }
> }
>
> +static const char *get_boot_loader_name(loader_ctx *lctx)
> +{
> + if (LOADER_CTX_BAD(lctx))
> + return NULL;
> + if (lctx->type == MB1_ONLY ){
> + if (((multiboot_info_t *)lctx->addr)->flags & MBI_BTLDNAME)
> + return (char *)((multiboot_info_t
> *)lctx->addr)->boot_loader_name;
> + return NULL;
> + }
> +
> + /* currently must be type 2 */
> + struct mb2_tag *start = (struct mb2_tag *)(lctx->addr + 8);
> + start = find_mb2_tag_type(start, MB2_TAG_TYPE_LOADER_NAME);
> + if (start)
> + return &((struct mb2_tag_string *)start)->string[0];
> +
> + return NULL;
> +}
> +
> +static void remove_filename_from_modules_cmdline(loader_ctx *lctx)
> +{
> + if (LOADER_CTX_BAD(lctx))
> + return;
> +
> + for ( unsigned int i = 0; i < get_module_count(lctx); i++ ) {
> + module_t *m = get_module(lctx, i);
> + char *cmdline = get_module_cmd(lctx, m);
> + const char *adjusted_cmdline = skip_filename(cmdline);
> + if ( adjusted_cmdline != NULL && cmdline != adjusted_cmdline )
> + strncpy(cmdline, adjusted_cmdline, strlen(cmdline));
> + }
> +}
> +
> static
> void *remove_first_module(loader_ctx *lctx)
> {
> @@ -1242,6 +1276,12 @@
>
> /* fix for GRUB2, which may load modules into memory before tboot
> */
> move_modules(g_ldr_ctx);
> +
> + /* for GRUB 2, remove the filename in mods' cmdline */
> + const char *loader_name = get_boot_loader_name(g_ldr_ctx);
> + if ( loader_name != NULL && strncmp(loader_name, "GNU GRUB 0",
> 10) )
> + remove_filename_from_modules_cmdline(g_ldr_ctx);
> +
> }
> else {
> printk(TBOOT_INFO"assuming kernel is Linux format\n");
>
>
> ------------------------------------------------------------------------------
> Want fast and easy access to all the code in your enterprise? Index and
> search up to 200,000 lines of code with a free copy of Black Duck
> Code Sight - the same software that powers the world's largest code
> search on Ohloh, the Black Duck Open Hub! Try it now.
> http://p.sf.net/sfu/bds
> _______________________________________________
> Tboot-changelog mailing list
> tboot-change...@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/tboot-changelog
>
------------------------------------------------------------------------------
Slashdot TV.
Video for Nerds. Stuff that matters.
http://tv.slashdot.org/
_______________________________________________
tboot-devel mailing list
tboot-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tboot-devel
