Re: [PATCH] libxc: restore: bounds check for start_info.{store_mfn, console.domU.mfn}

[Daniel De Graaf <dgdegra@tycho.nsa.gov>]

On 07/20/2012 12:30 PM, Ian Campbell wrote:
> On Fri, 2012-07-20 at 17:06 +0100, Ian Jackson wrote:
>> Ian Campbell writes ("[PATCH] libxc: restore: bounds check for start_info.{store_mfn, console.domU.mfn}"):
>>> libxc: restore: bounds check for start_info.{store_mfn,console.domU.mfn}
>>>
>>> These fields are canonicalised by the guest on suspend and therefore must be
>>> valid pfns during restore.
>>>
>>> Reported-by: Jonathan Ludlam <Jonathan.Ludlam@eu.citrix.com>
>>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>>
>> Does this mean that a malicious restore file can take over the
>> toolstack ?
> 
> Good question, I should have considered this before posting.
> 
> The value in question is used as an offset into the p2m. So this allows
> the attacker to read off the end of that array, potentially reading some
> other word and storing it in either *store_mfn or *console_mfn (or
> both). Lets assume that the attacker is clever and can control some
> value which can be seen in this way (perhaps the tools have a guest page
> mapped which they control).
> 
> The values are written to the attacker's guest's start info (harmful
> only to themselves, I think) and used to seed a grant table entry.
> Seeding the gnttab would allow the attacker to potentially grant access
> to some other domain to one of the attacker's domain's own pages, which
> again seems harmless enough. You cannot grant a page you do not own so
> there is no way to leak information that way.
> 
> The *foo_mfn pointers are arguments to the xc_domain_restore function
> and are then used by the toolstack to write the mfns to xenstore and for
> xs_domain_introduce (I can't see any other use in libxl/xl).
> 
> I believe both xenconsoled and xenstored will default to using the grant
> table entries seeded above these days, which will prevent them from
> inadvertently mapping a page other than that owned by the attacher's
> guest.

Actually, it's just xenstored that was changed (oxenstored was not). I
have a patch to do the same for xenconsoled saved for when 4.3 opens, but
it was regarded as too late for 4.2 last time I mentioned it.

> Some versions of those daemons use the mmap foreign privileged
> interface. I suppose this could be used to trick xenconsoled into
> treating an arbitrary page as the guests console or to trick xenstored
> into treating an arbitrary page as a xenstore ring. I'm not sure if that
> is dangerous or not.

The map_foreign_range call does include a domain ID all the way up to the
hypervisor, which prevents the daemons from mapping pages that the target
domain in question isn't able to map on its own.

-- 
Daniel De Graaf
National Security Agency