[MIR] volume-key

[Jeremy Bícha]

Availability
============
Built for all supported architectures. In sync with Debian.

Rationale
=========
GNOME Disks uses udisks2. Debian's udisks2 recommends libblockdev-crypto2 which depends on libvolume-key1.

The package description for libblockdev-crypto2 is:
 "The libblockdev library plugin (and in the same time a standalone library)
  providing the functionality related to encrypted devices (LUKS)."

This sounds like a very useful feature for Ubuntu since we offer full disk encryption using LUKS.

Security
========
No known security issues. Presumably should get a Security review.

https://security-tracker.debian.org/tracker/source-package/volume-key
https://launchpad.net/ubuntu/+source/volume-key/+cve

Quality assurance
=================
- Please subscribe Ubuntu Desktop bugs (although the Desktop Team thinks that Foundations should be responsible for udisks and friends)

https://bugs.launchpad.net/ubuntu/+source/volume-key
https://bugs.debian.org/cgi-bin/pkgreport.cgi?src=volume-key
https://pagure.io/volume_key/issues

dh_auto_test is run but tests are failing and ignored.

No autopkgtests

Dependencies
============
No universe dependencies

Standards compliance
====================
4.1.3, debhelper compat 11, simple dh7 style rules

Maintenance
===========
Maintained in Debian by the Debian Utopia team, which is a small team focused on cross-desktop freedesktop.org stuff.

https://salsa.debian.org/utopia-team/volume-key

upstream:
https://pagure.io/volume_key

[Mathieu Trudel-Lapierre (cyphermox)]

Please fix:
- Missing bug subscriber
- Package contains a limited test suite, which gets run at build time, but failures are ignored (and it does happen to fail)
- crypto/secrets handling should have a security review

[Iain Lane (laney)]

ok it's a bit more urgent than we thought --- AIUI this not being pulled in by udisks2 is causing bug #1757321 which is a failure to unlock encrypted usb devices. could the security team please review?

I'll add the subscribers now.

[Seth Arnold (seth-arnold)]

The tests run during the build failed on my machine yet the build did not fail:

Error creating passphrase-encrypted packet: Unknown error getting encryption result
FAIL tests/packet_roundtrips.sh (exit status: 1)

============================================================================
Testsuite summary for volume_key 0.3.9
============================================================================
# TOTAL: 1
# PASS: 0
# SKIP: 0
# XFAIL: 0
# FAIL: 1
# XPASS: 0
# ERROR: 0
============================================================================
See ./test-suite.log
Please report to <email address hidden>
============================================================================
Makefile:1328: recipe for target 'test-suite.log' failed
make[5]: *** [test-suite.log] Error 1
make[5]: Leaving directory '/<<PKGBUILDDIR>>'
Makefile:1434: recipe for target 'check-TESTS' failed
make[4]: *** [check-TESTS] Error 2
make[4]: Leaving directory '/<<PKGBUILDDIR>>'
Makefile:1664: recipe for target 'check-am' failed
make[3]: *** [check-am] Error 2
make[3]: Leaving directory '/<<PKGBUILDDIR>>'
Makefile:1214: recipe for target 'check-recursive' failed
make[2]: *** [check-recursive] Error 1
make[2]: Leaving directory '/<<PKGBUILDDIR>>'
dh_auto_test: make -j4 check VERBOSE=1 returned exit code 2
make[1]: Leaving directory '/<<PKGBUILDDIR>>'

- Why did this not fail the build?
- Why did this fail?

Thanks

[Jeremy Bícha (jbicha)]

- Why did this not fail the build?

Because then the build wouldn't work! I mentioned this in the MIR description under Quality Assurance.

- Why did this fail?

I don't know. Once we know how to get the tests to pass, I'll be happy to make test failures fail the build. (Currently, they're ignored by the override_dh_auto_test rule.)

[Jeremy Bícha (jbicha)]

With this commit, I now get this error:

"Error creating passphrase-encrypted packet: GPGME: Bad passphrase"

https://salsa.debian.org/utopia-team/volume-key/commit/42207b3d

[Mathieu Trudel-Lapierre (cyphermox)]

Wild guess is this is a test that expects user input, and the input never shows up?

[Iain Lane (laney)]

can you be extra clear with us please - are you saying that this test needs to be made to pass or we need to explain why it doesn't or what?

Are you planning to perform a security review?

[Jeremy Bícha (jbicha)]

I reported the test failures upstream several days ago, but I haven't gotten a response yet.

https://pagure.io/volume_key/issue/15

[Seth Arnold (seth-arnold)]

The security review is underway. I'm sad that tracking this test case failure down is as complicated as it is -- the code feels overly-generic, tracking call chains through to where anything is *done* is harder than other code bases. Each individual line of code looks fine but getting a handle on the big picture is difficult.

Thanks

[Seth Arnold (seth-arnold)]

I reviewed volume-key version 0.3.9-3 as checked into bionic. This should
not be considered a full security audit but rather a quick gauge of
maintainability.

- No CVEs in our database.
- volume-key's main purpose is to provide some key escrow capabilities for
  encrypted storage

- Build-Depends: debhelper, libglib2.0-dev, libcryptsetup-dev,
  libnss3-dev, libgpgme11-dev, libblkid-dev, swig, python-dev,
  libnss3-tools
- Does not daemonize
- No networking
- Does Cryptography
- No pre/post inst/rm
- No init scripts
- No systemd unit files
- No dbus services
- No setuid files
- volume_key in PATH
- No sudo fragments
- No udev rules
- There is a test suite but it doesn't appear useful as a quality tool
- No cron jobs
- Some warnings in the build logs, not ideal

- No subprocesses spawned
- I found some probable errors in memory management, but mostly good:
  - kmip_decode_object_symmetric_key() return -1 case leaks res?
  - kmip_decode_key_value() default: case leaks res?
  - kmip_decode_object_secret_data() return -1 case leaks res?
- Files opened are controlled by the user
- Logging looked careful
- No privileged operations
- Extensive cryptographic operations
- No networking
- No privileged portions of code
- No temp files
- No WebKit
- No JavaScript
- Clean cppcheck
- No PolicyKit

I don't like promoting this package to main already. The tests shouldn't
be failing in a brand-new project. The fact that nss's certutil's use of
UpdateRNG() does a bunch of garbage with the terminal and prints lies
about what it is doing suggests that certutil itself is not suitable for
use by this project:

https://sources.debian.org/src/nss/2:3.35-2/nss/cmd/certutil/keystuff.c/?hl=67#L67

I'd be much happier promoting volume-key for 18.10.

However, we've already gotten complaints from our users that their
encrypted storage no longer works because the old mechanism has apparently
already been torn down.

If there's no way to bring back the old mechanism, then..

Security team begrudging ACK for promoting volume-key to main. But I'd be
happier if we could just bring back what used to work.

Thanks

[Jeremy Bícha (jbicha)]

> The tests shouldn't be failing in a brand-new project.

Sadly, the project is not new at all (although it's only in Debian now because of udisks2). The current release packaged in Ubuntu, 0.3.9, was released in 2012.

https://pagure.io/volume_key/commits/master

[Jeremy Bícha (jbicha)]

I think we're just waiting on final approval from cyphermox now.

[Mathieu Trudel-Lapierre (cyphermox)]

Have the tests been fixed? I really don't much like having things in main that run tests but don't use the result; is there any way to just ignore the test(s) that are really broken and otherwise keep the remaining tests failing the build if they fail, such that we can catch a possible regression?

[Iain Lane (laney)]

gpgme_set_pinentry_mode (ctx, GPGME_PINENTRY_MODE_LOOPBACK) fixes this, apparently this is required with newer gpg, not sure where the right place to put this is tho

[Iain Lane (laney)]

nein, it still fails even with this from a package build, but works when you chroot in and run the test :(

[Iain Lane (laney)]

ok, just needed to be run as root, I asked jibel if he could test my proposed package in ppa:laney/ppa, just waiting to hear back

[Iain Lane (laney)]

umm, that's nonsense, try "just needed to have a writable home directory"

[Iain Lane (laney)]

volume-key_0.3.9-4 is uploaded to unstable. Although I forgot to say it in the changelog, it runs the tests fatally.

I'd still like some confirmation that my patch is OK, but assuming it is - it could be synced.

cyphermox - any chance you could review it pre-upload and approve the MIR on the basis that it is this version that is promoted?

[Iain Lane (laney)]

it's synced now, so never mind "pre-upload"

[Mathieu Trudel-Lapierre (cyphermox)]

MIR approved.

The new version synced from debian in bionic (UNAPPROVED) looks good now, and appears to have corrected test suite issues:

18057438 | X- | volume-key | 0.3.9-4 | 6 hours
  | * volume-key/0.3.9-4 Component: universe Section: misc

[Steve Langasek (vorlon)]

Override component to main
volume-key 0.3.9-4 in bionic amd64: universe/utils/optional/100% -> main
volume-key 0.3.9-4 in bionic arm64: universe/utils/optional/100% -> main
volume-key 0.3.9-4 in bionic armhf: universe/utils/optional/100% -> main
volume-key 0.3.9-4 in bionic i386: universe/utils/optional/100% -> main
volume-key 0.3.9-4 in bionic ppc64el: universe/utils/optional/100% -> main
volume-key 0.3.9-4 in bionic s390x: universe/utils/optional/100% -> main
6 publications overridden.

[Adam Conrad (adconrad)]

Promoted with slightly more gusto:

Override component to main
volume-key 0.3.9-4 in bionic: universe/misc -> main
libvolume-key-dev 0.3.9-4 in bionic amd64: universe/libdevel/optional/100% -> main
libvolume-key-dev 0.3.9-4 in bionic arm64: universe/libdevel/optional/100% -> main
libvolume-key-dev 0.3.9-4 in bionic armhf: universe/libdevel/optional/100% -> main
libvolume-key-dev 0.3.9-4 in bionic i386: universe/libdevel/optional/100% -> main
libvolume-key-dev 0.3.9-4 in bionic ppc64el: universe/libdevel/optional/100% -> main
libvolume-key-dev 0.3.9-4 in bionic s390x: universe/libdevel/optional/100% -> main
libvolume-key1 0.3.9-4 in bionic amd64: universe/libs/optional/100% -> main
libvolume-key1 0.3.9-4 in bionic arm64: universe/libs/optional/100% -> main
libvolume-key1 0.3.9-4 in bionic armhf: universe/libs/optional/100% -> main
libvolume-key1 0.3.9-4 in bionic i386: universe/libs/optional/100% -> main
libvolume-key1 0.3.9-4 in bionic ppc64el: universe/libs/optional/100% -> main
libvolume-key1 0.3.9-4 in bionic s390x: universe/libs/optional/100% -> main
volume-key 0.3.9-4 in bionic amd64: main/utils/optional/100% -> main
volume-key 0.3.9-4 in bionic arm64: main/utils/optional/100% -> main
volume-key 0.3.9-4 in bionic armhf: main/utils/optional/100% -> main
volume-key 0.3.9-4 in bionic i386: main/utils/optional/100% -> main
volume-key 0.3.9-4 in bionic ppc64el: main/utils/optional/100% -> main
volume-key 0.3.9-4 in bionic s390x: main/utils/optional/100% -> main
Override [y|N]? y
volume-key 0.3.9-4 in bionic amd64 remained the same
volume-key 0.3.9-4 in bionic arm64 remained the same
volume-key 0.3.9-4 in bionic armhf remained the same
volume-key 0.3.9-4 in bionic i386 remained the same
volume-key 0.3.9-4 in bionic ppc64el remained the same
volume-key 0.3.9-4 in bionic s390x remained the same
13 publications overridden; 6 publications remained the same.