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

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

[Ian Campbell]

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1342793598 -3600
# Node ID 5e8449a87a993cc2d2fb89a6ba0bbdc1df916771
# Parent  79cfa1892a5d55f2c137da6d4f2d5f261b47db26
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>

diff -r 79cfa1892a5d -r 5e8449a87a99 tools/libxc/xc_domain_restore.c
--- a/tools/libxc/xc_domain_restore.c	Fri Jul 20 14:51:05 2012 +0100
+++ b/tools/libxc/xc_domain_restore.c	Fri Jul 20 15:13:18 2012 +0100
@@ -1912,9 +1912,21 @@ int xc_domain_restore(xc_interface *xch,
             SET_FIELD(start_info, nr_pages, dinfo->p2m_size);
             SET_FIELD(start_info, shared_info, shared_info_frame<<PAGE_SHIFT);
             SET_FIELD(start_info, flags, 0);
+            if ( GET_FIELD(start_info, store_mfn) > dinfo->p2m_size )
+            {
+                ERROR("Suspend record xenstore frame number is bad");
+                munmap(start_info, PAGE_SIZE);
+                goto out;
+            }
             *store_mfn = ctx->p2m[GET_FIELD(start_info, store_mfn)];
             SET_FIELD(start_info, store_mfn, *store_mfn);
             SET_FIELD(start_info, store_evtchn, store_evtchn);
+            if ( GET_FIELD(start_info, console.domU.mfn) > dinfo->p2m_size )
+            {
+                ERROR("Suspend record console frame number is bad");
+                munmap(start_info, PAGE_SIZE);
+                goto out;
+            }
             *console_mfn = ctx->p2m[GET_FIELD(start_info, console.domU.mfn)];
             SET_FIELD(start_info, console.domU.mfn, *console_mfn);
             SET_FIELD(start_info, console.domU.evtchn, console_evtchn);

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

[Ian Jackson]

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 ?

Ian.

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

[Ian Campbell]

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.

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.

Ian.

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

[Daniel De Graaf]

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

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

[Ian Campbell]

On Fri, 2012-07-20 at 18:00 +0100, Daniel De Graaf wrote:
> 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.

That rings a bell. Sorry this freeze has been dragging on for so
long :-/

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

Thanks for pointing that out, I'd forgotten about that param.

Ian.

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

[Ian Jackson]

Daniel De Graaf writes ("Re: [Xen-devel] [PATCH] libxc: restore: bounds check for start_info.{store_mfn, console.domU.mfn}"):
> > On Fri, 2012-07-20 at 17:06 +0100, Ian Jackson wrote:
...
> >> Does this mean that a malicious restore file can take over the
> >> toolstack ?
> > 
> > Good question, I should have considered this before posting.
> > 
> > [discussion]

Thanks for looking into this; it looks like we don't need to worry
about that.  The original patch is good.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Ian.

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

[Ian Campbell]

On Mon, 2012-07-23 at 12:06 +0100, Ian Jackson wrote:
> Daniel De Graaf writes ("Re: [Xen-devel] [PATCH] libxc: restore: bounds check for start_info.{store_mfn, console.domU.mfn}"):
> > > On Fri, 2012-07-20 at 17:06 +0100, Ian Jackson wrote:
> ...
> > >> Does this mean that a malicious restore file can take over the
> > >> toolstack ?
> > > 
> > > Good question, I should have considered this before posting.
> > > 
> > > [discussion]
> 
> Thanks for looking into this; it looks like we don't need to worry
> about that.  The original patch is good.
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Thanks, applied.

> 
> Ian.