Bug 93036 - /uid-permissions/uae/root test fails with 1.10.4
Summary: /uid-permissions/uae/root test fails with 1.10.4
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.10
Hardware: Other All
: medium normal
Assignee: Simon McVittie
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review? 1.10
Keywords: patch, regression
Depends on:
Blocks:
 
Reported: 2015-11-20 10:50 UTC by Iain Lane
Modified: 2015-11-24 01:23 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
uid-permissions test: don't assert that root can UpdateActivationEnvironment (4.10 KB, patch)
2015-11-20 17:20 UTC, Simon McVittie
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Iain Lane 2015-11-20 10:50:52 UTC
We run Debian's autopkgtests, which contain this test, as part of our upload gating process in Ubuntu. This caught the fact that the test fails. I reproduced it in a clean (enough) unstable chroot against Debian's dbus package.

(sid-amd64)root@nightingale:/dbus-1.10.4# DBUS_TEST_DATA=/usr/lib/dbus-1.0/installed-tests/dbus/data /usr/lib/dbus-1.0/installed-tests/dbus/test-uid-permissions 
/uid-permissions/uae/root: **
ERROR:../../test/uid-permissions.c:129:test_uae: assertion failed (dbus_message_get_type (m) == DBUS_MESSAGE_TYPE_METHOD_RETURN): (3 == 2)

I suppose it's related to the UAE change in 1.10.4, but I didn't try reverting the patch to verify.
Comment 1 Simon McVittie 2015-11-20 17:06:24 UTC
Unfortunately I don't routinely run the tests as root for the upstream release process, I don't currently have a pre-upload autopkgtest environment, and ci.debian.net is sufficiently overloaded that dbus only gets tested once a month. Hopefully I'll eventually set up some sort of buildbot that runs autopkgtest on git master and the stable branch, although to do that, the Debian packaging might need some adjustment.

Yes, the regression is that the test relies on being able to call UpdateActivation as root or messagebus on a bus where we have an activation helper, which isn't useful (because the activation helper discards its environment anyway) and is no longer allowed since 1.10.4.

We'll need to change the test to assert that it fails as nobody, but not assert that it succeeds as root or messagebus. We could use Monitoring.BecomeMonitor() as the function that we assert is allowed for root.

The reason that multi-user.conf sets an activation helper (it's /bin/false) is that I wanted to rule out the possibility of arbitrary code execution (via activation) on machines where that test is run.

I've considered adding a command line option to disable traditional activation (so all activation must go via systemd, which we can control). That would address that issue.
Comment 2 Simon McVittie 2015-11-20 17:20:32 UTC
Created attachment 119997 [details] [review]
uid-permissions test: don't assert that root can  UpdateActivationEnvironment

Since 1.10.4 this is hard-coded to be disallowed when an activation
helper is used. That would be a security flaw waiting to happen,
and makes little sense anyway, because the activation helper sanitises
its environment.

Use BecomeMonitor() instead, as our way to assert that root and
messagebus are privileged.
Comment 3 Iain Lane 2015-11-23 13:16:22 UTC
Comment on attachment 119997 [details] [review]
uid-permissions test: don't assert that root can  UpdateActivationEnvironment

Review of attachment 119997 [details] [review]:
-----------------------------------------------------------------

It works for me and seems sensible when comparing it to test_uae (although I'm not very familiar with the code). The two tests have a fair bit in common, might be worth factoring some bits out, but not that important.

Thanks, I'm going to upload this to Ubuntu.
Comment 4 Simon McVittie 2015-11-24 01:23:54 UTC
Fixed in git for 1.10.6 and 1.11.0, thanks.

(In reply to Iain Lane from comment #3)
> The two tests have a fair bit in
> common, might be worth factoring some bits out, but not that important.

I'd be happy to review a patch (also for any other test coverage or simplicity improvements).


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.