[Bug 1508926] Re: [MIR] fwupdate

[Seth Arnold]

I reviewed fwupdate version 0.5-1 as checked into xenial. This should not
be considered a full security audit but rather a quick gauge of
maintainability.

- fwupdate helps Linux hosts install and run UEFI-based updates
- Build-Depends: debhelper, pkg-config, libpopt-dev, libefivar-dev,
  lsb-release, gnu-efi, elfutils
- Does not itself do networking or cryptography
- Does not itself daemonize
- pre/post inst/rm looks to properly undo EFI installed bits on uninstall
- No dbus services
- No setuid files
- fwupdate executable in the path
- No sudo fragments
- No udev rules
- No tests run during build or autopkgtest
- No cronjobs
- Relatively clean build logs

- No subprocesses spawned
- Memory management is very unusual (e.g. onstack() function) but looks
  disciplined and careful
- File IO sets up EFI boot environments, looks careful
- Logging looks careful
- One environment variable, looked careful, best to say it should only be
  set by an admin: LIBFWUP_ESRT_DIR
- Privileged operations are privileged by nature of writing to the EFI
  boot space, variables
- No cryptography
- No networking
- Temporary file handling looked careful
- No WebKit
- Clean cppcheck, some shellcheck issues
- No PolicyKit

This code is clean and well-written. I had reviewed an earlier version of
fwupdate and reported some issues and had some suggestions; the fwupdate
team took those suggestions seriously and addressed everything with aplomb
and style. (I didn't investigate each change individually but the end
result is clear, the package is clean and a pleasure to read.)

Security team ACK for promoting fwupdate to main.

Here's the notes I took while reviewing this package, along with a lintian
error and shellcheck output:

- fwup_set_up_update() lots of work between final fputc() and fclose() --
  does this need fflush(fout); fsync(outfd); before the work?
- get_fd_and_media_path() asprintf error path should not use "goto out;"
  because the fullpath variable is undefined (by asprintf(3)).


Lintian error:
E: libfwup0: postinst-must-call-ldconfig usr/lib/x86_64-linux-gnu/libfwup.so.0.5


shellcheck output:
./linux/cleanup.in:10:20: note: Double quote to prevent globbing and word splitting. [SC2086]
./linux/bash-completion:8:40: note: Double quote to prevent globbing and word splitting. [SC2086]
./linux/bash-completion:14:52: warning: This { is literal. Check expression (missing ;/\n?) or quote it. [SC1083]
./linux/bash-completion:14:66: warning: This } is literal. Check expression (missing ;/\n?) or quote it. [SC1083]
./linux/bash-completion:14:77: note: Double quote to prevent globbing and word splitting. [SC2086]
./debian/fwupdate.postinst:28:25: note: Double quote to prevent globbing and word splitting. [SC2086]
./debian/fwupdate.postinst:29:24: note: Double quote to prevent globbing and word splitting. [SC2086]
./debian/fwupdate.postinst:34:25: note: Double quote to prevent globbing and word splitting. [SC2086]
./debian/fwupdate.postinst:35:28: note: Double quote to prevent globbing and word splitting. [SC2086]
./debian/fwupdate.postrm:25:29: note: Double quote to prevent globbing and word splitting. [SC2086]
./debian/scripts/install:11:34: note: Useless cat. Consider 'cmd < file | ..' or 'cmd file | ..' instead. [SC2002]
./debian/scripts/install:15:15: warning: For loops over find output are fragile. Use find -exec or a while read loop. [SC2044]
./debian/scripts/install:22:22: note: Double quote to prevent globbing and word splitting. [SC2086]
./debian/scripts/install:32:25: note: Double quote to prevent globbing and word splitting. [SC2086]
./debian/scripts/install:34:17: note: Double quote to prevent globbing and word splitting. [SC2086]
./debian/scripts/install:35:20: note: Double quote to prevent globbing and word splitting. [SC2086]
./debian/scripts/install:37:14: note: Double quote to prevent globbing and word splitting. [SC2086]
./debian/scripts/install:37:23: note: Double quote to prevent globbing and word splitting. [SC2086]
./debian/libfwup0.install:3:1: note: bindir appears unused. Verify it or export it. [SC2034]
./debian/libfwup0.install:5:1: note: includedir appears unused. Verify it or export it. [SC2034]
./debian/libfwup0.install:7:1: note: mandir appears unused. Verify it or export it. [SC2034]
./debian/libfwup0.install:9:6: note: Double quote to prevent globbing and word splitting. [SC2086]
./debian/libfwup-dev.install:3:1: note: bindir appears unused. Verify it or export it. [SC2034]
./debian/libfwup-dev.install:7:1: note: mandir appears unused. Verify it or export it. [SC2034]
./debian/libfwup-dev.install:11:6: note: Double quote to prevent globbing and word splitting. [SC2086]
./debian/libfwup-dev.install:12:6: note: Double quote to prevent globbing and word splitting. [SC2086]


Thanks


** Changed in: fwupdate (Ubuntu)
     Assignee: Seth Arnold (seth-arnold) => (unassigned)

-- 
You received this bug notification because you are a member of Ubuntu
Foundations Bugs, which is subscribed to fwupdate in Ubuntu.
https://bugs.launchpad.net/bugs/1508926

Title:
  [MIR] fwupdate

Status in fwupdate package in Ubuntu:
  In Progress

Bug description:
  [Availability]
  fwupdate is available in universe and builds on all the architectures it is designed to work on (amd64, i386, armhf, arm64)

  [Rationale]
  fwupdate is a new EFI component for processing firmware updates for systems which support it. It will be used to allow users to run these upgrades, if their EFI BIOS allows, without the need to boot into Windows since it might not be available to them. This is only available for users with UEFI.

  [Security]
  There are no security reports open for fwupdate. However, fwupdate also ships an EFI binary and as such requires careful security review. Moreover, the fwupdate userland binary writes to ESRT tables in order to speak to the fwupdate EFI binary, so should benefit a security review of its own.

  [Quality assurance]
  Meets requirements. The package currently has bugs filed, but they are already handled although must wait to be fixed as they would require another round of signing of the EFI binary by Microsoft.

  [UI Standards]
  The application is translatable, although currently lacking is translations due to its relative newness.

  [Dependencies]
  All build and binary dependencies are in main.

  [Standards compliance]
  The package meets standards.

  [Maintenance]
  The package is quite small and does not require much effort on our part to maintain, aside from the occasional signing for the fwupdate EFI binary. It is maintained by the Debian EFI team in Debian; and subscribed to by foundations-bugs for Ubuntu.

  [Background information]
  fwupdate is a command-line too as well as an EFI binary for use to enable and apply firmware updates using the UEFI Capsule Update specification, which allows for flashing new firmware on BIOS or system components without requiring the use of a specific operating system, given that it can be done directly from the EFI firmware interfaces.

To manage notifications about this bug go to:
https://bugs.launchpad.net/ubuntu/+source/fwupdate/+bug/1508926/+subscriptions