[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: George Dunlap <george.dunlap@xxxxxxxxxx>, Doug Goldstein <cardoe@xxxxxxxxxx>
  • From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • Date: Mon, 13 Jan 2020 14:40:44 +0000
  • Authentication-results: esa5.hc3370-68.iphmx.com; dkim=none (message not signed) header.i=none; spf=None smtp.pra=andrew.cooper3@xxxxxxxxxx; spf=Pass smtp.mailfrom=Andrew.Cooper3@xxxxxxxxxx; spf=None smtp.helo=postmaster@xxxxxxxxxxxxxxx
  • Autocrypt: addr=andrew.cooper3@xxxxxxxxxx; prefer-encrypt=mutual; keydata= mQINBFLhNn8BEADVhE+Hb8i0GV6mihnnr/uiQQdPF8kUoFzCOPXkf7jQ5sLYeJa0cQi6Penp VtiFYznTairnVsN5J+ujSTIb+OlMSJUWV4opS7WVNnxHbFTPYZVQ3erv7NKc2iVizCRZ2Kxn srM1oPXWRic8BIAdYOKOloF2300SL/bIpeD+x7h3w9B/qez7nOin5NzkxgFoaUeIal12pXSR Q354FKFoy6Vh96gc4VRqte3jw8mPuJQpfws+Pb+swvSf/i1q1+1I4jsRQQh2m6OTADHIqg2E ofTYAEh7R5HfPx0EXoEDMdRjOeKn8+vvkAwhviWXTHlG3R1QkbE5M/oywnZ83udJmi+lxjJ5 YhQ5IzomvJ16H0Bq+TLyVLO/VRksp1VR9HxCzItLNCS8PdpYYz5TC204ViycobYU65WMpzWe LFAGn8jSS25XIpqv0Y9k87dLbctKKA14Ifw2kq5OIVu2FuX+3i446JOa2vpCI9GcjCzi3oHV e00bzYiHMIl0FICrNJU0Kjho8pdo0m2uxkn6SYEpogAy9pnatUlO+erL4LqFUO7GXSdBRbw5 gNt25XTLdSFuZtMxkY3tq8MFss5QnjhehCVPEpE6y9ZjI4XB8ad1G4oBHVGK5LMsvg22PfMJ ISWFSHoF/B5+lHkCKWkFxZ0gZn33ju5n6/FOdEx4B8cMJt+cWwARAQABtClBbmRyZXcgQ29v cGVyIDxhbmRyZXcuY29vcGVyM0BjaXRyaXguY29tPokCOgQTAQgAJAIbAwULCQgHAwUVCgkI CwUWAgMBAAIeAQIXgAUCWKD95wIZAQAKCRBlw/kGpdefoHbdD/9AIoR3k6fKl+RFiFpyAhvO 59ttDFI7nIAnlYngev2XUR3acFElJATHSDO0ju+hqWqAb8kVijXLops0gOfqt3VPZq9cuHlh IMDquatGLzAadfFx2eQYIYT+FYuMoPZy/aTUazmJIDVxP7L383grjIkn+7tAv+qeDfE+txL4 SAm1UHNvmdfgL2/lcmL3xRh7sub3nJilM93RWX1Pe5LBSDXO45uzCGEdst6uSlzYR/MEr+5Z JQQ32JV64zwvf/aKaagSQSQMYNX9JFgfZ3TKWC1KJQbX5ssoX/5hNLqxMcZV3TN7kU8I3kjK mPec9+1nECOjjJSO/h4P0sBZyIUGfguwzhEeGf4sMCuSEM4xjCnwiBwftR17sr0spYcOpqET ZGcAmyYcNjy6CYadNCnfR40vhhWuCfNCBzWnUW0lFoo12wb0YnzoOLjvfD6OL3JjIUJNOmJy RCsJ5IA/Iz33RhSVRmROu+TztwuThClw63g7+hoyewv7BemKyuU6FTVhjjW+XUWmS/FzknSi dAG+insr0746cTPpSkGl3KAXeWDGJzve7/SBBfyznWCMGaf8E2P1oOdIZRxHgWj0zNr1+ooF /PzgLPiCI4OMUttTlEKChgbUTQ+5o0P080JojqfXwbPAyumbaYcQNiH1/xYbJdOFSiBv9rpt TQTBLzDKXok86LkCDQRS4TZ/ARAAkgqudHsp+hd82UVkvgnlqZjzz2vyrYfz7bkPtXaGb9H4 Rfo7mQsEQavEBdWWjbga6eMnDqtu+FC+qeTGYebToxEyp2lKDSoAsvt8w82tIlP/EbmRbDVn 7bhjBlfRcFjVYw8uVDPptT0TV47vpoCVkTwcyb6OltJrvg/QzV9f07DJswuda1JH3/qvYu0p vjPnYvCq4NsqY2XSdAJ02HrdYPFtNyPEntu1n1KK+gJrstjtw7KsZ4ygXYrsm/oCBiVW/OgU g/XIlGErkrxe4vQvJyVwg6YH653YTX5hLLUEL1NS4TCo47RP+wi6y+TnuAL36UtK/uFyEuPy wwrDVcC4cIFhYSfsO0BumEI65yu7a8aHbGfq2lW251UcoU48Z27ZUUZd2Dr6O/n8poQHbaTd 6bJJSjzGGHZVbRP9UQ3lkmkmc0+XCHmj5WhwNNYjgbbmML7y0fsJT5RgvefAIFfHBg7fTY/i kBEimoUsTEQz+N4hbKwo1hULfVxDJStE4sbPhjbsPCrlXf6W9CxSyQ0qmZ2bXsLQYRj2xqd1 bpA+1o1j2N4/au1R/uSiUFjewJdT/LX1EklKDcQwpk06Af/N7VZtSfEJeRV04unbsKVXWZAk uAJyDDKN99ziC0Wz5kcPyVD1HNf8bgaqGDzrv3TfYjwqayRFcMf7xJaL9xXedMcAEQEAAYkC HwQYAQgACQUCUuE2fwIbDAAKCRBlw/kGpdefoG4XEACD1Qf/er8EA7g23HMxYWd3FXHThrVQ HgiGdk5Yh632vjOm9L4sd/GCEACVQKjsu98e8o3ysitFlznEns5EAAXEbITrgKWXDDUWGYxd pnjj2u+GkVdsOAGk0kxczX6s+VRBhpbBI2PWnOsRJgU2n10PZ3mZD4Xu9kU2IXYmuW+e5KCA vTArRUdCrAtIa1k01sPipPPw6dfxx2e5asy21YOytzxuWFfJTGnVxZZSCyLUO83sh6OZhJkk b9rxL9wPmpN/t2IPaEKoAc0FTQZS36wAMOXkBh24PQ9gaLJvfPKpNzGD8XWR5HHF0NLIJhgg 4ZlEXQ2fVp3XrtocHqhu4UZR4koCijgB8sB7Tb0GCpwK+C4UePdFLfhKyRdSXuvY3AHJd4CP 4JzW0Bzq/WXY3XMOzUTYApGQpnUpdOmuQSfpV9MQO+/jo7r6yPbxT7CwRS5dcQPzUiuHLK9i nvjREdh84qycnx0/6dDroYhp0DFv4udxuAvt1h4wGwTPRQZerSm4xaYegEFusyhbZrI0U9tJ B8WrhBLXDiYlyJT6zOV2yZFuW47VrLsjYnHwn27hmxTC/7tvG3euCklmkn9Sl9IAKFu29RSo d5bD8kMSCYsTqtTfT6W4A3qHGvIDta3ptLYpIAOD2sY3GYq2nf3Bbzx81wZK14JdDDHUX2Rs 6+ahAA==
  • Cc: Sergey Dyasli <sergey.dyasli@xxxxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Wei Liu <wl@xxxxxxx>, Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>, Ian Jackson <Ian.Jackson@xxxxxxxxxx>, Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
  • Delivery-date: Mon, 13 Jan 2020 14:41:15 +0000
  • Ironport-sdr: by5u+Cp8EXHkmqaaNGUxBqi1c8QjMPkGPKuAN+dbjxB8dh9i9dxVuIutBdWODvn3ZpPgh77gEg GzOctjZm2e6zJEjh2gBBG4wG/rQeamBDWGOfO6zzp+LQgQz1khLt1oYokRuimzg2zh+sjbWIL8 cUBeXHl98y+2YGat7tcHz6f6oNvV9ttAbkBeXlexK0mE/aZ6daaWrRJAV3Ngx3K6Db341NQMTM 6OSeMam5hwOV9FTF/65uPfu6XXOxbDXNVTKNpyE+zBUwWXWAFMXOJGDh3cRc6qhSF7SyKMgxB9 A5I=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Openpgp: preference=signencrypt

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@xxxxxxxxxx> 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@xxxxxxxxxx>
>>>>> ---
>>>>> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.