Comment 10 for bug 1735499

Revision history for this message
Seth Arnold (seth-arnold) wrote :

I reviewed libblockdev version 2.16-2 as checked into bionic. This should
not be considered a full security audit but rather a quick gauge of
maintainability.

- libblockdev is a plugin-based library to work with block devices,
  providing an API based interface that knows how to either perform
  underlying operations directly or call the necessary command line
  applications as appropriate.

- There are no CVEs in our database

- Build-Depends: debhelper, libtool, dh-python, python3:any, libglib2.0-dev
  libgirepository1.0-dev, libcryptsetup-dev libdevmapper-dev libudev-dev
  libsystemd-dev, libdmraid-dev, libvolume-key-dev, libbytesize-dev,
  libnss3-dev libparted-dev libmount-dev libblkid-dev libpython3-dev,
  libkmod-dev gtk-doc-tools, gobject-introspection, pylint

- libblockdev does not itself daemonize
- no networking
- automatically generated pre/post inst/rm scripts
- no initscripts
- no dbus services
- no setuid files
- no executables in PATH
- no sudo fragments
- no udev rules
- There is a test suite; it is not run during the build. I suspect keeping
  this test suite disabled is the right approach.
- No cronjobs
- Noisy build logs, primarily from the documentation tools; not ideal, but
  at least not much from the code itself

- Several plugins execute helper tools; the glib helpers make the
  resulting code fairly complex, but it looks like the executions were
  carefully written
- memory management is careful; allocated memory is quickly freed when it
  is no longer used to simplify error handling
- normally 'files' being opened are block devices
- Logging functions looked careful
- No environment variable use
- Does not itself do cryptography -- drives LUKS, VeraCrypt, etc via
  helpers
- Does not do networking
- Does not use temporary files
- Does not use WebKit
- Does not use PolicyKit
- Does not use JavaScript
- Mostly clean cppcheck results; s390 code has legitimate errors:
  [src/plugins/s390.c:293]: (error) Used file that is not opened.
  [src/plugins/s390.c:293]: (error) Resource handle 'fd' freed twice.
  [src/plugins/s390.c:968]: (error) Resource leak: fd

The s390 code does not feel as mature as the rest of the code base.
I suspect a deeper inspection of the s390 sources would find more issues.
If it can be disabled without significant loss of functionality then I
recommend we disable it.

Security team ACK for promoting libblockdev to main.

Here's some notes I took while reviewing the code, in the hopes that they
are useful:

- bd_s390_dasd_online() double-closes fd, uses fd when it isn't open
- bd_s390_zfcp_offline() leaks fd in rc == EOF branch
- bd_s390_zfcp_online() calls fclose(fd) in a branch when fd is known to
  be NULL
- bd_s390_zfcp_scsi_offline() does not check fopen(hba_path, "r") for an
  error return
- bd_s390_zfcp_scsi_offline() does not check fopen(wwpn_path, "r") for an
  error return
- bd_s390_zfcp_scsi_offline() does not check fopen(lun_path, "r") for an
  error return

- bd_crypto_tc_open(), luks_format(), (and other functions) do not zero
  out secrets such as passwords. (This is difficult to do; the goal is not
  to be perfect, but to try. Simply adding memset_s() calls would be a
  step in the right direction.)

- Many routines in src/plugins/crypto.c call strlen(passwd), some other
  functions are described as taking binary data in password parameters. Is
  there a chance of one interface setting, changing, or removing a
  password, that other interfaces cannot work with?

- write_escrow_data_file() writes what looks like a secret key equivalent
  to a file but does not itself set a safe umask before doing so.

Thanks