Re: [Xen-devel] [PATCH v2] xsm: hide detailed Xen version from unprivileged guests

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 13/01/2020 12:51, George Dunlap wrote:
> On 1/12/20 6:26 PM, Doug Goldstein wrote:
>> On 1/11/20 3:02 AM, George Dunlap wrote:
>>>
>>>> On Jan 11, 2020, at 4:02 AM, Doug Goldstein <cardoe@cardoe.com> wrote:
>>>>
>>>>
>>>>
>>>> On 1/10/20 4:37 AM, Sergey Dyasli wrote:
>>>>> Hide the following information that can help identify the running Xen
>>>>> binary version: XENVER_extraversion, XENVER_compile_info,
>>>>> XENVER_changeset.
>>>>> Add explicit cases for XENVER_commandline and XENVER_build_id as well.
>>>>> Introduce xsm_filter_denied() to hvmloader to remove "<denied>" string
>>>>> from guest's DMI tables that otherwise would be shown in tools like
>>>>> dmidecode.
>>>>> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
>>>>> ---
>>>>> v1 --> v2:
>>>>> - Added xsm_filter_denied() to hvmloader instead of modifying
>>>>> xen_deny()
>>>> So 100% this version of the patch won't fly with the various
>>>> downstreams that run the v1 of this patch. Those various consumers
>>>> will stick with v1.
>>>>
>>>> If the goal of this is to reduce the burden of the downstreams and
>>>> their customers to carry a patch against Xen then I wouldn't even
>>>> bother with this version.
>>> If the goal is to come up with a solution that works for everyone, it
>>> would be helpful if you said *why* “various consumers” would find this
>>> patch unacceptable; and also what they might think about the alternate
>>> solutions proposed (and why).
>>>
>>>   -George
>>>
> [snip]
>
>> Now I know someone is going to read this and say "Look at Doug and him
>> advocating for security through obscurity".
> FWIW I'd be the first person to contradict them, and say you were
> practicing "defense in depth". :-)
>
>> Ultimately my point is if the goal of this patch is to upstream a patch
>> that's carried by various downstreams, why not actually listen to what
>> caused them to write the patch?
> Right, that's what I'm trying to do; but I don't seem to be making much
> progress.
>
> Here's my summary of the situation and arguments so far:
>
> 1. The xen_version hypercall can return strings for a number of
> different values, including XENVER_extraversion, which gives the point
> release and build id.
>
> 2. The XSM dummy module has code to filter which of these are allowed
> for unprivileged guests.  When access to a given value is filtered, no
> error is returned; rather, the string "<denied>" is returned.

Given an ABI which is capable of expressing -EPERM, it is perhaps worth
noting that this behaviour is because some drivers will BSOD if the call
fails...

> 3. Knowledge about the specific instantiation of Xen on which they are
> running makes it easier for attackers to know how to attack t he system;
> the XENVER_extraversion provides little value to legitimate users, but a
> lot of value to attackers.   As a defense-in-depth measure, it's
> important to be able to hide this information.
>
> 4. There's currently a patch carried by many downstreams, which changes
> the XSM dummy module to deny XENVER_extraversion to unprivileged guests.
>
> 5. However, this caused "<denied>" to show up in various user-visible
> places, which caused customer support headaches.  So this out-of-tree
> patch also replaced the string returned when denying access to ""
> instead.  Note that this is not *only* for XENVER_extraversion; with
> that patch, *any* time the value requested in xen_version is denied by
> policy, "" will be returned.
>
> 6. Silently returning an empty string is considered bad interface design
> by several developers.

Several others disagree with this claim.  Not least because it is very
common signal for "no information".

This is why several downstreams really have gotten away with using the
empty string, for products which have been in the field for years.

>   So Sergey's second patch:
>  - Still denies XENVER_extraversion at the hypervisor level
>  - Leaves the value returned by the hypervisor as "<denied>"
>  - Filters the "<denied>" string at the hvmloader level, to prevent it
> leaking into a GUI and scaring customers.

The SMBios table isn't the only way XENVER_extraversion leaks up into
the UI.

XENVER_extraversion isn't the only source of redacted information
leaking up into the UI.

Linux for example exports it all via sysfs.  The windows drivers put
XENVER_extraversion into several other logs.

>
> Now we get to Andy's objection on the 10th:
>
> ---
> The reason for this (which ought to be obvious, but I guess only to
> those who actually do customer support) is basic human physiology.
> "denied" means something has gone wrong.  It scares people, and causes
> them to seek help to change fix whatever is broken.
>
> It is not appropriate for it to find its way into the guest in the first
> place, and that includes turning up in `dmesg` and other logs, and
> expecting guest runtime to filter for it is complete nonsense.
> ---
>
> Basically, Andy says that *anywhere* it might show up is way too scary,
> even a guest dmesg log.
>
> Well, I disagree; I look in "dmesg" and I see loads of "scary" things.

Just because dmesg is not an example of a good UI, doesn't mean its ok
for us to make:

Xen version: 4.14<denied> (preserve-AD)

the default thing shown in a Xen system.

> But if "<denied>" is too scary, then we can try "<hidden>".

From an abstract PoV, hidden is less bad than denied, but:

Xen version: 4.14<hidden> (preserve-AD)

isn't ok either.

>
> Then we come to your mail.
>
> You spend two paragraphs justifying why we need to do #4 (hide the value
> from unprivileged guests), basically reiterating point #3 and dealing
> with potential objections.  But nobody objects to #4, or disagrees with #3.
>
> You then have a paragraph arguing why it's important that information be
> stripped at the hypervisor rather than in the toolstack.
>
> But Sergey's v2 patch *does* strip the information at the hypervisor.
> His patch makes it so that XENVER_extraversion returns "<denied>".  The
> code which converts "<denied>" to "" in hvmloader is purely a UI thing,
> so that people looking in their Windows System Info don't get scary
> messages.
>
>> I'd be happy if we had a Kconfig option behind what the string is. Give
>> me a blank as an option but default it to whatever string like
>> "<hidden>" that you'd like. Every shipping Xen distro I've worked on has
>> had its own v1 variant of the patch and none of them authored by me.
>
> OK, so with this we have four proposed options:
>
> 1. Block XENVER_extraversion at the hypervisor level.  Change the
> xen_deny() string to "".  (This is v1 of sergey's patch.)
>
> 2. Block XENVER_extraversion at the hypervisor level.  Leave xen_deny()
> as returning "<denied>", but replace "<denied>" with "" in hvmloader so
> it doesn't show up in the System Info and scare users.
>
> 3. Block XENVER_extraversion at the hypervisor level.  Change xen_deny()
> to return a more benign string like "<hidden>".  (Perhaps also filter it
> in hvmloader, just for good measure.)
> defence in depth argument is quite compelling.
>
>
>
>>  -George
> 4. Block XENVER_extraversion at the hypervisor level.  Make the
> xen_deny() string configurable in KConfig.
>
> Fundamentally I have no objection to #4.  But I still don't know what
> your objections are to #2 and #3.

Because they don't fix the other bugs introduced by the patch.

I don't think there is any argument over redacting more information than
is currently redacted - the security arguments for doing so are entirely
fine.  The issues are all over how the redaction occurs, and in
particular, how redacting new information interacts with the current
redaction scheme to cause new problems.

It is physically possible to build a hypervisor which has empty strings
in all of this information, without even editing the source code. 
(Getting the compiler string to be empty would be a little tricky, but
not not impossible.  Everything else is easy.)

It is also possible to build a hypervisor which has the literal
"<denied>", which would then appear even to privileged domains, in this
form.

There is no such thing as a silently failure in the interface, because
there is no way to distinguish an elided value from a real value, in
either its "" or "<denied>" form.

What "" gets you that nothing else does is sensible looking logging with
no guest changes necessary.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel