[MIR] ipp-usb

[Till Kamppeter]

IPP-over-USB
------------

ipp-usb is the second implementation of the IPP-over-USB standard. This allows the PWG's Internet Printing Protocol (IPP) which is currently the most common communication protocol for network printers also to be used via USB, simply by a network printer being emulated on localhost. Advantages are:

- IPP is a high-level bi-directional packet-based protocol for printing,
  scanning, and fax
- Full device capabilities can be polled from the device, together with
  using standardized
  printing and scanning data format driverless printing and scanning is
  possible
- Status, like loaded paper, toner levels, ... can get polled
- Printing and scanning can be performed simultaneously and independently
- The administration web interface can get accessed

ipp-usb detects supported devices automatically and advertises their full functionality via DNS-SD on localhost, CUPS and the appropriate SANE backends discover the device automatically then and it is immediately available, no drivers needed, it just works.

This makes thousands of printers, scanners, and multi-function devices work on USB, USB-only devices, like the scanner Canon Lide 400 get working for the first time.

Why ipp-usb? There is ippusbxd already
--------------------------------------

ippusbxd was the first implementation of IPP-over-USB, with the very same intentions, but it has problems which were not easily to be solved in C and so after a short discussion with me the author of the driverless scanning SANE backend sane-airscan (https://github.com/alexpevzner/sane-airscan) created a first draft of ipp-usb in Go within a few hours, which solved these problems. The project has matured with the time and seems to work perfectly.

See also here:

https://github.com/OpenPrinting/ippusbxd#before-you-begin-ippusbxd-or-ipp-usb

See the original README for the rationale of ipp-usb:

https://github.com/OpenPrinting/ipp-usb

----------
Unfortunately, the naive implementation, which simply relays a TCP connection to USB, does not work. It happens because closing the TCP connection on the client side has a useful side effect of discarding all data sent to this connection from the server side, but it does not happen with USB connections. In the case of USB, all data not received by the client will remain in the USB buffers, and the next time the client connects to the device, it will receive unexpected data, left from the previous abnormally completed request.

Actually, it is an obvious flaw in the IPP-over-USB standard, but we have to live with it.

So the implementation, once the HTTP request is sent, must read the entire HTTP response, which means that the implementation must understand the HTTP protocol, and effectively implement a HTTP reverse proxy, backed by the IPP-over-USB connection to the device.

And this is what the ipp-usb program actually does.
----------

Many users reported this to work perfectly and I am using it since its creation in January 2020 on a daily basis without problems.

Here are the issues of ippusbxd:

https://github.com/OpenPrinting/ippusbxd/issues

Especially

https://github.com/OpenPrinting/ippusbxd/issues/15

shows that packets of canceled connections can get stuck in buffer and spilled out on next connection. Problems of ippusbxd's architecture and lack of useful C libraries to solve this is discussed here.

https://github.com/OpenPrinting/ippusbxd/issues/14

The flakiness of the web admin interface is caused by above problems.

Due to this most other Linux distributions are also migrating to ipp-usb. Chrome OS was the last system sticking with ippusbxd but they have introduced their own replacement, ippusb_bridge (https://github.com/dgreid/platform2/tree/master/ippusb_bridge) now.

From the README.md of ippusb_bridge:

----------
The impetus for creating this was that ippusbxd has lost a lot of interest since the release of ipp-usb, a similar project written in Golang. Upstream provides only some oversight, and the code is pure C and has security bugs.
----------

With this the interest in ippusbxd has gone and ippusbxd can get considered deprecated.

goipp
-----

goipp (Debian/Ubuntu package: golang-github-openprinting-goipp) is a simple IPP support library in Go needed by ipp-usb and created together with ipp-usb.

Original description from

https://github.com/OpenPrinting/goipp:

----------
The goipp library is fairly complete implementation of IPP core protocol in pure Go. Essentially, it is IPP messages parser/composer. Transport is not implemented here, because Go standard library has an excellent built-in HTTP client, and it doesn't make a lot of sense to wrap it here.

High-level requests, like "print a file" are also not implemented, only the low-level stuff.
----------

[Availability]

Both ipp-usb and goipp got initially packaged in Debian and synced into Universe:

https://launchpad.net/ubuntu/+source/ipp-usb/0.9.10-2
https://launchpad.net/ubuntu/+source/golang-github-openprinting-goipp/0.0~git20200517.da79ff1-3

They build on all currently supported architectures.

[Rationale]

See introduction above. Replaces ippusbxd.

[Security]

For both ipp-usb and goipp:

No CVE on http://cve.mitre.org/cve/search_cve_list.html

No mention on https://www.openwall.com/lists/oss-security/

Not listed on http://people.ubuntu.com/~ubuntu-security/cve/universe.html

No SUID/SGID

ipp-usb is a system daemon, running as root, triggered by plugging a supported device (USB printing device with IPP protocol, 7/1/4) via UDEV and systemd. In default configuration the daemon listens only on localhost, port 60000 (and following ports if more than one device is connected). So the device is not exposed to the network and communication with it stays on the local machine.

goipp is a Go library, the only binary package of it, golang-github-openprinting-goipp-dev contains only Go source code and documentation.

[Quality assurance]

To use ipp-usb one simply installs it and plugs the device(s) to USB. The devices get auto-detected by UDEV and the daemon automatically started. It immediately advertises the device via DNS-SD only on localhost where CUPS and SANE auto-discover it. So it immediately gets available for the user, for both printing and scanning.

goipp is a Go library only needed to build ipp-usb, it does not need to be installed by the end user.

Both packages do not use debconf at all.

Both ipp-usb and goipp are maintained upstream very well and actively developed.

Upstream site:
https://github.com/OpenPrinting/ipp-usb
https://github.com/OpenPrinting/gipp

Recent commits:
https://github.com/OpenPrinting/ipp-usb/commits/master
https://github.com/OpenPrinting/goipp/commits/master

Bugs:
https://github.com/OpenPrinting/ipp-usb/issues
https://github.com/OpenPrinting/goipp/issues
(only closed ones in ipp-usb currently, none at all in goipp)

The author of both, Alexander Pevzner, is very responsive, usually answers on the same day. He is actively working on driverless scanning (will mentor 2 students in LFMP on IPP Scan in Sep-Nov).

No known bugs in Debian and Ubuntu (Launchpad only lists this MIR for both).

Debian maintainer OdyX also very responsive.

No exotic hardware required, is for supporting the absolute standard hardware, most modern printers and multi-function devices, even very cheap ones. See introduction above.

Both packages have debian/watch files and the content of the files seems to be correct.

No dependencies on obsolete stuff.

[Dependencies]

ipp-usb build-depends on goipp, therefore this MIR is for both. They both were developed together and there is (yet) no other consumer than ipp-usb for goipp.

Otherwise build-depends on usual Go stack.

Run-time dependencies are only Avahi and libusb which are all in Main.

Avahi needs to support localhost, but Ubuntu's Avahi does this already for some years.

[Standards compliance]

Packages fulfill the standards concerning FHS and Go. Both ipp-usb and goipp packages declare Debian's "Standards-Version" as 4.5.0, Debian maintainer takes care of verifying and updating this. debian/ directories and debian/rules files are simple, fairly standard.

[Maintenance]

See [Quality assurance] above for upstream and Debian maintenance. I have subscribed myself to bugs in both packages and also the Ubuntu Printing Team. Also "Desktop Packages" (~desktop-packages) is subscribed to sane-airscan.

[Background information]

See introduction above.

[Christian Ehrhardt  (paelzer)]

[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...

[Till Kamppeter (till-kamppeter)]

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

ipp-usb should get seeded.

[Till Kamppeter (till-kamppeter)]

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

[Ken VanDine (ken-vandine)]

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

[Till Kamppeter (till-kamppeter)]

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

[Till Kamppeter (till-kamppeter)]

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

[Till Kamppeter (till-kamppeter)]

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

[Christian Ehrhardt  (paelzer)]

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 ...

[Christian Ehrhardt  (paelzer)]

[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

[Didier Roche-Tolomelli (didrocks)]

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)

[Didier Roche-Tolomelli (didrocks)]

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)

[Till Kamppeter (till-kamppeter)]

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.

[Till Kamppeter (till-kamppeter)]

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.

[Till Kamppeter (till-kamppeter)]

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.

[Till Kamppeter (till-kamppeter)]

ippusbxd got removed from Debian now.

[Till Kamppeter (till-kamppeter)]

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

[Till Kamppeter (till-kamppeter)]

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
----------

[Christian Ehrhardt  (paelzer)]

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.

[Seth Arnold (seth-arnold)]

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

[Christian Ehrhardt  (paelzer)]

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?

[Steve Langasek (vorlon)]

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.

[Alexander Pevzner (pzz)]

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.

[Till Kamppeter (till-kamppeter)]

Alexander, thanks for your explanations.

[Seth Arnold (seth-arnold)]

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. :)

[Seth Arnold (seth-arnold)]

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.

[Seth Arnold (seth-arnold)]

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

[Christian Ehrhardt  (paelzer)]

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.

[Didier Roche-Tolomelli (didrocks)]

$ ./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.