Comment 19 for bug 1891157

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

I reviewed ipp-usb 0.9.13-1ubuntu1 as checked into groovy. This shouldn't be
considered a full audit but rather a quick gauge of maintainability.

ipp-usb is an http proxy to provide ipp printing support to USB devices.

- CVE History:
  No CVEs in our database, very new
- Build-Depends: debhelper-compat (= 13),
               libusb-1.0-0-dev,
               libavahi-common-dev,
               libavahi-client-dev,
               pkg-config,
               dh-golang,
               golang-go (>= 2:1.14~2) | golang-any,
               golang-github-openprinting-goipp-dev,

- pre/post inst/rm scripts?
  automatically added by debhelper
- init scripts?
  none
- systemd units?
  Simple one-shot, starts after avahi and cups, runs ipp-usb with no
  configurable parameters
- dbus services?
  none
- setuid binaries?
  none
- binaries in PATH?
  /sbin/ipp-usb
- sudo fragments?
  none
- polkit files?
  none
- udev rules?
  matches usb devices, chowns to root:lp, 664 -- is this too wide?
- unit tests / autopkgtests?
  A handful of simple tests, no adversarial tests; the tests are for
  papersize size comparisons and parsing the configuration file.
- cron jobs?
  none
- Build logs:
  clean

- Processes spawned?
  none
- Memory management?
  None
- File IO?
  Only a handful of files are used with predictable paths
- Logging?
  logging rarely checked error conditions, probably fine
- Environment variable usage?
  none
- Use of privileged functions?
  No obvious uses
- Use of cryptography / random number sources etc?
  none
- Use of temp files?
  none
- Use of networking?
  networking primarily used go's http library; probably okay
- Use of WebKit?
  none
- Use of PolicyKit?
  none

- Any significant Coverity results?
  No, only two results:
  - resp is used before a nil check; it'll probably just crash if it hits
    this
  - sha1 used for uuid generation, probably this is a standard of some
    sort; it's not critical

The ipp-usb binary is missing PIE, apparently Go PIE is too buggy to use
by default. Hopefully someday we'll be able to turn this on for all golang
packages in one go.

gosec identified a lot of cases of ignoring error returns. Some, like
errors in logging, are probably not a big deal, but others I did wonder
how the program is supposed to make progress if there's an ignored
earlier. This worries me. I hope the application would just crash though.

Extensive use of atomic operations worries me that locking should have
been used instead. I didn't understand the reason for these to be atomic
while reading the code, and it's entirely possibly that they're needlessly
atomic. If there's a reason why they are atomic, then probably more effort
needs to be put into making this thread-safe.

The code is otherwise straightforward.

Security team ACK for promoting ipp-usb to main.

Thanks