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.
Created attachment 102077 [details] [review] [PATCH 1/4] Document <interface> in the dbus-daemon man page
Created attachment 102078 [details] [review] [PATCH 2/4] Add interface element to the bus config dtd
Created attachment 102079 [details] [review] [PATCH 3/4] Add interface element support to the bus config parsing
Created attachment 102080 [details] [review] [PATCH 4/4] bus driver: check whether the interface is enabled
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 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 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 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.
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.
(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.
(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.
(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.
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?
+ <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.
When the "Introspectable" typo is fixed, the patch works fine for me.
(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.
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
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 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 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.
Created attachment 107201 [details] [review] [PATCH 2/2] config: add examples to show how to enable/disable the Stats interface
(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. How we collect and use information is described in our Privacy Policy.