[MIR] cups-pk-helper

Things that caught my eye during review:

 - Uses dbus-glib, should be ported to gdbus at some point.
 - cups-pk-helper-mechanism.c is by and large boilerplate code and looks correct to me. The polkit result is properly checked, and there are no complicated memory operations.
- cups.c is rather large, but also pretty straightforward code to translate the dbus interface to the libcups API; it does some rather thorough input value validity checks.
- uses gettext properly
- Good maintenance in Debian so far, although the package is only half a year old; no bug reports
- No Ubuntu bugs, but not surprising as the package hasn't been used by default
- straightforward packaging

As Martin said, straightforward packaging, small code already in use in other distros, usage of different libs seems correct.
Seems there is no direct plan to port to gdbus (and 0.1.3 is out as well). Nothing risky from what I see in the code, so +1 from my side.
Need to fix the old fsf address while at it.

Bonus point having a team (I think the desktop one) subscribed to the bug reports :)
Martin, do you want to go on on the review, seeing if we really need it?

Bug #807261 seems quite important to fix though.

(setting as incomplete for now)

For oneiric we will use system-config-printer instead of the upstream g-c-c thing, so let's put this on hold until we need it again.

We're now shipping the GNOME System Settings>Printers panel in Raring. Without cups-pk-helper, you can't add a printer (bug 1082765).

ApportVersion: 2.6.2-0ubuntu5
Architecture: amd64
DistroRelease: Ubuntu 13.04
MarkForUpload: True
Package: cups-pk-helper 0.2.4-0ubuntu1
PackageArchitecture: amd64
ProcVersionSignature: Ubuntu 3.7.0-3.9-generic 3.7.0-rc6
Tags: raring
Uname: Linux 3.7.0-3-generic x86_64
UpgradeStatus: No upgrade log present (probably fresh install)
UserGroups: adm cdrom dialout dip libvirtd lpadmin plugdev sambashare sbuild sudo

apport information

apport information

The port to gdbus happened for 0.2.0.

Didier said that the MIR is approved and that we can add the recommends to g-c-c, we just need to ping him for promotion after upload

This has a CVE history: CVE-2012-4510. See http://www.debian.org/security/2012/dsa-2562 (it is still unfixed in Ubuntu 12.10 and lower, so it hasn't gotten community attention in Ubuntu yet).

Thanks Jamie, the issue is fixed in raring it seems ... do you consider the fact that it's not in older serie as an issue for the MIR request?

One of the criteria for main inclusion is how well it has been maintained in the past in Ubuntu. Another is if there is a security history, then it needs a security review.

Ok, I'm a bit unsure where we stand...

It was not very well looked at since it was in universe, once promoted the desktop team will look at it and make sure fixes land in a reasonable timeframe.

The codebase is small and the package is shipped in almost all the other distros and maintained upstream well enough.

Those are the things I can comment on. How do we process next? We will want a security team review, as you pointed, and then how do we conclude or an ack or nack decision?

A member of the security team will review it and report back.

FYI, a fix for CVE-2012-4510 (bug #1083416) is available in proposed, but no one has verified it yet. Since part of consideration for main is how well is package has been maintained in Ubuntu to date, can someone test those?

@Jamie: desktop will do the verification, it's friday night though so maybe not before monday...

(Subscribing Till)

Hey Till, is there any chance you could help testing that cups-pk-helper security update?

seb128, I am running said version of cups-pk-helper for some weeks under Quantal and it did not cause any regressions for me. Marked the CVE bug verified.

Thanks!

@Till, fyi, this was only uploaded a week ago to quantal-proposed. We also need testers for oneiric and precise.

Security review:
With a shallow code audit, the code seems ok and is quite small. It (obviously) ships a dbus system service. The recent fixes for the CVE look ok. Upstream reported the issue responsibly, provided patches and was receptive to feedback on the patches. The package is a root running application that processes input from the user session. It would be good if this had full hardening options during compilation. The policykit policy looks ok (note that we ship polkit overrides that allow all actions to anyone in the lpadmin, sudo or admin groups if they have an active session-- this is ok). The packaging looks fine. It looks like there are some test files that could be run (in src/test-*).

Conditional ACK provided we compile with PIE and BIND_NOW and try to get the testsuite going during build.

Added the hardening to the cups-pk-helper package and checked whether system-config-printer still works correctly with it which is the case. Made build system more verbose to check whether hardening options got really applied on compiling and linking, which is also the case.

Checked also the test programs. They do not serve for an automated build by the build server's system user for build processes. The test programs are based on print queues with fixed given names and they also should do their job only without asking for a password in some form if the calling user has appropriate admin privileges. Therefore I did not call them during the build process.

Till Kamppeter [2012-12-10 19:11 -0000]:
> Added the hardening to the cups-pk-helper package and checked whether
> system-config-printer still works correctly with it which is the case.
> Made build system more verbose to check whether hardening options got
> really applied on compiling and linking, which is also the case.

Thanks!

> Checked also the test programs. They do not serve for an automated build
> by the build server's system user for build processes. The test programs
> are based on print queues with fixed given names and they also should do
> their job only without asking for a password in some form if the calling
> user has appropriate admin privileges. Therefore I did not call them
> during the build process.

These seem more suitable as an autopkgtest then, where you can
configure a user as needed and have root privs for the tests?

Based on Till's feedback and looking at 0.2.4-0ubuntu4, this meets the conditional ACK.

Moved to main.