Oh, I have to agree that PXE/iso/syslinux related boot was impacted by this patch. But change the user behavior is not what we want to see. So I will try find better way to fix it instead of changing user behaviors.
Thanks Jimmy From: Ed Swierk [mailto:eswi...@skyportsystems.com] Sent: Tuesday, August 26, 2014 1:57 PM To: Wei, Gang Cc: tboot-devel@lists.sourceforge.net Subject: Re: [Tboot-changelog] changeset in code: Security Fix: TBOOT Argument Measurement Vuln... 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
