[pulseaudio-discuss] Access control

[Wim Taymans]

On 13 February 2017 at 13:58, Tanu Kaskinen <tanuk at iki.fi> wrote:
> On Fri, 2017-01-27 at 17:15 +0100, Wim Taymans wrote:
>> Hi All,
>>
>> I think (1) is quite ugly and requires you to modify many functions
>> with a new parameter that
>> is not at all related to what the function does.
>
> I disagree with the characterization that the client parameter is not
> related to what the function does. If the function does access control,
> then the client parameter is very much related to that.

Together with things such as logging,configuration and allocation, security
is typically a Cross-cutting concern
(https://en.wikipedia.org/wiki/Cross-cutting_concern).
The methods that operate on objects should not include the parameters for
the cross-cutting concern, you would typically keep those stored in a global
environment or singleton (like how we have the logging in a singleton, the
mempool and configuration in pa_core, ..).
We should to the same with the current client, IMHO.

>
>> It does however give
>> you good control over what
>> you can check. One example is pa_sink_update_rate() which might
>> suspend and unsuspend
>> the source to implement the rate change. It a new client does not have
>> permission to suspend a
>> sink, it will not be able to change the rate.
>
> If I understood correctly, you're comparing options (1) and (2) here,
> and you're saying that option (2) will prevent sink rate changes
> for streams that are played by clients that don't have the permission
> to suspend sinks. I don't think that's true. pa_sink_update_rate() can
> set current_client to NULL before it calls pa_sink_suspend().

That is true but it is not so nice because it assumes that pa_sink_update_rate()
knows pa_sink_suspend() is going to perform some sort of access
control that would somehow fail and so it needs to work around it. Ideally
pa_sink_suspend() knows in what context it's called and can decide this
for itself.

Fortunately pa_sink_suspend() has a 'cause' argument that we can actually
use for this: PA_SUSPEND_USER. We are lucky here, theoretically, in some
cases there might not be enough context to do the right access checks.

>
>> (2) looks quite ok. Setting and clearing the current_client is
>> straightforward and easily verifiable.
>> You can also be certain that when you add new checks, you can just get
>> the client from there
>> instead of having to change a method. The downside is that if ever a
>> non-main thread tries to
>> do some access control, you get random behaviour. using TLS will, of
>> course fix this.
>
> I don't know why you're worried about other threads. I don't see the
> need to do access control from other threads than the main thread.

I've put some asserts to make sure the set and get are always done from
the control thread.

>
>> (3) is probably still the most simple solution but only deals with the
>> native-protocol. (1) and (2)
>> now also automatically work for the cli, DBus, esound, http, simple
>> and native protocol.
>>
>> What do you think? Which option do you like best?
>
> I feel I need to have a close look at all the implementations to give a
> definite opinion. So far I've only looked at the implementation of (3).
> At the moment I'm not strongly opposed to any of the alternatives, but
> if I had to choose right now, I'd prefer (1), then (2) and last (3).
> Option (3) is last, because it's easier to forget access checks when
> the core doesn't enforce it. (1) and (2) are functionally equivalent.
> Both are about enforcing access control in the core, and in both cases
> the core functions need the client object as a parameter. Option (1)
> does the parameter passing in the usual way, and option (2) does the
> parameter passing in an unusual way. Option (1) makes it obvious which
> functions involve access control, and forces the programmer to think
> about how access control should be done in each case, which I believe
> reduces the possibility for errors.
>

I've update the flatpak branch here:
https://cgit.freedesktop.org/~wtay/pulseaudio/log/?h=flatpack

Since option 3 is now at the bottom of the list and I also think it's
inferior to the
other options, I'm no longer going to update or propose it.

I tried to incorporate the suggestions you listed below.

> creds: add pid to pa_creds and use store it in pa_client
> --------------------------------------------------------
>
> This patch seems to have three separate changes that should probably be
> in separate patches: adding pid to pa_creds, adding creds to pa_client
> and adding the CLIENT_AUTH hook.

Split into 3 patches.

>
> +typedef struct pa_protocol_native_access_data {
> +    pa_access_data d;
> +
> +    pa_pdispatch *pd;
> +    uint32_t command;
> +    uint32_t tag;
> +    pa_tagstruct *tc;
> +    void *userdata;
> +} pa_protocol_native_access_data;
>
> The struct is not exported outside the .c file, so typedef should not
> be used, and it doesn't need the "pa_protocol_native_" prefix.
> https://www.freedesktop.org/wiki/Software/PulseAudio/Documentation/Developer/CodingStyle/
>
> "tc" is a weird abbreviation of "tagstruct", IMHO. I think "tagstruct"
> would be a better name for the field.
>
> The userdata field should be changed to "pa_native_connection
> *connection".

Done, done and done

>
> Reusing the existing pa_hook_result_t type is a bit ugly. I'd define a
> new type with more descriptive names than OK, STOP and CANCEL. Also, I
> think it would be good to not allow more than one callback per hook,
> because the current stacking semantics are weird: if the first callback
> returns OK and the second returns STOP, the result is that access is
> denied, which makes sense, but if the first callback returns CANCEL
> (i.e. async check), the second callback won't be called at all. If the
> second callback would return STOP if it were called, then shouldn't
> that override the previous CANCEL result?

I made a new pa_access_result_t which basically rename the
pa_hook_result_t values.  The access function checks then all use
and return this new enum.

>
> +static void check_access_finish_cb(pa_access_data *data, bool res) {
>
> What's "res"? The result of the authorization? "access_granted" would
> be a better name, except that the parameter doesn't affect the
> authorization end result when the value is true, so inverting the
> meaning and renaming the parameter to "access_denied" would perhaps
> make sense. It's not clear to me why the parameter even exists.

now:  void (*complete_cb) (pa_access_data *data, pa_access_result_t res);

>
> +finish:
> +    pa_xfree((char *)d->d.name);
> +    if (d->pd)
> +        pa_pdispatch_unref(d->pd);
> +    if (d->tc)
> +        pa_tagstruct_free(d->tc);
> +    pa_xfree(d);
>
> It would be good to encapsulate this in access_data_free().

Done

>
> Is it necessary to hold a reference to the pdispatch object? The
> pdispatch is guaranteed to stay around as long as the
> pa_native_connection object exists. Can we cancel all pending access
> checks (and free the access_data structs) when a connection dies,
> before pa_native_connection drops its reference to the pdispatch
> object?
>

Will look at that next.

>
> module-access: add example access module
> ----------------------------------------

I removed the example access module, I now have an example flatpak
access module, just so that I don't need to update the same thing another
time in a relatively useless example module.

>
> +typedef struct access_policy access_policy;
> +typedef struct event_item event_item;
> +typedef struct client_data client_data;
> +typedef struct userdata userdata;
>
> What's the purpose of these typedefs? If you need to forward declare
> something (like the access_policy struct references the userdata struct
> before the userdata struct has been defined) you can just do "struct
> foo;", no need for typedefs.

Removed the typedefs

>
> +struct client_data {
> +    uint32_t index;
> +    uint32_t policy;
> +
> +    PA_LLIST_HEAD(event_item, events);
> +};
>
> Why is the policy referenced by index rather than by pointer?

Used a pointer now

>
> +struct userdata {
> +    pa_core *core;
> +
> +    pa_hook_slot *hook[PA_ACCESS_HOOK_MAX];
> +
> +    pa_idxset *policies;
> +    uint32_t default_policy;
> +
> +    pa_hashmap *clients;
> +    pa_hook_slot *client_put_slot;
> +    pa_hook_slot *client_proplist_changed_slot;
> +    pa_hook_slot *client_unlink_slot;
> +};
>
> Why is the default policy referenced by index rather than by pointer?

Used a pointer now.

>
> It would be nice to have a comment that documents the clients hashmap's
> key and value types. It seems that the keys are client indexes, why not
> pa_client pointers?

Commented. I used the client index because that's what you get in the
pa_access_data structure. I could also set a pa_client* there, will check that
in next patch.

>
> Why does the module follow client proplist changes? It looks like the
> intention is to update the access policy based on the proplist, but the
> module actually never changes it. Maybe that's because the code is only
> at a prototype phase, but I don't think the code should ever evolve to
> use the proplist as a basis for choosing the access policy. The
> proplist is controlled by the client, so it can't be trusted.

Removed this. Before the AUTH hook was added, it was the only way to
get some (incomplete and unreliable) info about a client.

>
> You can use pa_module_hook_connect() for avoiding the need to manually
> free the client put, unlink and proplist_changed hook slots.

Done

>
> +        u->hook[i] = pa_hook_connect(&u->core->access[i], PA_HOOK_EARLY - 1, cb, u);
>
> Setting the priority to super high doesn't make much sense to me. This
> is the only module using the hooks, and you don't know what other
> modules there are going to be, and how they should be prioritized
> relative to this one. Stacking access control modules doesn't make much
> sense anyway with the current hook semantics (as I explained earlier).

I've added a core method to install the hook, which now checks if there is no
previous hook installed and set a NORMAL priority.

>
> module-access: add async handler for record
> -------------------------------------------
>
> struct client_data {
> +    struct userdata *u;
> +
> +    uint32_t index;
> +    uint32_t policy;
> +
> +    struct async_cache cached[PA_ACCESS_HOOK_MAX];
> +    pa_time_event *time_event;
> +    pa_access_data *access_data;
> +
> +    PA_LLIST_HEAD(event_item, events);
> +};
>
> pa_access_data is a per-request object, but this struct allows having
> only one pa_access_data per client. Shouldn't the module be prepared to
> handle multiple simultaneous requests?

It should, in the flatpak module it should keep the pa_access_data around in
a hashmap with the Request object path as the key. I will attempt this next.

>
> It would be nice to have a comment about what an "event_item"
> represents. I read the code yesterday, but now I have already forgotten
> what the purpose of those event items is... Ok, now I re-read the code,
> and an "event item" actually is just an identifier for an object
> (consisting of the type and object index). The events list in
> client_data contains the identifiers of all objects that the client is
> allowed to see. If an object isn't in the list, then subscription
> events for that object are prevented from reaching the client. I think
> "visible_objects" or something like that would be a better name for the
> list.

Renamed to visible_object.

Wim

>
> --
> Tanu
>
> https://www.patreon.com/tanuk