[MIR] ipp-usb

[Summary]
- This is just the review for ipp-usb, the other two are not yet reviewed.
- MIR Team ack from a packaging POV.
- This does need a security review, so I'll assign Ubuntu-security

Prereq's for Promotion:
- get the Desktop team to ack and subscribe to the packages
- prep a change to system-config-printer-udev replacing the ippusbxd dependency
- golang-gopkg-ini.v2 needs to be owned and MIR processed as well

Since the above might make the requester or the Desktop team reconsider this
I'm holding back on the two golang packages until explicitly confirmed that
this stays the way to go and that owning the package will be fine.
I'll add a task for it and set the two golang libs to incomplete - if you want
to own and MIR this provide the details and set it back to new.

Recommended:
- try to get the service more confined

[Duplication]
As mentioned by the bug report already, there is ippusbxd which is in main.
This shall be demoted from main to universe to allow just one (2) ipp-on-usb
program in main.

There is only one dependency holding it in main:
  Reverse-Recommends
  * system-config-printer-udev (for ippusbxd)

ipp-usb is depended on by "cups-daemon" in groovy-proposed.
Can system-config-printer-udev be changed as part of the promotion (once ready)
to be replaced to ipp-usb as well. So that we can demote ippusbxd in the same
step when we promote ipp-usb?

[Dependencies]
OK:
- no other Dependencies to MIR due to this (avahi and libusb are in main)
- no -dev/-debug/-doc packages that need exclusion

[Embedded sources and static linking]
OK:
- no embedded source present
- no static linking, well except the usual go lib inclusion :-/

There are more Built-Using than the bug is filed right now:
Built-Using:
  - golang0.14 (= 1.14.4-1ubuntu2), (in main)
  - golang-github-openprinting-goipp (= 1.0~git20200517.da79ff1-2), (part of
    this MIR)
  - golang-gopkg-ini.v2 (= 1.57.0-1) (Missing, this will have to be MIRed and
  owned as well then.

[Security]
OK:
- history of CVEs does not look concerning, but ippusbxd had issues and we can
  expect this might have as well at some point
- does not use webkit2,2
- does not use lib*v9 directly
- does not process arbitrary web content
- does not use centralized online accounts
- does not integrate arbitrary javascript into the desktop
- does not deal with system authentication (eg, pam), etc)

Problems:
- does not parse data formats
- does not open a port
- does run a daemon as root
  - any chance to run the service more confined e.g. protected* features of
    systemd?
  - if there is a chance even as non-root?

[Common blockers]
OK:
- does not FTBFS currently
- does have a test suite that runs at build time (but fairly minimal)
  - test suite fails will fail the build upon error.
- no translation present, but none needed for this case (user visible)?
- not a python package, no extra constraints to consider int hat regard
- Go package that uses dh-golang

Problems:
- The package has a team bug subscriber
  please get a full Team to subscribe to the package
  The printing team (https://launchpad.net/~ubuntu-printing) is the perfect
  team to actually handle things here and is subscribed al...

Removed recommendation of ippusbxd from system-config-printer in version 1.5.12-0ubuntu2.

ipp-usb should get seeded.

Subscribed to bugs from golang-gopkg-ini.v1 package.

I've subscribed ~desktop-bugs to golang-github-openprinting-goipp, ipp-usb, golang-gopkg-ini.v1 and sane-airscan

"Ubuntu Desktop Bugs" (~desktop-bugs) is now subscribed to all the three packages: ipp-usb, golang-github-openprinting-goipp, golang-gopkg-ini.v1

I have talked with the upstream maintainer of ipp-usb, Alexander Pevzner now and he will emove the dependency on ini.v1.

"Desktop Packages" (~desktop-package) is now subscribed to ipp-usb and golang-github-openprinting-goipp.

Thanks Till and Ken for sorting out subscriptions and the golang-gopkg-ini.v1 situation.
I'm going to do a "golang-github-openprinting-goipp" review then ...

[Summary]
MIR team ack for the packaging and overall quality.
But it needs a security review as well, assigning ubuntu-security.

[Duplication]
Only the code that is part of ippusbxd is similar in function and it is
planned to replace ippusbxd in main with this.

[Dependencies]
OK:
- no other Dependencies to MIR due to this
- no -dev/-debug/-doc packages that need exclusion

[Embedded sources and static linking]
OK:
- no embedded source present, despite go package \o/
- static linking (go package), but no deps \o/

[Security]
OK:
- history of CVEs does not look concerning
  but it is rather new and the ipp protocol and implementations had many
  issues so one can expect this might happen here as well
- does not run a daemon as root
  but it will be used in a demon
- does not use webkit1,2
- does not use lib*v8 directly
- does not open a port
- does not process arbitrary web content
- does not use centralized online accounts
- does not integrate arbitrary javascript into the desktop
- does not deal with system authentication (eg, pam), etc)

Problems:
- does parse data formats
  The parsing in go is supposed to be safer, especially since transport is
  done done by the standard lib of go that should be exercised and tested
  a lot already.
  Due to that and the history of IPP in general it will need a security review.

[Common blockers]
OK:
- does not FTBFS currently
- does have a test suite that runs at build time
  - test suite fails will fail the build upon error.
- The package has a team bug subscriber
- no translation present, but none needed for this case (user visible)?
- not a python package, no extra constraints to consider int hat regard
- Go package that uses dh-golang

Problems:
- does not have a test suite that runs as autopkgtest
  But it is only a lib, an autopkgtest would be better in things using it

[Packaging red flags]
OK:
- Ubuntu does not carry a delta
- symbols tracking not applicable for this kind of code.
- d/watch is present and looks ok
- Upstream update history is unclear (too new)
- Debian/Ubuntu update history is unclear (too new)
- the current release is packaged
- promoting this does not seem to cause issues for MOTUs that so far
  maintained the package
- no massive Lintian warnings
- d/rules is rather clean
- Does not have further Built-Using
- Go Package that follows the Debian Go packaging guidelines

[Upstream red flags]
OK:
- no Errors/warnings during the build
- no use of sudo, gksu, pkexec, or LD_LIBRARY_PATH
- no use of user nobody
- no use of setuid
- no important open bugs (crashers, etc) in Debian or Ubuntu
- no dependency on webkit, qtwebkit, seed or libgoa-*
- no embedded source copies
- not part of the UI for extra checks

Note: There is still some unanswered question Christian asked on ipp-usb about confinement and the daemon that needs to be answered before going further.

golang-github-openprinting-goipp:
[Summary]
- MIR Team ack from a packaging and code POV.
- Needs Security team review

[Duplication]
Nothing to add over the top request. Providing and use of Go native binding is welcome.

[Dependencies]
OK:
- no other Dependencies to MIR
- only one -dev package that needs to be in main due to the nature of Go library (statically linked)

[Embedded sources and static linking]
OK:
- no embedded source present
- no static linking, only ship source code

[Security]
OK:
- no CVEs, but really fresh new package.
- it does use Go battle-proof http stack
- does not use webkit2,2
- does not use lib*v9 directly
- does not process arbitrary web content
- does not use centralized online accounts
- does not integrate arbitrary javascript into the desktop
- does not deal with system authentication (eg, pam), etc)

Problems:
- parse data formats, but only in pure Go, via consts. Should be safe but better to double check with Security
- does not open a port
- does not run a daemon as root

[Common blockers]
OK:
- does not FTBFS currently
- does have a test suite that runs at build time (but fairly minimal)
  - test suite fails will fail the build upon error.
- no translation present, but none needed
- not a python package, no extra constraints to consider int hat regard
- Go package that uses dh-golang
- Team subscription is now OK

[Packaging red flags]
OK:
- Ubuntu does not carry a delta
- symbols tracking not applicable for this kind of code.
- d/watch is present and looks ok
- Upstream update history is good
- Debian/Ubuntu update history is good, but short
- there is no official release yet so it’s a git snapshot (latest upstream commit)
- promoting this does not seem to cause issues for MOTUs that so far maintained the package
- no massive Lintian warnings
- d/rules is clean and minimal
- Go package that follows the Debian Go packaging guidelines

[Upstream red flags]
OK:
- standard and comprehensible Go code.
- use of go modules.
- no Errors/warnings during the build
- no use of sudo, gksu, pkexec, or LD_LIBRARY_PATH
- no use of user nobody
- no use of setuid
- no important open bugs (crashers, etc) in Debian or Ubuntu
- no dependency on webkit, qtwebkit, seed or libgoa-*
- no embedded source copies
- not part of the UI for extra checks
- no upstream bug opened at this date (none over the lifetime of the project)

FWIW, I did start the golang-gopkg-ini.v1 review before the discussion went to invalidate it, I will still post what I found in case we have another MIR one day.

golang-gopkg-ini.v1:
[Summary]
- Some work is needed as right now :
  * golang-github-smartystreets-goconvey-dev MIRing. Blocking this one
- The rest looks good both from a packaging and code POV.
- Needs security review for parsing data

[Duplication]
Nothing to add over the top request. Providing and use of Go native binding is welcome.

[Dependencies]
Needs fixing:
- golang-gopkg-ini.v1-dev depends on golang-github-smartystreets-goconvey-dev which is in universe. One way would be to separate the _test.go files from regular code one. That way, only test files are using convey and no dependency is needed. We can hope that Depends:misc DTRT.
- only one -dev package that needs to be in main due to the nature of Go library (statically linked)

[Embedded sources and static linking]
OK:
- no embedded source present
- only ship source code, so no static linking

[Security]
OK:
- no CVEs, but really fresh new package.
- it does use Go battle-proof http stack
- does not use webkit2,2
- does not use lib*v9 directly
- does not process arbitrary web content
- does not use centralized online accounts
- does not integrate arbitrary javascript into the desktop
- does not deal with system authentication (eg, pam), etc)

Problems:
- parse data formats, but only in pure Go, via consts. If one day we promote it, it will need a security review
- does not open a port
- does not run a daemon as root

[Common blockers]
OK:
- does not FTBFS currently
- does have a test suite that runs at build time (but fairly minimal)
  - test suite fails will fail the build upon error.
- no translation present, but none needed
- not a python package, no extra constraints to consider int hat regard
- Go package that uses dh-golang
- Team subscription is now OK

[Packaging red flags]
OK:
- Ubuntu does not carry a delta
- symbols tracking not applicable for this kind of code.
- d/watch is present and looks ok
- Upstream update history is good
- Debian/Ubuntu update history is good, but short
- there is no official release yet so it’s a git snapshot (latest upstream commit)
- promoting this does not seem to cause issues for MOTUs that so far maintained the package
- no massive Lintian warnings
- d/rules is clean and minimal
- Go package that follows the Debian Go packaging guidelines

[Upstream red flags]
OK:
- standard and comprehensible Go code.
- no use of go modules :-/ running go mod init though before running tests
- CI is running as part of upstream build. One of the targetted plateform is ubuntu-latest.
- no Errors/warnings during the build
- no use of sudo, gksu, pkexec, or LD_LIBRARY_PATH
- no use of user nobody
- no use of setuid
- no important open bugs (crashers, etc) in Debian or Ubuntu
- no dependency on webkit, qtwebkit, seed or libgoa-*
- no embedded source copies
- not part of the UI for extra checks
- no upstream bug opened at this date (none over the lifetime of the project)

The removal of the dependency on golang-gopkg-ini.v1 has been done on the upstream GitHub:

https://github.com/OpenPrinting/ipp-usb/commit/a68d21c18e
   Added .INI file parser, to replace gopkg.in/ini.v1

https://github.com/OpenPrinting/ipp-usb/commit/7d2b8e81ff
   gopkg.in/ini.v1 replaced with home-made .INI parser

eleases and packaging will follow soon.

ipp-usb 0.9.11 got released upstream:

https://github.com/OpenPrinting/ipp-usb/releases/tag/0.9.11

I already asked the Debian developers for updating their packages. As soon as they have uploade it I will sync it to Ubuntu.

The replacement of ippusbxd by ipp-usb is also important as ippusbxd sometimes causes problems which makes people end up not being able to print and scan at all.

See bug 1878974

An HP printer which works with HPLIP triggers the launch of ippusbxd, because the printer also supports IPP-over-USB and driverless printing and scanning, but due to the bugs (or architectural shortcomings) of ippusbxd the communication between the printer and CUPS/SANE does not work correctly, making printing and scanning fail.

On the development of ipp-usb these shortcomings were identified and fixed and according to user reports ipp-usb is reliable.

ippusbxd got removed from Debian now.

ipp-usb 0.9.12 without build dependency on INI.v1 is now in Groovy.

Uploaded ipp-usb 0.9.12-1ubuntu1 to eliminate remaining dependencies on Universe:

----------
ipp-usb (0.9.12-1ubuntu1) groovy; urgency=medium

  * Removed build dependency on golang-any, a meta package in Universe,
    to allow one of golang-go or gccgo, put golang-go in its place
    as this is the only of the two in Main
  * Removed unneeded man page build with ronn, to eliminate build
    dependency on ronn in Universe

 -- Till Kamppeter <email address hidden> Wed, 25 Aug 2020 17:53:00 +0200
----------

The dependency on
  golang-gopkg-ini.v1 (= 1.57.0-1)
is indeed dropped.
Thanks Till!

This seems to be ready from the MIR POV, but waits for security review and ack now.

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

Thanks Seth, thereby for ipp-uxb itself everything is ready and since it shows up in component mismatches I'll set it to Fix-Committed.

For now golang-github-openprinting-goipp stays open waiting for the security review.

FYI: also on golang-github-openprinting-goipp since nothing holds in the golang build deps automatically, you'd need to seed them.
@didrocks would you mind to prep that in the seeds?

Override component to main
ipp-usb 0.9.13-1ubuntu1 in groovy: universe/misc -> main
ipp-usb 0.9.13-1ubuntu1 in groovy amd64: universe/comm/optional/100% -> main
ipp-usb 0.9.13-1ubuntu1 in groovy arm64: universe/comm/optional/100% -> main
ipp-usb 0.9.13-1ubuntu1 in groovy armhf: universe/comm/optional/100% -> main
ipp-usb 0.9.13-1ubuntu1 in groovy ppc64el: universe/comm/optional/100% -> main
ipp-usb 0.9.13-1ubuntu1 in groovy riscv64: universe/comm/optional/100% -> main
ipp-usb 0.9.13-1ubuntu1 in groovy s390x: universe/comm/optional/100% -> main
7 publications overridden.

Hi Seth,

thank you for security review of the ipp-usb package. My few comments:

> resp is used before a nil check; it'll probably just crash if it hits this

resp is always used after check of returned error, and if this check passed, resp cannot be nil. In one place resp is explicitly checked against nil after error check and a couple of uses. This nil check is excessive and probably present due to historic reasons, I will remove it

> sha1 used for uuid generation

Yes, and in this context cryptographic properties of the SHA1 hash are not important

> gosec identified a lot of cases of ignoring error returns

I reviewed them all, and they all are not critical and falls into the following categories:

1) Error can't happen, or no actions required if it has happened. For example, if I can't set TCP keep-alive parameters (which is very unlikely to happen), OK, I can continue with default parameters. And if I can't send few bytes to the HTTP client, because client has closed its connection, nobody cares about these lost bytes.

2) Error can happen, but reasonable recovery not possible, though it is possible to continue ignoring the error. For example, if I can't write to log file, OK, part of logs will be lost, but I can't report this situation anyway.

3) Further (dependent on failed) actions perform appropriate error checking. For example, I don't check error when creating lock file directory, but attempt to create a lock file in that directory is checked.

4) Error may cause some non-critical functionality to be lost, but program will still continue to work correctly. For example, there is some state associated with each device, if it can't be saved due to file I/O error, it is not considered a serious error. Program can live without it, with some minor regression on its convenience (assigned TCP ports or DNS-SD name override, assigned during DNS-SD name conflict resolution, will not be preserved between sessions, for example)

> Extensive use of atomic operations worries me that locking should have been used instead

Atomics used mostly with purpose to minimize cost of USB connection allocation/deallocation instrumentation and to achieve a near-zero overhead, when debug logging is actually turned off. Even in a case of mistake, logs will be slightly inaccurate, but it will not cause any serious synchronization problems.

Alexander, thanks for your explanations.

On Thu, Oct 08, 2020 at 07:31:36PM -0000, Alexander Pevzner wrote:
> thank you for security review of the ipp-usb package. My few comments:

Hello Alexender, thank you very much for the feedback review! It's very
helpful, and very encouraging. Thanks. :)

I reviewed golang-github-openprinting-goipp 1.0.0-1 as checked into
groovy. This shouldn't be considered a full audit but rather a quick
gauge of maintainability.

golang-github-openprinting-goipp is a low-level serializer/deserializer
for IPP protocol messages.

- CVE History:
  - No CVEs in our database
- Build-Depends:
  debhelper-compat (= 13),
  dh-golang,
  golang-any
- pre/post inst/rm scripts?
  none
- init scripts?
  none
- systemd units?
  none
- dbus services?
  none
- setuid binaries?
  none
- binaries in PATH?
  none
- sudo fragments?
  none
- polkit files?
  none
- udev rules?
  none
- unit tests / autopkgtests?
  pretty decent collection of smoketests
- cron jobs?
  none
- Build logs:
  No actual build, very clean

- Processes spawned?
  none
- Memory management?
  none
- File IO?
  none
- Logging?
  none
- Environment variable usage?
  none
- Use of privileged functions?
  none
- Use of cryptography / random number sources etc?
  none
- Use of temp files?
  none
- Use of networking?
  not directly, but will process data from a network. Looked clean.
- Use of WebKit?
  none
- Use of PolicyKit?
  none
- Any significant Coverity results?
  none (no build, no coverity)

decodeString() and decodeBytes() look like they could be made to hang
forever if the remote peer sends along a length of N but then doesn't send
along N actual bytes of data in the stream afterwards.

decodeString() does no well-formedness checks on inputs: no utf8
enforcement, no newlines / whitespace etc. This appears to be used with
Name inputs. Much of the code appears able to handle arbitrary bytes well
enough, but the String() method for Collection doesn't do anything to
ensure that an output Name doesn't have { or }, [ or ], or a comma, etc.
Injecting newlines and new headers is a quite popular exploit technique;
are these routines susceptible to similar problems?

There's a pretty decent array non-adversarial test cases.

 W: golang-github-openprinting-goipp source: debhelper-compat-file-is-missing
 W: golang-github-openprinting-goipp source: package-uses-deprecated-debhelper-compat-version 1
 E: golang-github-openprinting-goipp source: package-uses-debhelper-but-lacks-build-depends
 W: golang-github-openprinting-goipp source: newer-standards-version 4.5.0 (current is 4.1.4)

Security team ACK for promoting golang-github-openprinting-goipp to main.

Alexander, Till, it would be nice if we could run these components as non-root. It'd be wonderful to have some systemd seccomp enforcement, as well as AppArmor profiles. Please consider what steps could be taken to reduce the privileges of these services.

Thanks

golang-github-openprinting-goipp is ready as well then - thanks.

I also verified that without further changes it is held in main, currently detected and listed as component mismatch: https://people.canonical.com/~ubuntu-archive/component-mismatches-proposed.html
Source only movements to main (desktop-packages): golang-github-openprinting-goipp
Source only movements to main (ubuntu-printing): golang-github-openprinting-goipp

Therefore setting Fix Committed and once an AA moved it that will be fully done.

$ ./change-override -c main -S golang-github-openprinting-goipp
Override component to main
golang-github-openprinting-goipp 1.0.0-1 in groovy: universe/misc -> main
golang-github-openprinting-goipp-dev 1.0.0-1 in groovy amd64: universe/devel/optional/100% -> main
golang-github-openprinting-goipp-dev 1.0.0-1 in groovy arm64: universe/devel/optional/100% -> main
golang-github-openprinting-goipp-dev 1.0.0-1 in groovy armhf: universe/devel/optional/100% -> main
golang-github-openprinting-goipp-dev 1.0.0-1 in groovy i386: universe/devel/optional/100% -> main
golang-github-openprinting-goipp-dev 1.0.0-1 in groovy ppc64el: universe/devel/optional/100% -> main
golang-github-openprinting-goipp-dev 1.0.0-1 in groovy riscv64: universe/devel/optional/100% -> main
golang-github-openprinting-goipp-dev 1.0.0-1 in groovy s390x: universe/devel/optional/100% -> main
Override [y|N]? y
8 publications overridden.