Ian Campbell
2012-Jul-20 14:13 UTC
[PATCH] libxc: restore: bounds check for start_info.{store_mfn, console.domU.mfn}
# 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);
Ian Jackson
2012-Jul-20 16:06 UTC
[PATCH] libxc: restore: bounds check for start_info.{store_mfn, console.domU.mfn}
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.
Ian Campbell
2012-Jul-20 16:30 UTC
Re: [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:> 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.
Daniel De Graaf
2012-Jul-20 17:00 UTC
Re: [PATCH] libxc: restore: bounds check for start_info.{store_mfn, console.domU.mfn}
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
Ian Campbell
2012-Jul-23 11:03 UTC
Re: [PATCH] libxc: restore: bounds check for start_info.{store_mfn, console.domU.mfn}
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.
Ian Jackson
2012-Jul-23 11:06 UTC
Re: [PATCH] libxc: restore: bounds check for start_info.{store_mfn, console.domU.mfn}
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.
Ian Campbell
2012-Jul-23 12:15 UTC
Re: [PATCH] libxc: restore: bounds check for start_info.{store_mfn, console.domU.mfn}
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.