[Spice-devel] [PATCH spice-gtk 4/5] Add a suid root helper to open usb device nodes

[Hans de Goede]

Hi,

On 11/15/2011 08:37 PM, Marc-André Lureau wrote:
> On Tue, Nov 15, 2011 at 4:31 PM, Hans de Goede<hdegoede at redhat.com>  wrote:
>> +AC_ARG_ENABLE([polkit],
>> +  AS_HELP_STRING([--enable-polkit=@<:@yes/no@:>@],
>> +                 [Enable policykit support (for the usb acl helper)@<:@default=yes@:>@]),
>> +  [],
>> +  [enable_polkit="yes"])
>
> No need for that (Apparently. Anyway, it shouldn't need to be
> optionnal for usbredir).

Ah, so you want to just make it a mandatory dep for usb-redir. The problem with
that is that usbredir should work on mac os x too (without any changes, not tested),
and with some more work on windows as well, and neither will have policykit.

>
>> +       -lacl                                   \
>> +       $(NULL)
>
> Shouldn't libacl be checked in configure.ac too?

You're right, will fix.

>> @@ -145,10 +153,39 @@ static gboolean spice_usbredir_channel_open_device(
>>      }
>>
>>      spice_channel_connect(SPICE_CHANNEL(channel));
>> +    priv->connected = TRUE;
>>
>>      return TRUE;
>>   }
>>
>> +#if USE_POLKIT
>> +static void spice_usbredir_channel_open_acl_cb(
>> +    GObject *gobject, GAsyncResult *acl_res, gpointer user_data)
>> +{
>> +    SpiceUsbAclHelper *acl_helper = SPICE_USB_ACL_HELPER(gobject);
>> +    SpiceUsbredirChannel *channel = SPICE_USBREDIR_CHANNEL(user_data);
>> +    SpiceUsbredirChannelPrivate *priv = channel->priv;
>> +    GError *err = NULL;
>> +
>> +    g_return_if_fail(acl_helper == priv->acl_helper);
>> +
>> +    if (spice_usb_acl_helper_open_acl_finish(acl_helper, acl_res,&err))
>> +        spice_usbredir_channel_open_device(channel,&err);
>> +
>> +    if (err) {
>> +        g_simple_async_result_take_error(priv->result, err);
>> +        g_clear_object(&priv->context);
>> +        g_clear_object(&priv->device);
>> +    }
>> +
>> +    spice_usb_acl_helper_close_acl(priv->acl_helper);
>> +    g_clear_object(&priv->acl_helper);
>> +
>> +    g_simple_async_result_complete_in_idle(priv->result);
>> +    g_clear_object(&priv->result);
>> +}
>> +#endif
>> +
>>   G_GNUC_INTERNAL
>>   void spice_usbredir_channel_connect(SpiceUsbredirChannel *channel,
>>                                      GUsbContext          *context,
>> @@ -180,9 +217,21 @@ void spice_usbredir_channel_connect(SpiceUsbredirChannel *channel,
>>      priv->context = g_object_ref(context);
>>      priv->device  = g_object_ref(device);
>>      if (!spice_usbredir_channel_open_device(channel,&err)) {
>> +#if USE_POLKIT
>> +        priv->result = result;
>> +        priv->acl_helper = spice_usb_acl_helper_new();
>
> They should be closed&  disposed in dispose imho.

The close, closes stdin of the helper, at which point it will restore
the acl to its original state and then exit. Which I would rather see done
sooner then later.

More over the channel (object) is created at client connect, but not connected
to the spice-server until a usb device shows up. Then once the usb device goes
away, the channel is disconnect, but stays around. It can then be re-used for
another usb device.

So the lifetime of the helper should be either one of the connected time of the
channel, or just long enough for the channel code to open the device. Since there
is no need for / use in keeping the helper around after the channel code has
opened the device node I've choosen for the latter.

>
>> +        spice_usb_acl_helper_open_acl(priv->acl_helper,
>> +                                      g_usb_device_get_bus(device),
>> +                                      g_usb_device_get_address(device),
>> +                                      cancellable,
>> +                                      spice_usbredir_channel_open_acl_cb,
>> +                                      channel);
>
> Since the SpiceUsbAclHelper keep running when it's unref, we should
> give away a reference to the channel.  And probably close&  dispose on
> channel dispose. See below
>

I'm afraid don't understand / completely follow what you're trying to say
here...

>> +#else
>>          g_simple_async_result_take_error(result, err);
>>          g_clear_object(&priv->context);
>>          g_clear_object(&priv->device);
>> +#endif
>
> I don't think we need a fallback, we are not interested in weird
> setups, and people shouldn't run spice-gtk as root.

We need this path for Mac OS X.

>> +#if USE_POLKIT
>> +    /*
>> +     * If we're still waiting for the acl helper cancel it and don't clear
>> +     * priv->device and context, keeping this channel "busy" until
>> +     * spice_usbredir_channel_open_acl_cb() has run from the main loop,
>> +     * avoiding a new connect happening before the old connect has finished.
>> +     */
>
> If it's called from finalize() I don't see how you could keep the
> channel "busy".

It cannot be called from finalize() while the acl_helper is still active,
since the async result holds a reference to its source object, and we don't
unref the result until the acl_open completes at which point acl_helper will be unset.

  Instead we should break depedency in dispose(),
> "break" the reference and cancel the helper asap or in idle. This
> would also avoid the "busy" situation if it's called in a different
> case, not from finalize().
>
>> +    if (priv->acl_helper) {
>> +        spice_usb_acl_helper_close_acl(priv->acl_helper);
>> +    } else
>
> Do we need a else?
>

Yes, otherwise we could get:
channel_open_device 1 -> in progress
channel_close_device()
  calls spice_usb_acl_helper_close_acl
   cancels the pending acl open, will complete open in idle!
channel_open_device 2
  sets priv->device to new device
acl_open called from channel_open_device 1 completes
  complete callback clears priv->device since there is an err (it was cancelled)

But now it is clearing the new device from the 2nd open! Hence the else,
which results in keeping the channel in the open / busy state until the
acl_open complete callback runs from idle.


>> +#endif
>> +    {
>>          g_clear_object(&priv->device);
>>          g_clear_object(&priv->context);
>>      }
>
>
> the rests looks ok, but we might want to be extra careful about the
> setuid root... Did you discuss that issue with Kay? Couldn't we have a
> group/user for it / USB devices? (I am just sayin', I don't really
> know what that imply)

I've discussed this with Kay in the sense of: "Kay, can't we let udev
do the acl mangling for us like it does for normal devices too" and
the answer to that was no. Using an other group for usb devices
and making the helper sgid to this group won't work since changing
acl's can only be done by the file owner and root. We could change
the default owner of the usb files to a special user and have the helper
be suid this user. I'll discuss this with Kay tomorrow.

Regards,

Hans




>