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

[David Jaša]

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
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


> Regards,
> 
> Hans

-- 

David Jaša, RHCE

SPICE QE based in Brno
GPG Key:     22C33E24 
Fingerprint: 513A 060B D1B4 2A72 7F0D 0278 B125 CD00 22C3 3E24