Comment 2 for bug 1745454

Seth Arnold (seth-arnold) wrote :

I reviewed libfprint version 1:0.7.0-1 as checked into cosmic. This
shouldn't be considered a full security audit but rather a quick gauge of

- There are no CVEs for libfprint in our CVE database
- libfprint provides drivers to fingerprint readers, fingerprint storage,
  and manipulation
- Build-Depends: debhelper, libusb-1.0-0-dev, libglib2.0-dev,
  libpixman-1-dev, libxv-dev, libnss3-dev
- Does not daemonize
- Does not do networking
- Does not do cryptography
- postinst runs ~60 udevadm trigger commands for various hardware
  parameters, probably fine
- No initscripts
- No dbus services
- No systemd unit files
- No setuid files
- No binaries in usual bin/
- No sudo fragments
- Extensive udev rules, nodes set to 664, group plugdev
- No tests
- No cronjobs
- Build logs show deprecation warnings, unused variables warnings,
  misleading indentation
- Build logs show large number of lintian warnings and errors

- No subprocesses spawned
- Memory management frequently assumes no integer overflows in allocation
  requests, copy requests, etc. It's not up to modern quality.
- Some file IO, with names potentially coming from outside the library
- Logging functions looked okay
- Uses LIBFPRINT_DEBUG and HOME environment variables
- No privileged operations
- No cryptography
- No networking
- No temporary files
- No WebKit
- No JavaScript
- No PolicyKit
- ~six real issues found via cppcheck

This code is mixed quality. Some is pretty old and no longer up to modern
standards. Some is pretty good. Upstream authors mentioned insufficient
time to address issues due to faulty or malicious hardware. The things
I've found may very well only be issues in the face of malicious hardware.

However, malicious hardware abounds -- and people will pick anything that
looks like a USB mass storage device and plug it into their computer.

So I'm not thrilled about adding this attack surface to our distribution.

Furthermore, the Ubuntu security team is strongly of the opinion that
fingerprints themselves, even in an ideal world, are strictly *usernames*
and not suitable as passwords. Any configurations we support will adhere
to this. Screen unlocking via fingerprint is fine, because we also support
password-less logins and password-less unlocks if users choose to
configure their systems in such a fashion, and unlock-via-fingerprint is
not that different.

Here's some notes I took while reviewing the code base. There's a list of
lintian warnings and errors at the end:

- get_score_filename() assumes the listfile parameter NEVER receives "" as
  an input value. Also gives an inane error message if the outdir argument
  is "". I'm curious why basename does any strrchr() things, it feels like
  a mistake.

- fp_img_save_to_file() leaks 'fd' via r < 0 and r < write_size error
  return paths

- bz_load() leaks 'fd' via m != 3 && m != and m != nargs_expected return

- alloc_power_stats() multiple integer overflow possibilities; parameter
  'nstats' appears to come from outside the library in at least one code
  path, so this routine should handle large values properly: change the
  malloc() calls to calloc() calls.

- alloc_power_stats() leaks 'powmax_dirs' in pownorms failure case

- morph_TF_map() multiplies mw and mh together for memory allocations,
  loop bounds; I could not find any constraints on inputs for these
  parameters. It's possible the inputs come from within the library, I
  lost track at a global parameter. I think this routine should enforce
  reasonable sizes on these parameters before performing memory allocation
  and calcuating loop bounds.

- morph_TF_map() leaks 'cimage' via mimage memory allocation failure.

- morph_TF_map() -- should those loops be replaced with memcpy() or

- lfs_detect_minutiae_V2() memory allocation with iw*ih, no input bounds

- sanitize_image() does not validate reasonable height and width

- fpi_img_is_sane() does not validate reasonable height and width

- sanitize_image() is only called via fpi_imgdev_image_captured()

- pixelize_map() leaks 'pmap' via block dimensions that do not match

- pixelize_map() integer overflows in malloc(), loop bounds

- allocate_contour() integer overflows in malloc()

- main() in ./examples/img_capture_continuous.c integer overflows in

- gen_initial_maps() integer overflows in malloc(), memset() calls

- interpolate_direction_map() integer overflows in malloc(), memcpy() calls

- gen_high_curve_map() integer overflows in malloc(), memset() calls

- gen_initial_imap() integer overflows in malloc(), memset() calls

- gen_quality_map() integer overflows in malloc(), array index

Lintian messages:

W: libfprint-dev: priority-extra-is-replaced-by-priority-optional
E: libfprint changes: bad-distribution-in-changes-file unstable
W: libfprint source: debian-rules-uses-unnecessary-dh-argument dh ... --parallel (line 7)
W: libfprint source: vcs-deprecated-in-debian-infrastructure vcs-git
W: libfprint source: vcs-deprecated-in-debian-infrastructure vcs-browser
W: libfprint0: appstream-metadata-missing-modalias-provide lib/udev/rules.d/60-libfprint0.rules
W: libfprint0: priority-extra-is-replaced-by-priority-optional
W: libfprint0: udev-rule-missing-uaccess lib/udev/rules.d/60-libfprint0.rules:2 user accessible device missing TAG+="uaccess"
W: libfprint0: udev-rule-missing-uaccess lib/udev/rules.d/60-libfprint0.rules:6 user accessible device missing TAG+="uaccess"
[... many more ...]

I'd really like to see some of our resources used to address the long list
of integer overflows, memory leaks, etc. I found. I'd also like to see the
lintian errors either fixed or describe why they're not an issue.

Security team ACK for promoting libfprint to main is CONDITIONAL upon the
desktop team (or other stakeholder) investigating the lintian messages.