Comment 14 for bug 1686393

I reviewed gdm3 version 3.24.2-1ubuntu2 as checked into artful. This
should not be considered a full security audit but a quick gauge of
maintainability.

UCT has two CVEs: first, holding esc key allowed bypassing the lock screen.
Second, one REJECTed CVE that was assigned for the usual "desktop visible
shortly after suspend" issue that for some reason everyone is affected by
all the time. (It may not be user-friendly but locking before suspending
is the usual Linux way to make sure it's locked when re-waking.)

- gdm3 is a login/lock display manager
- Build-Depends: gnome-pkg-tools, debhelper, dconf-cli, intltool,
  libdbus-glib-1-dev, libglib2.0-dev, libgtk-3-dev, libpango1.0-dev,
  libcanberra-gtk3-dev, libfontconfig1-dev, libaccountsservice-dev,
  gnome-settings-daemon-dev, gnome-settings-daemon, libnss3-dev,
  libxcb1-dev, libx11-dev, libxau-dev, libxt-dev, libxext-dev, check,
  libgirepository1.0-dev, gobject-introspection, libpam0g-dev,
  libkeyutils-dev, libxdmcp-dev, libwrap0-dev, libxft-dev, libxi-dev,
  libxinerama-dev, libplymouth-dev plymouth-dev, yelp-tools,
  libselinux1-dev, libattr1-dev, iso-codes, libaudit-dev, docbook-xml,
  gsettings-desktop-schemas, libsystemd-dev, xserver-xorg-dev
- Does not itself do encryption
- Appears it still supports xdmcp
- pre-inst file deletes /etc/pam.d/gdm-launch-environment if upgrading
  from 3.10.0.1-3~ or earlier
- pre-rm file has very involved script to manage debconf and systemd
  service files
- post-inst file has involved script to add gdm group and user, manage
  debconf, systemd service files, convert gsettings to gconf, and restart
  gdm3 via invoke-rc.d
- post-rm file removes init scripts, /etc files, /var/*/gdm3 directories,
  gdm user, gdm group, and manages debconf
- initscript starts systemd logind, rebuilds configuration, uses
  start-stop-daemon to run gdm3
- systemd unit checks with /etc/X11/default-display-manager before
  running, loads in the environment from /etc/default/locale
- Fairly complicated dbus interfaces
- No setuid executables
- gdm-screenshot gdm3 and gdmflexiserver executables in the PATH
- No sudo fragments
- No udev rules

- Processes spawned extensively via glib wrappers. I didn't see any cases
  of unsafe data being mishandled but the amount of extra overhead in each
  execution is surprising.
- Memory management looked careful if wasteful.
- File management may suffer from leaky abstractions: e.g.
  create_auth_file() uses g_open() on the results of g_build_filename(),
  g_mkdir_with_parents(), and g_get_user_runtime_dir(), which doesn't have
  any error checking on most of these calls. (There's also no O_EXCL,
  O_NONBLOCK, O_NOCTTY, O_NOFOLLOW; I don't know if any of these would fit
  in the threat model of the application so they may not be security
  issues, but O_EXCL may be important for reliability.)

  Errors here appear to percolate to a NULL pointer added to an array in
  spawn_x_server() which will hopefully cause X to fail with an error
  ("expected argument" for the -auth parameter), but may have other
  consequences.
- Extensive logging, looked safe.
- Uses WAYLAND_DISPLAY RUNNING_UNDER_GDM GTK_MODULES DISPLAY XAUTHORITY
  GDM_SESSION_DBUS_ADDRESS GDM_SESSION_FOR_REAUTH XDG_VTNR WINDOWPATH PATH
  variables
- Sets GTK_MODULES XORG_RUN_AS_USER_OK DISPLAY XAUTHORITY
  DBUS_SESSION_BUS_ADDRESS WINDOWPATH WINDOWPATH PATH LOGNAME USER
  USERNAME HOME SHELL variables
- Privileged operations sometimes do not check return errors
- No cryptography
- Temporary files appear to be created safely
- No WebKit
- Clean cppcheck (three cppcheck errors)
- I didn't see polkit use

Here's some notes I took while reading the source code:

- 88 cases of "deprecation warning"
- chown and chmod errors in the build logs (below)
- chmod/chown in debian/: WARNING:
  debian/gdm3.postinst:chown -R gdm:gdm /var/lib/gdm3
- /bin/sh as shell in debian/: WARNING:
  debian/gdm3.prerm:#!/bin/bash
- dh: unable to load addon gnome: Can't locate
Debian/Debhelper/Sequence/gnome.pm in @INC (you may need to install the
Debian::Debhelper::Sequence::gnome module) (@INC contains: /etc/perl
/usr/local/lib/x86_64-linux-gnu/perl/5.22.1 /usr/local/share/perl/5.22.1
/usr/lib/x86_64-linux-gnu/perl5/5.22 /usr/share/perl5
/usr/lib/x86_64-linux-gnu/perl/5.22 /usr/share/perl/5.22
/usr/local/lib/site_perl /usr/lib/x86_64-linux-gnu/perl-base .) at (eval 13)
line 2.

jbicha reports the lintians are false positives [removed here], and filed:

https://bugzilla.gnome.org/783079 (chown)
https://bugzilla.gnome.org/783080
https://bugzilla.gnome.org/783081
https://bugzilla.gnome.org/783082

No response 48 days later. (Also, wow, I'm sorry this review took 48 days.)

- gdm_get_script_environment() comment claims to populate the hash with
  the existing environment but I don't see it. If it no longer does then
  the call to remove MAIL can be removed.

- gdm_get_script_environment() builds an environment that pretends either
  the user's home directory is the PWD or / is the PWD, but
  gdm_run_script() does not have any calls to change the current working
  directory.

- gdm_session_execute() has a codepath that sets PATH to "/bin:/usr/bin:."
  which is a poor default. . should not be included and /sbin:/usr/sbin
  probably should be.

- gdm_session_execute() may handle a PATH with value "" poorly. (I suspect
  the p == path branch will be taken and then startp will be set to point
  past the end of the path.)

- gdm_session_execute()'s memory handling is complicated; is this really
  better than libc's execvpe()?

- maybe_lock_screen() performs many allocations, string splitting, and
  directory walk operations on every use. Simply calling into execvp()
  twice (if the first fails) would save overhead and potential failure
  points. How often is this function called and when?

- I understand GTK_MODULES allows for essentially arbitrary code
  execution. Are there any ways to get gdm3 to run with privilege and
  unexpected values for this variable?

I don't like this codebase: in short it feels like over-complicated
abandonware. Even simple things ("execute this screensaver") involve
surprising amounts of runtime memory allocation and string handling to
save the authors from an array of strings and execv*() by hand. Standard
Unix primitives are wrapped in enough layers of abstraction that I'm
afraid I've read security bugs and completely overlooked them.

The coding standard feels adequate for a usual desktop application but not
to standard for privilege enforcement.

48 days without responses to bug reports is not ideal.

I beg the desktop team to consider investing whatever time it takes to
make lightdm appropriate for the task.

Security team grants a very begrudging ACK to promote gdm3 to main but
really would rather stick with lightdm if at all possible.

Thanks