Created attachment 266933 [details] [review]
remove 'nonsense' <standard_session_servicedirs/>

Created attachment 266935 [details] [review]
Use well known session configuration

I am not sure which of these two is preferable (if either are) but here is another possibility.

Due bug 728319, I realized this bug, and their patches. Although we would need for Mike Gorse final review, some comments.

Review of attachment 266933 [details] [review]:

Yes I agree with you, that taking into account the documentation of this option, it doesn't makes sense on the configuration file, and I think that it would be safe to commit.

Review of attachment 266935 [details] [review]:

This one is somewhat more tricky. Comments below.

::: bus/accessibility.conf
@@ +2,3 @@
 <busconfig>
 
+  <type>session</type>

Im not really sure about this change. at-spi2 is not using the session dbus, but using an own bus[1], the accessibility bus. And at the same time, I find the dbus documentation for type somewhat confusing:
"The well-known type of the message bus. Currently known values are "system" and "session""

What means exactly well-know? well-known by dbus-daemon? Well-known on linux environments?

Right now I see two options:
  a) accessibility is another "well-known" type
  b) what we have here is a custom accessibility bus of type session

Right now Im more biased to b) (as you suggested), but I think that it is worth asking. I created a bug on freedesktop suggesting to add accessibility as a well-known bus, just in case is a), or asking for further details. If it doesn't get answer, I will ping via IRC.

[1] https://git.gnome.org/browse/at-spi2-core/tree/README#n93

@@ -12,3 @@
   <policy context="default">
-    <!-- Allow root to connect -->
-    <allow user="root"/>

I think that this is correct as it is. This was changed on bug 643110. Before that bug, any user could connect to the bus. It was changed to allow root to connect. Note that this doesn't means that anyone connecting to the bus would get root privileges, means that root can connect to it.

Review of attachment 266933 [details] [review]:

(I am a D-Bus maintainer, not an AT-SPI maintainer)

Having <standard_session_servicedirs/> in the accessibility bus's configuration does seem a bad idea. If this is present, and someone connects to the accessibility bus and sends a message to an activatable service listed in /usr/share/dbus-1/services, then that service will be started, and will think that the accessibility bus is its session bus.

(Picking a random example: the GNOME Contacts app, org.gnome.Contacts.service, could be started like that.)

If the accessibility bus is meant to have a more limited set of activation directories - perhaps ${datadir}/dbus-1/accessibility-services or something - then it will need to list them explicitly.

Is activation a desired feature on the accessibility bus?

(In reply to comment #5)
> What means exactly well-know? well-known by dbus-daemon? Well-known on linux
> environments?

"Documented in the D-Bus Specification".

> Right now I see two options:
>   a) accessibility is another "well-known" type

It almost is, but it isn't documented in the D-Bus Specification. You could document it in the D-Bus Specification if you want it to become "well-known".

>   b) what we have here is a custom accessibility bus of type session

No, this isn't a session bus. It's parallel to the real session bus, and (some) applications are expected to connect to both.

There is a third option:

c) don't have a <type> at all

I haven't tested this, but the dbus-daemon appears to support it.

The only practical effects of the <type> at the moment are:

* activated services get DBUS_STARTER_BUS_TYPE=accessibility in their
  environment

* if the type is exactly "system" or "session", it shows up in syslog
  message prefixes

(In reply to comment #5)
> Im not really sure about this change. at-spi2 is not using the session dbus,
> but using an own bus[1], the accessibility bus.

Is this considered to be an API, or an implementation detail of AT-SPI2?

(In reply to comment #0)
> Could using the session bus instead of accessibility not deal with the security
> quite nicely?

No:

        The accessibility bus is separate from the session bus
        because it may in fact span user sessions if a user, for instance,
        runs an application that escalates to run as root. The accessibility
        bus is thus tied to the X session rather than the D-Bus session.

  -- https://git.gnome.org/browse/at-spi2-core/tree/README#n93

The security model appears to be:

* session bus: same uid that ran dbus-daemon may connect and do stuff
  (this is always assumed to be true, without needing configuration)

* accessibility bus: either root or the same uid that ran dbus-daemon
  may connect and do stuff, to support traditional bad ideas involving
  running GUIs as root, such as gksu(1)

(In reply to comment #6)
> Review of attachment 266933 [details] [review]:
> 
> (I am a D-Bus maintainer, not an AT-SPI maintainer)

Thanks for your feedback.

> Having <standard_session_servicedirs/> in the accessibility bus's configuration
> does seem a bad idea. If this is present, and someone connects to the
> accessibility bus and sends a message to an activatable service listed in
> /usr/share/dbus-1/services, then that service will be started, and will think
> that the accessibility bus is its session bus.

Ok, then it is clear that this patch makes sense.

> (Picking a random example: the GNOME Contacts app, org.gnome.Contacts.service,
> could be started like that.)
> 
> If the accessibility bus is meant to have a more limited set of activation
> directories - perhaps ${datadir}/dbus-1/accessibility-services or something -
> then it will need to list them explicitly.
> 
> Is activation a desired feature on the accessibility bus?

Yes, clearly. Historically this has been tricky, as the accessibility services needed to be available before any app we want to be accessible. In at-spi the daemon taking care of the bus launching is called at-spi-bus-launcher. It is briefly explained on bus/README [1]. Anyway, the need of having both at-spi2-registryd and at-spi-bus-launcher is not clear. Take a look to this comment [2], from the bug that created at-spi-bus-launcher

[1] https://git.gnome.org/browse/at-spi2-core/tree/bus/README
[2] https://bugzilla.gnome.org/show_bug.cgi?id=644851#c15

(In reply to comment #7)
> (In reply to comment #5)
> > What means exactly well-know? well-known by dbus-daemon? Well-known on linux
> > environments?
> 
> "Documented in the D-Bus Specification".

Ok. Thanks for the explanation.


> > Right now I see two options:
> >   a) accessibility is another "well-known" type

> c) don't have a <type> at all

So that lets a) and c) as the only possible options. Based on the info that you have right now, what would you recommend?

(In reply to comment #10)
> > Is activation a desired feature on the accessibility bus?
> 
> Yes, clearly. Historically this has been tricky, as the accessibility services
> needed to be available before any app we want to be accessible. In at-spi the
> daemon taking care of the bus launching is called at-spi-bus-launcher.

Note that I am not asking about:

* app A wants to use accessibility
* app A sends some request on the session bus
* `dbus-daemon --session` runs at-spi-bus-launcher and gives it the request
* at-spi-bus-launcher ensures that an accessibility bus is running

That's just ordinary session bus activation.

What I am asking about is:

* app A wants to use accessibility
* app A does whatever is necessary to get an accessibility bus
* app A connects to the accessibility bus
* app A wants accessibility component B
* app A sends some request on the *accessibility* bus
* the accessibility dbus-daemon runs accessibility component B
  and gives it the request

If there is such a situation - is there? - then accessibility-component-b.service needs to be in a directory that is on the accessibility bus's configured search path, which is currently $XDG_DATA_DIRS/dbus-1/services (the same as for the session bus), but should be something more like ${datadir}/dbus-1/accessibility-services or something.

That would be a compatibility break which needs to be coordinated between the package that ships /etc/at-spi2/accessibility.conf, and all packages that want to be activatable on the accessibility bus.

From what's on my Debian system, it seems that org.a11y.Bus.service (at-spi-bus-launcher) wants to be an activatable session service (only). It is not clear to me whether org.a11y.atspi.Registry (at-spi2-registryd) is intended to be activatable on the session bus or the accessibility bus.

At the moment both are activatable on both, which is clearly wrong - I assume you're only meant to have 0 or 1 copies of each.

(In reply to comment #11)
> > c) don't have a <type> at all
> 
> So that lets a) and c) as the only possible options. Based on the info that you
> have right now, what would you recommend?

I would recommend testing what happens with no <type> at all. If it works, do that.

(In reply to comment #12)

> That would be a compatibility break which needs to be coordinated between the
> package that ships /etc/at-spi2/accessibility.conf, and all packages that want
> to be activatable on the accessibility bus.

Not really; at-spi2-registryd is the only program that activates on the accessibility bus.

> From what's on my Debian system, it seems that org.a11y.Bus.service
> (at-spi-bus-launcher) wants to be an activatable session service (only). It is
> not clear to me whether org.a11y.atspi.Registry (at-spi2-registryd) is intended
> to be activatable on the session bus or the accessibility bus.
> 
> At the moment both are activatable on both, which is clearly wrong - I assume
> you're only meant to have 0 or 1 copies of each.

Yeah, what you were suggesting there makes sense to me (ie, installing org.a11y.atspi.Registry.service into a different directory).

Review of attachment 266933 [details] [review]:

Ok thanks for the feedback. I have now committed this since it was agreed and accepted. Maybe a good idea to keep this bug open while we test out the other points highlighted in the discussion, though.

https://git.gnome.org/browse/at-spi2-core/commit/?id=1dc6d14047d887d337d6f169ace6cd3a726d5729

(In reply to comment #14)
> (In reply to comment #12)
> > That would be a compatibility break which needs to be coordinated between the
> > package that ships /etc/at-spi2/accessibility.conf, and all packages that want
> > to be activatable on the accessibility bus.
> 
> Not really; at-spi2-registryd is the only program that activates on the
> accessibility bus.

Excellent. Then it needs to be coordinated between AT-SPI2, and, er, itself. Should be easy :-)

(In reply to comment #15)
> Ok thanks for the feedback. I have now committed this since it was agreed and
> accepted.

I don't know, and I don't use AT-SPI myself, but I suspect you've just broken on-demand activation of at-spi2-registryd.

(In reply to comment #17)
> (In reply to comment #15)
> > Ok thanks for the feedback. I have now committed this since it was agreed and
> > accepted.
> 
> I don't know, and I don't use AT-SPI myself, but I suspect you've just broken
> on-demand activation of at-spi2-registryd.

You seem to be right actually. I will undo it. 

This is annoying.

Shall if we're putting it back we should test a patch without the type in and see if that works if Mike Gorse thinks that is a good idea too? I was not that clear on what Mike's response suggested about the type suggestion, to be honest. Mike?

(In reply to comment #9)
> (In reply to comment #0)

> * accessibility bus: either root or the same uid that ran dbus-daemon
>   may connect and do stuff, to support traditional bad ideas involving
>   running GUIs as root, such as gksu(1)

Just looking at this again does this not reinforce the argument for changing/removing 

    <!-- Allow root to connect -->
    <allow user="root"/>

rather than contradict it? At the moment it seems like root shouldn't be able to connect. I also noticed an improvement to the CPU performance problem in opening rhythm box with that but then I was also changing to the session bus with that patch too.

(In reply to comment #19)
> (In reply to comment #9)
> > (In reply to comment #0)
> 
> > * accessibility bus: either root or the same uid that ran dbus-daemon
> >   may connect and do stuff, to support traditional bad ideas involving
> >   running GUIs as root, such as gksu(1)
> 
> Just looking at this again does this not reinforce the argument for
> changing/removing 
> 
>     <!-- Allow root to connect -->
>     <allow user="root"/>
> 
> rather than contradict it? At the moment it seems like root shouldn't be able
> to connect.

If you (a plural form of "you" involving all AT-SPI developers) remove this, then you need to be clear about what you're doing, namely, breaking accessibility for GUI applications that run as root within another user's session. If that's acceptable collateral damage, sure, do it; if not, don't.

I think running X applications with privileges other than those of the desktop session is a bad idea for many reasons, but in practice, people do. I also think su(1) and proprietary video drivers are bad ideas, but people use those too. Just because I think something is a bad idea, that doesn't necessarily mean it's acceptable to make it stop working (even assuming I'm right).

(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #9)
> > > (In reply to comment #0)
> > 
> > > * accessibility bus: either root or the same uid that ran dbus-daemon
> > >   may connect and do stuff, to support traditional bad ideas involving
> > >   running GUIs as root, such as gksu(1)
> > 
> > Just looking at this again does this not reinforce the argument for
> > changing/removing 
> > 
> >     <!-- Allow root to connect -->
> >     <allow user="root"/>
> > 
> > rather than contradict it? At the moment it seems like root shouldn't be able
> > to connect.
> 
> If you (a plural form of "you" involving all AT-SPI developers) remove this,
> then you need to be clear about what you're doing, namely, breaking
> accessibility for GUI applications that run as root within another user's
> session. If that's acceptable collateral damage, sure, do it; if not, don't.
> 
> I think running X applications with privileges other than those of the desktop
> session is a bad idea for many reasons, but in practice, people do. I also
> think su(1) and proprietary video drivers are bad ideas, but people use those
> too. Just because I think something is a bad idea, that doesn't necessarily
> mean it's acceptable to make it stop working (even assuming I'm right).

I see what you're saying but surely there must be a smarter way to do this? I think default root access across the spectrum could be a potential security concern because currently accessibility is always on by default and whilst it is preferable to keep that going, it would be most preferable to not have any side effects which could be detrimental.

Incidentally, I went ahead and tested removing <type> but that seemed to break guis, so it's a no go :-(.

(In reply to comment #21)
> (In reply to comment #20)
> > (In reply to comment #19)
> > > (In reply to comment #9)
> > > > (In reply to comment #0)
> > > 
> > > > * accessibility bus: either root or the same uid that ran dbus-daemon
> > > >   may connect and do stuff, to support traditional bad ideas involving
> > > >   running GUIs as root, such as gksu(1)
> > > 
> > > Just looking at this again does this not reinforce the argument for
> > > changing/removing 
> > > 
> > >     <!-- Allow root to connect -->
> > >     <allow user="root"/>
> > > 
> > > rather than contradict it? At the moment it seems like root shouldn't be able
> > > to connect.
> > 
> > If you (a plural form of "you" involving all AT-SPI developers) remove this,
> > then you need to be clear about what you're doing, namely, breaking
> > accessibility for GUI applications that run as root within another user's
> > session. If that's acceptable collateral damage, sure, do it; if not, don't.
> > 
> > I think running X applications with privileges other than those of the desktop
> > session is a bad idea for many reasons, but in practice, people do. I also
> > think su(1) and proprietary video drivers are bad ideas, but people use those
> > too. Just because I think something is a bad idea, that doesn't necessarily
> > mean it's acceptable to make it stop working (even assuming I'm right).
> 
> I see what you're saying but surely there must be a smarter way to do this? I
> think default root access across the spectrum could be a potential security
> concern

I will repeat what I said before. That option doesn't grant automatically root access to any user using at-spi2. Means that if you already have root access, you will be allowed to connect. So, having root privileges is a pre-requisite. As Simon is saying, this is needed if you want to run an application while being root, that although not really advisable, people still does that.

(In reply to comment #16)
> (In reply to comment #14)
> > (In reply to comment #12)
> > > That would be a compatibility break which needs to be coordinated between the
> > > package that ships /etc/at-spi2/accessibility.conf, and all packages that want
> > > to be activatable on the accessibility bus.
> > 
> > Not really; at-spi2-registryd is the only program that activates on the
> > accessibility bus.
> 
> Excellent. Then it needs to be coordinated between AT-SPI2, and, er, itself.
> Should be easy :-)

Hmm:

registryd/org.a11y.atspi.Registry.service.in:3:Exec=@LIBEXECDIR@/at-spi2-registryd --use-gnome-session

The use --use-gnome-session flag indicated the session bus is being activated there when the service is registered, does it not?

(In reply to comment #23)

> registryd/org.a11y.atspi.Registry.service.in:3:Exec=@LIBEXECDIR@/at-spi2-registryd
> --use-gnome-session
> 
> The use --use-gnome-session flag indicated the session bus is being activated
> there when the service is registered, does it not?

From at-spi2-registryd --help:

  --use-gnome-session     Should register with gnome session manager

that flag is used to register to the gnome session manager. gnome session manager is not the same that the dbus session bus.

For example, when this option is used, at-spi2-registryd will try to register to the gnome-session (using RegisterClient [1] method), close itself if the gnome session ends, etc,

[1] https://people.gnome.org/~mccann/gnome-session/docs/gnome-session.html#org.gnome.SessionManager.RegisterClient (somewhat old link, but the only public one I found in relation to gnome-session. Anyway the documentation is the same)

Created attachment 287199 [details] [review]
Limit service activation to accessibility bus

Ok, not totally sure because I wasn't confident I was fully able to grasp the registryd chat, but I think this should do it.

The patch installs these two services into the $datadir/dbus-1/accessibility-services directory and stops using the session bus:

1. org.a11y.atspi.Registry.service
2. org.a11y.Bus.service

(In reply to comment #22)
> (In reply to comment #21)
> > (In reply to comment #20)
> > > (In reply to comment #19)
> > > > (In reply to comment #9)
> > > > > (In reply to comment #0)
> > > > 
> > > > > * accessibility bus: either root or the same uid that ran dbus-daemon
> > > > >   may connect and do stuff, to support traditional bad ideas involving
> > > > >   running GUIs as root, such as gksu(1)
> > > > 
> > > > Just looking at this again does this not reinforce the argument for
> > > > changing/removing 
> > > > 
> > > >     <!-- Allow root to connect -->
> > > >     <allow user="root"/>
> > > > 
> > > > rather than contradict it? At the moment it seems like root shouldn't be able
> > > > to connect.
> > > 
> > > If you (a plural form of "you" involving all AT-SPI developers) remove this,
> > > then you need to be clear about what you're doing, namely, breaking
> > > accessibility for GUI applications that run as root within another user's
> > > session. If that's acceptable collateral damage, sure, do it; if not, don't.
> > > 
> > > I think running X applications with privileges other than those of the desktop
> > > session is a bad idea for many reasons, but in practice, people do. I also
> > > think su(1) and proprietary video drivers are bad ideas, but people use those
> > > too. Just because I think something is a bad idea, that doesn't necessarily
> > > mean it's acceptable to make it stop working (even assuming I'm right).
> > 
> > I see what you're saying but surely there must be a smarter way to do this? I
> > think default root access across the spectrum could be a potential security
> > concern
> 
> I will repeat what I said before. That option doesn't grant automatically root
> access to any user using at-spi2. Means that if you already have root access,
> you will be allowed to connect. So, having root privileges is a pre-requisite.
> As Simon is saying, this is needed if you want to run an application while
> being root, that although not really advisable, people still does that.

Also regarding this: Isn't the main advantage of switching over to wayland from X is that the compositor can handle a lot of stuff which X normally has root access to handle and hence it is likely to be more secure? i.e. not every running process at the moment which has root access, should have root access.

As far as I understand it, the major use-case for root being allowed to access the accessibility bus is something like:

    gksudo gedit /etc/hosts

which is a terrible idea (all of the gedit codebase gets run as uid 0!) but people still do it.

This is orthogonal to whether the X server runs as root for, for instance, direct access to input and video devices.

(In reply to comment #27)
> As far as I understand it, the major use-case for root being allowed to access
> the accessibility bus is something like:
> 
>     gksudo gedit /etc/hosts
> 
> which is a terrible idea (all of the gedit codebase gets run as uid 0!) but
> people still do it.
> 
> This is orthogonal to whether the X server runs as root for, for instance,
> direct access to input and video devices.

Mmm... I'm not sure. There are more applications with default root access than should probably have that for a secure user login session to be taking place. Just running 

    ps -ef | grep root

I see that.

That said, if people were sure it wouldn't break the user experience for accessibility users to not have that access by default making (let's use your example of:) gksudo gedit /etc/hosts impossible. I'd be inclined to say it should not be on offer by default because as you suggest: It's a terrible idea...  

All the root discussion aside though, are you able to give a review of my latest patch, if you have any further thoughts on that?

I'd value your opinion on how it addresses the  <standard_session_servicedirs/> problem, and I think the others may too.

Comment on attachment 287199 [details] [review]
Limit service activation to accessibility bus

I apologize for the delay.

At-spi-bus-launcher is meant to run on the session bus--it is used primarily to retrieve the address for the accessibility bus. Anyway, I've modified your patch to keep org.a11y.Bus.service in the standard directory and committed.

(In reply to comment #27)
> As far as I understand it, the major use-case for root being allowed to access
> the accessibility bus is something like:
> 
>     gksudo gedit /etc/hosts
> 
> which is a terrible idea (all of the gedit codebase gets run as uid 0!) but
> people still do it.
> 
> This is orthogonal to whether the X server runs as root for, for instance,
> direct access to input and video devices.

YaST (the configuration tool used in openSUSE) also runs UI code as root (which probably isn't ideal, but it is being done for now). I don't know if other distributions have tools that do this. There was also a recent thread on the orca list with someone trying to run an app as the super-user and having problems. Anyway, some of the former use cases for running UI as root have been alleviated by policykit, but, still, it isn't functionality that I would like to break.

I think we can close this. Thanks for the report and patch.

(In reply to comment #29)
> (From update of attachment 287199 [details] [review])
> I apologize for the delay.
> 
> At-spi-bus-launcher is meant to run on the session bus--it is used primarily to
> retrieve the address for the accessibility bus. Anyway, I've modified your
> patch to keep org.a11y.Bus.service in the standard directory and committed.

On seeing the edit you made that actually makes perfect sense as a complete solution. 

I agree the root situation has a bit too many unknowns to risk messing about with it at this stage. The moment you mention YaST though, I begin to wonder about the status of ruby atspi matters... That's definitely digressing far away from this thread now I think, though!

Anyway, so... Thanks for the review and for closing the bug!

Fyi, I'd forgotten about setting a servicedir in accessibility.conf. Fixed in commit 6ad476.

(In reply to comment #21)
> Incidentally, I went ahead and tested removing <type> but that seemed to break
> guis, so it's a no go :-(.

Magdalen, would you mind clarifying what went wrong and why, either here or on <https://bugs.freedesktop.org/show_bug.cgi?id=84289>? (I tried to Cc you on that bug but you don't seem to have a freedesktop Bugzilla account.)

When I looked into what <type/> actually does before writing comment #7, I didn't find any effects that looked likely to break anything... but perhaps you've found another effect that I missed, or perhaps I didn't think through all the consequences of the ones I already listed?