[MIR] fwupdate

[Mathieu Trudel-Lapierre]

[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.

[Seth Arnold (seth-arnold)]

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 writte...

[Mario Limonciello (superm1)]

It's worth mentioning that upstream saw this and has corrected many things upstream:
https://github.com/rhinstaller/fwupdate/issues/32#event-467050566

Also, it's available in debian-next w/ packaging here: http://anonscm.debian.org/cgit/uefi/fwupdate.git/log/?h=debian-next
That will be released into Debian unstable when upstream tags the next release.

[Jackson Doak (noskcaj)]

The 0.5 release is in ubuntu now, fixing the majority of the issues above

[Michael Terry (mterry)]

Back to Seth I guess!

[Robert Ancell (robert-ancell)]

Required for fwupd MIR (bug 1536871)

[Mario Limonciello (superm1)]

Just wanted to check in on this security review for MIR. Seth is this still in your queue?

[Seth Arnold (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. C...

[Mario Limonciello (superm1)]

Thanks Seth.
I've submitted upstream a fix for get_fd_and_media_path and the various shellcheck fixes. I expect they should be merged for the next fwupdate release.
https://github.com/rhinstaller/fwupdate/pull/45/commits

I'm unsure if fflush(fout) is needed between fputc() and fclose(). I think that's better answered by upstream.
https://github.com/rhinstaller/fwupdate/issues/46

The shellcheck fixes in debian/ I've commited to debian-next (which will be uploaded on the next fwupdate release).
http://anonscm.debian.org/cgit/uefi/fwupdate.git/log/?h=debian-next

Let me know if anything called out is a blocker for promotion, otherwise expect fixes for all at next fwupdate release.

[Michael Terry (mterry)]

What's stopping fwupdate from migrating from proposed? update_excuses has it as a valid candidate.

Further fixes are great, but not blockers for promotion. With Seth's ACK and a look myself at the packaging, fwupdate seems fine to me.

[Mario Limonciello (superm1)]

I think it might be be because 0.5-2 synced from Debian and fwupdate-signed needed to be redone. I just bumped fwupdate-signed version to 1.4 to see if that does the trick on migration.

Does fwupdate-signed need an MIR too? Or since it's built from fwupdate and just signing the binary it's implicitly promoted too?

[Michael Terry (mterry)]

Looked at the -signed version too, it's fine.

What pulls the -signed version in? Does it need to be seeded or does something depend on it?

[Mario Limonciello (superm1)]

Once the MIR for fwupd (https://bugs.launchpad.net/ubuntu/+source/fwupd/+bug/1536871) is done I'm expecting that firmware support will be turned on in gnome-software (https://bugs.launchpad.net/ubuntu/+source/gnome-software/+bug/1544376).

In my mind either gnome-software should recommend the signed version, or the desktop seeds should.

[Mario Limonciello (superm1)]

Yeah it looks like it was the -signed package. That upload caused the proposed migration to happen.

[Mario Limonciello (superm1)]

@mterry,

I noticed this is still actually in universe. Is there something else that needs to be done for it to finish the move to main?

[Michael Terry (mterry)]

Yeah, it doesn't get pulled into main until something in main depends on it. So in this case, we're probably waiting for fwupd's MIR (bug 1536871). And then for gnome-software to depend on fwupd and pull it all in.

[Mario Limonciello (superm1)]

Ah OK, thanks that makes sense.

On Fri, Mar 11, 2016 at 9:40 PM Michael Terry <email address hidden>
wrote:

> Yeah, it doesn't get pulled into main until something in main depends on
> it. So in this case, we're probably waiting for fwupd's MIR (bug
> 1536871). And then for gnome-software to depend on fwupd and pull it
> all in.
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> 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
>

[Steve Langasek (vorlon)]

Override component to main
fwupdate 0.5-2 in xenial: universe/misc -> main
fwupdate 0.5-2 in xenial amd64: universe/admin/optional/100% -> main
fwupdate 0.5-2 in xenial arm64: universe/admin/optional/100% -> main
fwupdate 0.5-2 in xenial armhf: universe/admin/optional/100% -> main
fwupdate 0.5-2 in xenial i386: universe/admin/optional/100% -> main
libfwup-dev 0.5-2 in xenial amd64: universe/libdevel/optional/100% -> main
libfwup-dev 0.5-2 in xenial arm64: universe/libdevel/optional/100% -> main
libfwup-dev 0.5-2 in xenial armhf: universe/libdevel/optional/100% -> main
libfwup-dev 0.5-2 in xenial i386: universe/libdevel/optional/100% -> main
libfwup0 0.5-2 in xenial amd64: universe/libs/optional/100% -> main
libfwup0 0.5-2 in xenial arm64: universe/libs/optional/100% -> main
libfwup0 0.5-2 in xenial armhf: universe/libs/optional/100% -> main
libfwup0 0.5-2 in xenial i386: universe/libs/optional/100% -> main
13 publications overridden.

[Steve Langasek (vorlon)]

fwupdate-signed promoted to main.