My review from mid-september. I'd also like to suggest using Coverity on this codebase. The 'delete_variable()' portion was handled upstream already: https://github.com/rhinstaller/fwupdate/commit/8139030084c51418778815ed283237553b1cec47
I also understand that Microsoft is the correct signing entity, so feel free to ignore that too. But fwup_set_up_update() and fwup_strerror_r() absolutely must be addressed before we move fwupdate to main. Thanks. ======== I reviewed fwupdate version 0.4-4 as checked into wily. This shouldn't be considered a full security audit; I also did not investigate the EFI specification or systems to validate the EFI code. - fwupdate provides a shim to update EFI boot order and an EFI program to install firmware updates. - Build-Depends: debhelper, pkg-config, libpopt-dev, libefivar-dev, lsb-release, gnu-efi, pesign, elfutils - Does not daemonize - pre/post inst/rm scripts are boilerplate - No initscripts - No Dbus services - No setuid - Provides /usr/bin/fwupdate - No sudo fragments - No udev rules - No test suite; one test executable ./linux/tester.c exercises very little of the library/application - No cronjobs - Clean build logs I'm skipping the usual summary-of-code to include only the errors and oddities I discovered while reading the code: - needs pesign from universe - fwup_strerror_r() has unreachable statements, looks very buggy. Needs rewrite. - fwup_set_up_update() calls free(relpath) after calling relpath = onstack(relpath,...). This is a severe error. - fwup_set_up_update() calls open(O_CREAT) without specifying permissions, the permissions will be set to garbage - fwup_set_up_update() fullpath memory leak via open(fullpath...) error - fwup_set_up_update() mkostemps() call does not need O_CREAT or O_RDWR - fwup_set_up_update() nested read()/write() loop is intricate; stdio.h fgetc() and fputc() loop with final fflush() might be easier to reason about and probably no less efficient. - fwup_set_up_update() probably needs a rewrite -- it is doing too much and its memory management is too confused which leads to a severe bug. - extensive use of stack for storage; is stack able to handle the largest EFI variables? - put_info() does ssize_t arithmetic without checking for overflows -- how large will efidp_size() ever grow? - get_info() free(local) may be called on uninitialized pointer if efi_get_variable() returns success but data_size < sizeof(*local) - fwup_resource_iter_create() memory leak if opendir() or dirfd() fail - utf8len() and ucs2len() handle limit=0 differently, > vs >= - utf8len() may read beyond the end of a malformed utf8 string that is shorter than limit - utf8_to_ucs2() may read beyond the end of a malformed utf8 string that is shorter than max or may read beyond the end of utf8+max. - print_system_resources() and main() have user-visible strings that aren't localized - delete_variable() contains unreachable code, is this a patch failure? - All the signed files are signed by 'Red Hat Test Certificate" -- is this suitable for deployment? Most of the code is written in a defensive style; however, there are significantly troubling bugs and poorly written functions that need to be addressed. This software also makes the assumption that stack memory is plentiful and can handle large allocations. Unfortunately the extensive use of stack space for storage has confused several portions of the program; fwup_set_up_update() for example has memory corruption issues due to calling free() on memory that was allocated via alloca(). fwup_strerror_r() looks completely unused and never tested. The fwup_set_up_update() and fwup_strerror_r() functions need work. delete_variable() needs a domain expert to determine if it is correct. I strongly recommend that this program be run with Valgrind or ASAN as the memory management is significantly more complicated than usual and I may have overlooked other mistakes. This program needs work before we promote it to main. At a minimum fwup_set_up_update(), fwup_strerror_r() need attention. An EFI expert needs to confirm that delete_variable() is correct. An expert needs to confirm that the Red Hat Test Certificate is the correct certificate to use to sign the binaries. Thanks -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/1508926 Title: [MIR] fwupdate To manage notifications about this bug go to: https://bugs.launchpad.net/ubuntu/+source/fwupdate/+bug/1508926/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
