Bug #1745454 “[MIR] libfprint” : Bugs : libfprint package : Ubuntu

Revision history for this message

Didier Roche-Tolomelli (didrocks) wrote on 2018-02-12:

#1

Looks generally good, some small things I need to get some clarification on:

[ required ]
- There is some lintian warning on the udev rule: udev-rule-missing-uaccess
* Check for a fix with upstream
* Override it in lintian with rationale

[ optional ]
- debian/copyright is clearly out of date, would be better to get it updated with latest copyright owners
- bonus point if you introduce dh_missing --fail-missing
- the -dev package ships a .la file that we normally strip on normal install

As for fprintd, I would like to get a security review on this one to confirm everything's fine.

Changed in libfprint (Ubuntu):
assignee:	nobody → Canonical Security Team (canonical-security)

Sebastien Bacher (seb128) on 2018-02-27

Changed in libfprint (Ubuntu):
assignee:	Canonical Security Team (canonical-security) → Ubuntu Security Team (ubuntu-security)

Revision history for this message

Seth Arnold (seth-arnold) wrote on 2018-06-28:

#2

Download full text (5.9 KiB)

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

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

- 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 'powm...

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

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

- 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
  memmove()?

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

- sanitize_image() does not validate reasonable height and width
  parameters

- fpi_img_is_sane() does not validate reasonable height and width
  parameters

- 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
  malloc()

- 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 https://anonscm.debian.org/git/fingerforce/libfprint.git
W: libfprint source: vcs-deprecated-in-debian-infrastructure vcs-browser https://anonscm.debian.org/gitweb/?p=fingerforce/libfprint.git
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.

Thanks

Changed in libfprint (Ubuntu):
assignee:	Ubuntu Security Team (ubuntu-security) → nobody

Revision history for this message

Sebastien Bacher (seb128) wrote on 2018-08-24:

#3

Thanks for the review, the lintian warnings have been addressed in debian&cosmic with the changes from
https://launchpad.net/ubuntu/+source/libfprint/1:0.8.2-1

> W: libfprint-dev: priority-extra-is-replaced-by-priority-optional
> W: libfprint0: priority-extra-is-replaced-by-priority-optional

* debian/control: Bump all the packages to priority optional, priority extra
is now deprecated

> W: libfprint source: debian-rules-uses-unnecessary-dh-argument dh ... --parallel (line 7)

* debian/rules: Do not explicitly pass --parallel to debhelper as it's the
default now

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

* debian/libfprint0.lintian-overrides: Overrides udev-rule-missing-uaccess,
we don't want the readers to be accessible by unprivileged users

Changed in libfprint (Ubuntu):
importance:	Undecided → High
status:	New → Fix Committed

Revision history for this message

Matthias Klose (doko) wrote on 2018-08-30:

#4

Override component to main
libfprint 1:0.8.2-2 in cosmic: universe/libs -> main
libfprint-dev 1:0.8.2-2 in cosmic amd64: universe/libdevel/extra/100% -> main
libfprint-dev 1:0.8.2-2 in cosmic arm64: universe/libdevel/extra/100% -> main
libfprint-dev 1:0.8.2-2 in cosmic armhf: universe/libdevel/extra/100% -> main
libfprint-dev 1:0.8.2-2 in cosmic i386: universe/libdevel/extra/100% -> main
libfprint-dev 1:0.8.2-2 in cosmic ppc64el: universe/libdevel/extra/100% -> main
libfprint-dev 1:0.8.2-2 in cosmic s390x: universe/libdevel/extra/100% -> main
libfprint-doc 1:0.8.2-2 in cosmic amd64: universe/doc/optional/100% -> main
libfprint-doc 1:0.8.2-2 in cosmic arm64: universe/doc/optional/100% -> main
libfprint-doc 1:0.8.2-2 in cosmic armhf: universe/doc/optional/100% -> main
libfprint-doc 1:0.8.2-2 in cosmic i386: universe/doc/optional/100% -> main
libfprint-doc 1:0.8.2-2 in cosmic ppc64el: universe/doc/optional/100% -> main
libfprint-doc 1:0.8.2-2 in cosmic s390x: universe/doc/optional/100% -> main
libfprint0 1:0.8.2-2 in cosmic amd64: universe/libs/extra/100% -> main
libfprint0 1:0.8.2-2 in cosmic arm64: universe/libs/extra/100% -> main
libfprint0 1:0.8.2-2 in cosmic armhf: universe/libs/extra/100% -> main
libfprint0 1:0.8.2-2 in cosmic i386: universe/libs/extra/100% -> main
libfprint0 1:0.8.2-2 in cosmic ppc64el: universe/libs/extra/100% -> main
libfprint0 1:0.8.2-2 in cosmic s390x: universe/libs/extra/100% -> main
19 publications overridden.

Changed in libfprint (Ubuntu):
status:	Fix Committed → Fix Released

Ubuntu
libfprint package

[MIR] libfprint

Bug Description

Other bug subscribers

Remote bug watches

Ubuntulibfprint package

[MIR] libfprint

Bug Description

Other bug subscribers

Remote bug watches

Ubuntu
libfprint package