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