[Spice-devel] Fix filter string behaviour to be more intuitive before lots of people start depending on it

[Uri Lublin]

On 06/04/2012 04:28 PM, David Jaša wrote:
> Hans de Goede píše v Po 04. 06. 2012 v 14:55 +0200:
>> Hi,
>>
>> On 06/04/2012 01:17 PM, David Jaša wrote:
>>
>>> Hans de Goede píše v Pá 01. 06. 2012 v 20:45 +0200:
>>>> More importantly then the "incentive pro did it this way" argument though,
>>>> is that it is a safe default from security pov. These filter strings are
>>>> used in more then one place:
>>>>
>>> IIRC, they were quite confusing too, generating their number of false
>>> bugs. I see this as a opportunity to get things right before we start to
>>> depend on "wrong" behavior.
>>>
>>>> 1) In the client to decide which devices to auto-redirect when new devices
>>>>       are hotplugged
>>>> 2) In the client to specify (already plugged in) devices to redirect on
>>>>       client connect (*)
>>>> 3) On the qemu cmdline to specify which redirected devices will be accepted
>>>>
>>>> *) Not yet available, to be implemented soon.
>>>>
>>>> For 2 and 3 deny by default clearly is the right policy,
>>> WRT 2: agreed. I'd question 3 though. The best of "default deny" is USB
>>> redirection disabled altogether. I can imagine that when there is no
>>> "sane default" here, we'll end up with administrators going for the
>>> shortest string that will do the job. "-1,-1,-1,-1,1". Effectively doing
>>> exactly the opposite you want to achieve.
>> The default qemu behavior of course is no redirection, only if you specify
>> usbredir devices on the qemu cmdline you get usbredir. Then once you enable
>> usbredir devices, the default is to allow any devices, but once you
>> specify a filter, then, and only then does "-1,-1,-1,-1,0" always get
>> appended. So that if the admin requests filtering and he forgets to close
>> with the filter with a catch all rule, the behavior will be to deny anything
>> not explicitly allowed.
> Iff there is indeed used deny-all filter by default AND qemu will spit a
> message when no whitelist filter is set (something like "USB redirection
> is set but no devices are allowed to pass by the filter: %s" %
> FILTER_STIRNG), I'm fine with that.
>
>>>>    for 2 having a
>>>> redirect everything that does not match policy means redirecting all
>>>> devices the client has on client connect, not good. For 3 an allow by
>>>> defautl policy is simple not an option from a security pov.
>>>>
>>>>
>>>>>         2. there is a "soft default" that enables everything but HID
>>>>>            devices (aka 3,-1,-1,-1,0|-1,-1,-1,-1,1)
>>>>>         3. when user specifies a custom filter string, it
>>>>>                  * replaces string from #2
>>>>>                  * is based on top of #1
>>>>>
>>>>> All combined together cause unexpected behaviours like the one described
>>>>> in https://bugzilla.redhat.com/show_bug.cgi?id=823541 .
>>>>>
>>>>> The most straightforward fix seems to get rid of "soft default" #2 and
>>>>> use its value in "hard default" #1. That way, when user specifies
>>>>> --spice-usbredir-filter="11,-1,-1,-1,0", the effective filter string
>>>>> will be what user will expect:
>>>>> "3,-1,-1,-1,0|11,-1,-1,-1,0|-1,-1,-1,-1,1"
>>>>>
>>>>> instead of current behaviour where such string will become
>>>>> "11,-1,-1,-1,0|-1,-1,-1,-1,0", effectively block-all "-1,-1,-1,-1,0"
>>>>>
>>>>> Hans, do you think this is doable in a short time frame? If it is not
>>>>> done this way soon, people will constantly ask about that...
>>>> Doable yes, desirable no. Having a default policy is one thing, but
>>>> mandatory adding (in a very hidden way) "11,-1,-1,-1,0" to any
>>>> filter is just plain wrong, as it will surprise the user.
>>>>
>>> I might have been unclear here. Effective filter when one doesn't
>>> specify one is: "3,-1,-1,-1,0|-1,-1,-1,-1,1"
>> Correct.
>>
>>> When user specifies --spice-usbredir-filter="11,-1,-1,-1,0", the
>>> effective filter will be "-1,-1,-1,-1,0" (!)
>> Correct.
>>
>>> This does exactly what you
>>> question - it in a "very hidden way" removes default values from any
>>> filter, IMO exactly as "plain wrong" as what you say.
>> It does not remove default values, it overrides the default with
>> a user specified value, which is perfectly normal behavior for
>> user setable things.
>>
>> What you are asking for is a -background option for a program
>> which by default has red background and then when a user specifies
>> "-background blue" he gets a purple background!
>>
> No, I'm thinking in sets here:
>
> Current "soft default" filter is like this:
>
> +--------------------+
> |  -1,-1,-1,-1,1     |
> |                    |
> |  +--------------+  |
> |  | 3,-1,-1,-1,0 |  |
> |  +--------------+  |
> +--------------------+
>
> If I add there "11,-1,-1,-1,Z", I expect it to touch just newly-defined

I agree with Hans.

Default values are for cases where the user does not specifies specific 
values/options.
If a user specifies a value/option that's what being used.

A user specified filter is not added to the default filter, but 
overrides it, becoming the effective filter.

If you specify 11,-1-1-1,Z then that's your filter.

To prevent accidents Hans added the block-everything rule at the end.

That makes the filter   11,-1,-1,-1,Z|-1,-1,-1,-1,0

You can set the filter to be 11,-1,-1,-1,Z| 3,-1,-1,-1,0|-1,-1,-1,-1,1 
if you like to.

Regards,
     Uri.

> specific set "11,-1,-1,-1", not non-intersecting "-3,-1,-1,-1,X" nor
> universal "-1,-1,-1,-1,Y" sets:
>
> +-------------------------------------------------+
> |  -1,-1,-1,-1,UNCHANGED                          |
> |                             +---------------+   |
> |  +----------------------+   | 11,-1,-1,-1,Z |   |
> |  | 3,-1,-1,-1,UNCHANGED |   +---------------+   |
> |  +----------------------+                       |
> +-------------------------------------------------+
>
> Touching them is counter-intuitive.
>
> In your colour analogy it's like saying "hey I want blue square in
> top-left corner" and you'll get whole background blue instead of the
> rest staying red.
>
> David
>