Comment 5 for bug 1936907

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

I reviewed adsys 0.8 as checked into jammy. This shouldn't be
considered a full audit but rather a quick gauge of maintainability. As
usual with golang code, there's vastly more code in the package than we've
authored, and it's not feasible to review the entirety.

adsys allows network administrators to include Ubuntu systems in Windows
Group Policy ecosystem. There's easy support for a lot of individual
tunable elements, as well as generic support for running both machine and
user scripts on login, logout, etc.

- CVE History:
  none :)
- Build-Depends?
  Build-Depends: debhelper-compat (= 13),
               golang-go (>= 2:1.16~),
- pre/post inst/rm scripts?
  mostly added automatically by dh_ helpers; registers and unregisters pam
  module, enables systemd units, purges and unmasks systemd units, etc.
- init scripts?
- systemd units?
  sets up socket activation, sets up timers, refreshes policies, runs
  machine scripts in machine units, runs user scripts in user units
- dbus services?
- setuid binaries?
- binaries in PATH?
  adsysd, adsysctl
- sudo fragments?
  /etc/sudoers.d/99-adsys-privilege-enforcement is under control of the

"%admin ALL=(ALL) !ALL\n"
"%sudo ALL=(ALL:ALL) !ALL\n"

contentSudo += fmt.Sprintf("\"%s\" ALL=(ALL:ALL) ALL\n", e)

  these are very powerful; I'd appreciate a second set of eyes here :)

- polkit files?
  yes, seems reasonable
- udev rules?
- unit tests / autopkgtests?
  yes, many tests, run during the build
- cron jobs?
  none, systemd timer units used instead
- Build logs:
  the shell completion files are dumped during build, it's a bit noisy,
  but otherwise looks clean

- Processes spawned?
  Yes -- pam module, copied from pam_exec.c
  Yes -- adsys spawned from the user manager will run scripts, seems okay
- Memory management?
  Most is golang, safe enough
  pam module has some memory leaks; when reported to upstream pam_exec.c
  folks, they appear to be leaning towards leaking even more memory :) so
  probably fine.
- File IO?
  Some issues, raised elsewhere.
- Logging?
  pam module looked fine
- Environment variable usage?
  NO_COLOR and KRB5CCNAME, seemed safe
- Use of privileged functions?
- Use of cryptography / random number sources etc?
- Use of temp files?
- Use of networking?
  grpc; to the extent I looked at it, it felt safe enough
- Use of WebKit?
- Use of PolicyKit?
  yes, internal/authorizer/authorizer.go
  looks up process start time by searching *backwards* through
  /proc/pid/stat file for a ), then looking forward 19 fields. I didn't
  double-check the math but it sure sounds promising.

- Any significant cppcheck results?
  memory leaks in pam_adsys.c, upstream for inspiration pam_exec didn't seem bothered
- Any significant Coverity results?
- Any significant shellcheck results?
- Any significant bandit results?

adsys is carefully written, well-documented, and didrocks and jibel were
very responsive to comments and feedback.

Security team ACK for promoting adsys to main.

I filed a few bugs along the way:

And some miscellaneous notes I took:

Are there any conditions that can be added to adsys-boot.service to make
it less likely to spam the journal every five seconds for ten hours when on an airplane?

pam_adsys.c update_policy() arggv leak in fork() failure
pam_adsys.c update_machine_policy() arggv leak in fork() failure
pam_adsys.c update_machine_policy() -- status != 0 looks like it ought to
work but I don't think that's how that API is supposed to be used
pam_adsys.c pam_sm_open_session() -- gethostname() indentation is funny


./internal/policies/scripts/scripts.go -- typo %qto

Both these use /tmp/adsysd/ .. paths in configuration files -- is this
the recommended way to use adsys? It'd be nice if the configs were "real",
production-ready, defaults, etc.