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

Reply via email to