Comment 23 for bug 1790855

I reviewed gpsd 3.20-1 as checked into focal. This shouldn't be considered
a full audit but rather a quick gauge of maintainability. As noted in the
MIR description, this review only covers code and binaries which are
contained in the gpsd and libgps25 packages.

gpsd is a daemon (and associated library) to handle input from GPS
devices. The daemon translates the various GPS reciever inputs into a
standard format and multiplexes this out to the various connected
clients. It can also be used to connect to a remote gpsd instance as a
source as well. For the purpose of this MIR, gpsd is the standard way to
provide GPS time sources to chrony, which is now used as the time
synchronisation daemon in Ubuntu.

- CVE History:
  - 3 CVEs in project history (CVE-2004-1388 CVE-2013-2038 CVE-2018-17937)
  - 1 is still open for xenial, bionic and disco (CVE-2018-17937)
  - All fixed upstream in a timely manner
- Build-Depends
  - Nothing of note here (no crypto / networking libraries)
- pre/post inst/rm scripts
  - gpsd.postinst setups up the gpsd user account
  - gpsd.postrm removes the gpsd user account on purge
- No init scripts
- systemd units are used to spawn gpsdctl (see below) for GPS devices, and
  to manage the local GPSD socket and hence launch the GPSD daemon on
  demand when a local client connects
- No dbus services
- No setuid binaries
- gpsd binary package provides the following binaries in PATH
  - /usr/bin/ppscheck
  - /usr/sbin/gpsd
  - /usr/sbin/gpsdctl
- AppArmor policy:
  - /etc/apparmor.d/usr.sbin.gpsd
    - This looks quite sensible and provides access to the commonly used
      components and paths plus includes a site-specific
      <local/usr.sbin.gpsd> profile to allow for easy customisation
- No sudo fragments
- Uses udev rules to ensure GPS devices have correct name (/dev/gps%n) and
  the right systemd service (gpsdctl@%k) is spawned - the gpsdctl service
  spawns /usr/sbin/gpsdctl for each device which inturn spawns a gpsd
  instance to read from that device and set the device permissions so that
  the gpsd binary can still read from the device once it drops privileges
- unit tests / autopkgtests
  - Some unit tests are run during the build
  - autopkgtest exists - this does a basic test that the gpsd service and
    socket are available and active and that the output from the socket is
    the bare-minimum expected
- No cron jobs
- Build logs:
  - A single compile-time warning comes from an included system Qt header
    file using a deprecated copy method
    - /usr/include/x86_64-linux-gnu/qt5/QtCore/qvariant.h:275:25: warning: implicitly-declared 'constexpr QVariant::Private& QVariant::Private::operator=(const QVariant::Private&)' is deprecated [-Wdeprecated-copy]
  - Warning when generating man pages:
    - W: libgps-dev: manpage-has-errors-from-man usr/share/man/man5/gpsd_json.5.gz 272: warning [p 2, 2.2i, div '3tbd15,3', 0.8i]: can't break line
  - dpkg warnings:
    - dpkg-gensymbols: warning: debian/libqgpsmm25/DEBIAN/symbols doesn't match completely debian/libqgpsmm25.symbols
    - dpkg-gencontrol: warning: Recommends field of package gpsd: substitution variable ${python:Depends} used, but is not defined
      (this is also repeated for other packages but listed gpsd only here since this MIR is only for gpsd )

  - Various lintian errors:
    - E: gpsd changes: bad-distribution-in-changes-file unstable
    - W: gpsd source: package-needs-versioned-debhelper-build-depends 12
    - W: gpsd source: orig-tarball-missing-upstream-signature gpsd_3.20.orig.tar.xz
    - W: gpsd source: ancient-python-version-field x-python3-version 3.5
    - E: gpsd-dbg: python-script-but-no-python-dep usr/lib/gpsd/debug/test_clienthelpers.py #!python
    - E: gpsd-dbg: python-script-but-no-python-dep usr/lib/gpsd/debug/test_misc.py #!python
    - W: libgps-dev: manpage-has-errors-from-man
      usr/share/man/man5/gpsd_json.5.gz 272: warning [p 2, 2.2i, div
      '3tbd15,3', 0.8i]: can't break line
    - W: gpsd-clients: executable-not-elf-or-script
      usr/share/doc/gpsd-clients/examples/skyview.php
    - N: 1 tag overridden (1 info)
    - E: Lintian run failed (policy violation)

- Processes spawned
  - gpsd can be configured to run a hook script (/etc/gpsd/device-hook)
    when devices are activated / deactived - this is done by spawning
    system() with the path to the gps device and a fixed argument
    indicating if it was activated or deactivated. If an attacker were able
    to control the device name they would be able to get gpsd to execute
    arbitrary commands when the hook script is executed so whilst this is
    unlikely, this would be better if it was a call to execve() or similar
    to avoid possible shell-command injection in the call to
    system(). However, since it is unlikely an attacker could configure a
    device name which contained shell directives this is unlikely to be a
    practical threat.
- Memory management
  - gpsd appears to demonstrate quite good memory management and no obvious
    instances of errors were found - memory allocations are checked for
    failure, memcpy()'s which might overflow bounds are checked etc.
- File IO
  - PPS input is taken from a file in sysfs which is assumed trusted as is
    provided by the kernel
  - No files are created by gpsd
- Logging
  - Logging is generally done via the GPSD_LOG() macro which expands to
    call the gpsd_log() function to allow runtime log-level control - this
    inturn eventually uses vsnprintf(). There does not appear to be any
    instances where format string vulnerabilities could be abused. There
    are also uses of syslog() but again this looks quite safe.
- Environment variable usage
  - Uses GPSD_SHM_KEY when opening a connection to a GPSD daemon via shared
    memory
  - Uses LISTEN_PID and LISTEN_FDS to support systemd socket activation
  - These look safe
- Use of privileged functions
  - Drops privileges to the dialout group and gpsd user correctly during
    execution
  - Changes device permissions of default devices (ones specified on the
    command-line) before dropping permissions to ensure can be read
    afterwards
- No use of cryptography / random number sources etc
- No use of temp files
- Use of networking appears safe and sanitised appropriately
- No use of WebKit
- No use of PolicyKit

- One cppcheck result:
  - [monitor_italk.c:166]: (error) Shifting signed 32-bit value by 31 bits
    is undefined behaviour
    - This was apparently fixed in upstream commit
      https://gitlab.com/gpsd/gpsd/commit/b1b555e0cb37382655a3a7cb37f2f89cb89eda7a
      however on some platforms (32-bit ARM and x86 in particular) the use
      of unsigned long is not enough (as it is still only 32-bits wide) so
      this should likely be a uint64_t to ensure there is no possible
      undefined behaviour by shifting beyond the size of the underlying
      type
- Significant Coverity results:
  - Some sections of code use large stack allocated buffers (eg. 18436
    bytes) which could possibly overrun the stack size but this is quite
    unlikely in practice as these are not deep code paths etc
  - See attached list of other coverity findings.

gpsd appears to be quite well written and defensive. It contains a lot of
bespoke code to handle various custom GPS receiver protocols but in general
these appear to be safe. The daemon drops privileges safely and in general
runs unprivileged and includes an AppArmor profile which provides quite
good isolation from the rest of the system.

Security team ACK for promoting the gpsd and libgps25 binary packages to
main.