Bug 80759

Summary: Enable/disable the Stats interface without recompiling
Product: dbus Reporter: Alban Crequy <alban.crequy>
Component: coreAssignee: Simon McVittie <smcv>
Status: RESOLVED FIXED QA Contact: D-Bus Maintainers <dbus>
Severity: enhancement    
Priority: medium CC: msniko14, smcv, thiago, walters
Version: 1.5Keywords: patch
Hardware: All   
OS: All   
Whiteboard: review?
i915 platform: i915 features:
Attachments: [PATCH 1/4] Document <interface> in the dbus-daemon man page
[PATCH 2/4] Add interface element to the bus config dtd
[PATCH 3/4] Add interface element support to the bus config parsing
[PATCH 4/4] bus driver: check whether the interface is enabled
[untested] Enable Stats interface by default; disallow non-root use on system bus
[PATCH 1/2] Enable Stats interface by default; disallow non-root use on system bus
[PATCH 2/2] config: add examples to show how to enable/disable the Stats interface
[PATCH 2/2] config: add examples to show how to enable/disable the Stats interface

Description Alban Crequy 2014-07-01 16:01:28 UTC
Dbus-daemon implements several interfaces (see Bug #33757). Some of them are always enabled:
- org.freedesktop.DBus
- org.freedesktop.DBus.Introspectable

But another one is not and it depends on a compilation flag:
- org.freedesktop.DBus.Debug.Stats (--enable-stats)

--enable-stats is not used on production systems because of fears it could introduce security issues. I am also trying to add more things in this interface: see Bug #24307.

In order to write test cases on applications without requiring testers to recompile dbus-daemon, I would like to be able to enable and disable this interface by configuration.

The following patches implement the possibility to add this in the configuration to enable the Stats interface at run-time:
  <interface>org.freedesktop.DBus.Debug.Stats</interface>

Then, testers can change the configuration and use SIGHUP.
Comment 1 Alban Crequy 2014-07-01 16:06:11 UTC
Created attachment 102077 [details] [review]
[PATCH 1/4] Document <interface> in the dbus-daemon man page
Comment 2 Alban Crequy 2014-07-01 16:06:43 UTC
Created attachment 102078 [details] [review]
[PATCH 2/4] Add interface element to the bus config dtd
Comment 3 Alban Crequy 2014-07-01 16:07:16 UTC
Created attachment 102079 [details] [review]
[PATCH 3/4] Add interface element support to the bus config parsing
Comment 4 Alban Crequy 2014-07-01 16:07:51 UTC
Created attachment 102080 [details] [review]
[PATCH 4/4] bus driver: check whether the interface is enabled
Comment 5 Thiago Macieira 2014-07-01 17:27:23 UTC
Comment on attachment 102077 [details] [review]
[PATCH 1/4] Document <interface> in the dbus-daemon man page

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

Looks good.
Comment 6 Thiago Macieira 2014-07-01 17:27:31 UTC
Comment on attachment 102078 [details] [review]
[PATCH 2/4] Add interface element to the bus config dtd

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

Looks good.
Comment 7 Thiago Macieira 2014-07-01 17:29:00 UTC
Comment on attachment 102079 [details] [review]
[PATCH 3/4] Add interface element support to the bus config parsing

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

Looks correct, but I'm not entirely sure about the XML parsing (not familiar anymore with the library).
Comment 8 Thiago Macieira 2014-07-01 17:30:14 UTC
Comment on attachment 102080 [details] [review]
[PATCH 4/4] bus driver: check whether the interface is enabled

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

Looks good.

I see now why you chose to implement it with an <interface> element. The code looks simpler with that.
Comment 9 Simon McVittie 2014-09-04 15:38:12 UTC
The implementation looks fine in principle, but I wonder whether we'd be better off using D-Bus' existing support for allowing/denying method calls instead:

     <!-- Allow anyone to talk to the message bus -->
-    <allow send_destination="org.freedesktop.DBus"/>
+    <allow send_destination="org.freedesktop.DBus"
+          send_interface="org.freedesktop.DBus"/>
     <!-- But disallow some specific bus services -->
     <deny send_destination="org.freedesktop.DBus"
           send_interface="org.freedesktop.DBus.Debug.Stats"/>
     <deny send_destination="org.freedesktop.DBus"
           send_interface="org.freedesktop.DBus"
           send_member="UpdateActivationEnvironment"/>

and further down the file:

+  <!-- uncomment, or add to a file in system.d, to let root
+       debug the system bus:
+  <policy user="root">
+    <allow send_destination="org.freedesktop.DBus"
+          send_interface="org.freedesktop.DBus.Debug.Stats"/>
+  </policy>
+  -->

I'm curious how much compiled size the Stats interface adds.
Comment 10 Alban Crequy 2014-09-05 12:46:49 UTC
(In reply to comment #9)
> The implementation looks fine in principle, but I wonder whether we'd be
> better off using D-Bus' existing support for allowing/denying method calls
> instead:

As long as it can be enabled / disabled in the configuration file and SIGHUP, I am fine with it.

> I'm curious how much compiled size the Stats interface adds.

On x86_64:

./configure && make
bus/dbus-daemon 1874194

./configure --enable-stats && make
bus/dbus-daemon 1891232

So the difference is ~17KB.

And while running, it will use a bit more memory:
- 6 * sizeof(int) in struct BusConnections, used only 1 time
- 2 * sizeof(long) in struct DBusCounter, used in DBusConnection and DBusTransport

It seems very reasonable to me.
Comment 11 Simon McVittie 2014-09-05 14:03:35 UTC
(In reply to comment #10)
> So the difference is ~17KB.

I think that's a sufficiently small difference that we should just mask it off with <deny/> rules in the default system bus configuration file, and consider compile-time-enabling it by default.
Comment 12 Alban Crequy 2014-09-05 16:38:22 UTC
(In reply to comment #10)
> (In reply to comment #9)
> > The implementation looks fine in principle, but I wonder whether we'd be
> > better off using D-Bus' existing support for allowing/denying method calls
> > instead:
> 
> As long as it can be enabled / disabled in the configuration file and
> SIGHUP, I am fine with it.

I tested the changes in Comment #9 and changing the policy + SIGHUP worked fine.

(In reply to comment #11)
> (In reply to comment #10)
> > So the difference is ~17KB.
> 
> I think that's a sufficiently small difference that we should just mask it
> off with <deny/> rules in the default system bus configuration file, and
> consider compile-time-enabling it by default.

I agree. Do you have a patch ready or do you want me to write one?

I wonder about the user root:
+  <policy user="root">
Since I would like d-feet to use it:
https://bugzilla.gnome.org/show_bug.cgi?id=735167
and d-feet should not run as root, does it worth adding a comment to say to replace "root" by another user?

I wonder if there is an easy way to temporarily grant an user to access the interface, for a better integration in d-feet. gksudo does not help here.
Comment 13 Simon McVittie 2014-09-05 17:55:50 UTC
Created attachment 105819 [details] [review]
[untested] Enable Stats interface by default; disallow non-root use on  system bus

---

I was planning to test this in a VM at some point, but if you get there first, so much the better! :-)

> does it worth adding a comment to say to
> replace "root" by another user?

Already done, in fact.

> I wonder if there is an easy way to temporarily grant an user to access the
> interface, for a better integration in d-feet. gksudo does not help here.

I think the way to do it is "developers should enable it for themselves".

We could ship a sample file for system.d in /usr/share/doc/dbus/examples/enable-stats.conf?
Comment 14 Alban Crequy 2014-09-08 09:59:34 UTC
+    <allow send_destination="org.freedesktop.DBus"
+           send_interface="org.freedesktop.DBus.Introspection"/>

It should be "org.freedesktop.DBus.Introspectable" instead.

Otherwise, the patch looks good. But I will test it a bit more...

(In reply to comment #13)
> We could ship a sample file for system.d in
> /usr/share/doc/dbus/examples/enable-stats.conf?

Yes.

And I ponder about a sample file for session.d to disable stats: distributions that want to isolate apps with SELinux or AppArmor would reuse it.
Comment 15 Alban Crequy 2014-09-08 10:09:18 UTC
When the "Introspectable" typo is fixed, the patch works fine for me.
Comment 16 Simon McVittie 2014-09-08 10:35:26 UTC
(In reply to comment #14)
> And I ponder about a sample file for session.d to disable stats:
> distributions that want to isolate apps with SELinux or AppArmor would reuse
> it.

Good idea.
Comment 17 Alban Crequy 2014-09-26 10:57:23 UTC
Created attachment 106917 [details] [review]
[PATCH 1/2] Enable Stats interface by default; disallow non-root use on system bus

smcv's patch modified to fix typo in interface name
Comment 18 Alban Crequy 2014-09-26 10:59:16 UTC
Created attachment 106918 [details] [review]
[PATCH 2/2] config: add examples to show how to enable/disable the Stats interface

make distcheck pass, and @EXPANDED_SYSCONFDIR@ works, but not fully tested.
Comment 19 Simon McVittie 2014-10-01 15:26:02 UTC
Comment on attachment 106917 [details] [review]
[PATCH 1/2] Enable Stats interface by default; disallow non-root use on system bus

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

I still need to test this system-wide, but it looks good
Comment 20 Simon McVittie 2014-10-01 15:28:04 UTC
Comment on attachment 106918 [details] [review]
[PATCH 2/2] config: add examples to show how to enable/disable the Stats interface

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

Mostly looks good.

::: bus/Makefile.am
@@ +42,4 @@
>  config_DATA += system.conf
>  endif
>  
> +doc_DATA = \

If you want this to be installed in $(docdir)/examples you need to use:

exampledir = $(docdir)/examples
example_DATA = \
...

@@ +215,4 @@
>  install-data-hook:
>  	$(mkinstalldirs) $(DESTDIR)$(configdir)/session.d
>  	$(mkinstalldirs) $(DESTDIR)$(datadir)/dbus-1/services
> +	$(mkinstalldirs) $(DESTDIR)$(docdir)/examples

This should be unnecessary if you do what I said above.
Comment 21 Alban Crequy 2014-10-01 17:05:12 UTC
Created attachment 107201 [details] [review]
[PATCH 2/2] config: add examples to show how to enable/disable the Stats interface
Comment 22 Simon McVittie 2014-10-01 18:02:04 UTC
(In reply to comment #20)
> If you want this to be installed in $(docdir)/examples you need to use:
...
> This should be unnecessary if you do what I said above.

Fixed both, tested with and without the configuration snippets, and pushed it. Fixed in git for 1.9.0, thanks!

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct.