[PATCH] libxenlight: fix heap overflow when domid_to_name returns NULL

[PATCH] libxenlight: fix heap overflow when domid_to_name returns NULL

[Eamon Walsh]

The function flexarray_vappend() will stop at the first NULL
argument.  In libxl_device_vfb_add(), this has been observed
to result in keys being added to the backend array without
associated values in cases where the value can be NULL.
This causes libxl__xs_kvs_of_flexarray(), which expects an even
number of array elements, to write past the end of the array.

Fix by manually appending two values.

Signed-off-by: Eamon Walsh <ewalsh@tycho.nsa.gov>
---

diff -r c5d121fd35c0 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Mon Feb 28 16:55:20 2011 +0000
+++ b/tools/libxl/libxl.c	Thu Mar 03 18:32:10 2011 -0500
@@ -1890,9 +1890,11 @@
     flexarray_vappend(back, "frontend-id", libxl__sprintf(&gc, "%d", vfb->domid), NULL);
     flexarray_vappend(back, "online", "1", NULL);
     flexarray_vappend(back, "state", libxl__sprintf(&gc, "%d", 1), NULL);
-    flexarray_vappend(back, "domain", libxl__domid_to_name(&gc, domid), NULL);
+    flexarray_append(back, "domain");
+    flexarray_append(back, libxl__domid_to_name(&gc, domid));
     flexarray_vappend(back, "vnc", libxl__sprintf(&gc, "%d", vfb->vnc), NULL);
-    flexarray_vappend(back, "vnclisten", vfb->vnclisten, NULL);
+    flexarray_append(back, "vnclisten");
+    flexarray_append(back, vfb->vnclisten);
     flexarray_append(back, "vncpasswd");
     flexarray_append(back, vfb->vncpasswd);
     flexarray_vappend(back, "vncdisplay", libxl__sprintf(&gc, "%d", vfb->vncdisplay), NULL);



-- 

Eamon Walsh 
National Security Agency

Re: [PATCH] libxenlight: fix heap overflow when domid_to_name returns NULL

[Ian Campbell]

On Thu, 2011-03-03 at 23:29 +0000, Eamon Walsh wrote:
> The function flexarray_vappend() will stop at the first NULL
> argument.  In libxl_device_vfb_add(), this has been observed
> to result in keys being added to the backend array without
> associated values in cases where the value can be NULL.

If these values are NULL should we be writing them at all? e.g. for:
	flexarray_vappend(back, foo, bar);
where bar may be NULL shouldn't it become:
	if (bar) 
		flexarray_vappend(back, foo, bar);
or perhaps:
	flexarray_vappend(back, foo, bar ? bar : "");
?

> This causes libxl__xs_kvs_of_flexarray(), which expects an even
> number of array elements, to write past the end of the array.

Sounds like libxl__xs_kvs_of_flexarray could also be made more robust
here...

Ian.

> 
> Fix by manually appending two values.
> 
> Signed-off-by: Eamon Walsh <ewalsh@tycho.nsa.gov>
> ---
> 
> diff -r c5d121fd35c0 tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c	Mon Feb 28 16:55:20 2011 +0000
> +++ b/tools/libxl/libxl.c	Thu Mar 03 18:32:10 2011 -0500
> @@ -1890,9 +1890,11 @@
>      flexarray_vappend(back, "frontend-id", libxl__sprintf(&gc, "%d", vfb->domid), NULL);
>      flexarray_vappend(back, "online", "1", NULL);
>      flexarray_vappend(back, "state", libxl__sprintf(&gc, "%d", 1), NULL);
> -    flexarray_vappend(back, "domain", libxl__domid_to_name(&gc, domid), NULL);
> +    flexarray_append(back, "domain");
> +    flexarray_append(back, libxl__domid_to_name(&gc, domid));
>      flexarray_vappend(back, "vnc", libxl__sprintf(&gc, "%d", vfb->vnc), NULL);
> -    flexarray_vappend(back, "vnclisten", vfb->vnclisten, NULL);
> +    flexarray_append(back, "vnclisten");
> +    flexarray_append(back, vfb->vnclisten);
>      flexarray_append(back, "vncpasswd");
>      flexarray_append(back, vfb->vncpasswd);
>      flexarray_vappend(back, "vncdisplay", libxl__sprintf(&gc, "%d", vfb->vncdisplay), NULL);
> 
> 
> 

Re: [PATCH] libxenlight: fix heap overflow when domid_to_name returns NULL

[Stefano Stabellini]

On Fri, 4 Mar 2011, Ian Campbell wrote:
> On Thu, 2011-03-03 at 23:29 +0000, Eamon Walsh wrote:
> > The function flexarray_vappend() will stop at the first NULL
> > argument.  In libxl_device_vfb_add(), this has been observed
> > to result in keys being added to the backend array without
> > associated values in cases where the value can be NULL.
> 
> If these values are NULL should we be writing them at all? e.g. for:
> 	flexarray_vappend(back, foo, bar);
> where bar may be NULL shouldn't it become:
> 	if (bar) 
> 		flexarray_vappend(back, foo, bar);
> or perhaps:
> 	flexarray_vappend(back, foo, bar ? bar : "");
> ?
> 

This is actually a serious issue because it means that every time
flexarray_vappend is used and the argument is NULL the behaviour is
going to be different from what the coder expected.
Maybe flexarray_vappend should assume that the number of args is odd and
greater than 2?
At least in that case flexarray_vappend would only break if the user
misused the function.
Or we could use a terminator other than NULL...

Re: [PATCH] libxenlight: fix heap overflow when domid_to_name returns NULL

[Ian Campbell]

On Mon, 2011-03-07 at 13:12 +0000, Stefano Stabellini wrote:
> On Fri, 4 Mar 2011, Ian Campbell wrote:
> > On Thu, 2011-03-03 at 23:29 +0000, Eamon Walsh wrote:
> > > The function flexarray_vappend() will stop at the first NULL
> > > argument.  In libxl_device_vfb_add(), this has been observed
> > > to result in keys being added to the backend array without
> > > associated values in cases where the value can be NULL.
> > 
> > If these values are NULL should we be writing them at all? e.g. for:
> > 	flexarray_vappend(back, foo, bar);
> > where bar may be NULL shouldn't it become:
> > 	if (bar) 
> > 		flexarray_vappend(back, foo, bar);
> > or perhaps:
> > 	flexarray_vappend(back, foo, bar ? bar : "");
> > ?
> > 
> 
> This is actually a serious issue because it means that every time
> flexarray_vappend is used and the argument is NULL the behaviour is
> going to be different from what the coder expected.
> Maybe flexarray_vappend should assume that the number of args is odd and
> greater than 2?

flexarray_vappend is not solely used with libxl__xs_kvs_of_flexarray
though, in other cases it may be perfectly valid to have an odd number
of items.

flexarray_vappend_pair() perhaps?

Ian.

> At least in that case flexarray_vappend would only break if the user
> misused the function.
> Or we could use a terminator other than NULL...

Re: [PATCH] libxenlight: fix heap overflow when domid_to_name returns NULL

[Stefano Stabellini]

On Mon, 7 Mar 2011, Ian Campbell wrote:
> On Mon, 2011-03-07 at 13:12 +0000, Stefano Stabellini wrote:
> > On Fri, 4 Mar 2011, Ian Campbell wrote:
> > > On Thu, 2011-03-03 at 23:29 +0000, Eamon Walsh wrote:
> > > > The function flexarray_vappend() will stop at the first NULL
> > > > argument.  In libxl_device_vfb_add(), this has been observed
> > > > to result in keys being added to the backend array without
> > > > associated values in cases where the value can be NULL.
> > > 
> > > If these values are NULL should we be writing them at all? e.g. for:
> > > 	flexarray_vappend(back, foo, bar);
> > > where bar may be NULL shouldn't it become:
> > > 	if (bar) 
> > > 		flexarray_vappend(back, foo, bar);
> > > or perhaps:
> > > 	flexarray_vappend(back, foo, bar ? bar : "");
> > > ?
> > > 
> > 
> > This is actually a serious issue because it means that every time
> > flexarray_vappend is used and the argument is NULL the behaviour is
> > going to be different from what the coder expected.
> > Maybe flexarray_vappend should assume that the number of args is odd and
> > greater than 2?
> 
> flexarray_vappend is not solely used with libxl__xs_kvs_of_flexarray
> though, in other cases it may be perfectly valid to have an odd number
> of items.
> 
> flexarray_vappend_pair() perhaps?

flexarray_vappend_pairs considering that it is going to take a variable
number of pairs?
Otherwise we might as well have a flexarray_append_pair that doesn't use
vargs at all.

Re: [PATCH] libxenlight: fix heap overflow when domid_to_name returns NULL

[Ian Campbell]

On Mon, 2011-03-07 at 15:04 +0000, Stefano Stabellini wrote:
> On Mon, 7 Mar 2011, Ian Campbell wrote:
> > On Mon, 2011-03-07 at 13:12 +0000, Stefano Stabellini wrote:
> > > On Fri, 4 Mar 2011, Ian Campbell wrote:
> > > > On Thu, 2011-03-03 at 23:29 +0000, Eamon Walsh wrote:
> > > > > The function flexarray_vappend() will stop at the first NULL
> > > > > argument.  In libxl_device_vfb_add(), this has been observed
> > > > > to result in keys being added to the backend array without
> > > > > associated values in cases where the value can be NULL.
> > > > 
> > > > If these values are NULL should we be writing them at all? e.g. for:
> > > > 	flexarray_vappend(back, foo, bar);
> > > > where bar may be NULL shouldn't it become:
> > > > 	if (bar) 
> > > > 		flexarray_vappend(back, foo, bar);
> > > > or perhaps:
> > > > 	flexarray_vappend(back, foo, bar ? bar : "");
> > > > ?
> > > > 
> > > 
> > > This is actually a serious issue because it means that every time
> > > flexarray_vappend is used and the argument is NULL the behaviour is
> > > going to be different from what the coder expected.
> > > Maybe flexarray_vappend should assume that the number of args is odd and
> > > greater than 2?
> > 
> > flexarray_vappend is not solely used with libxl__xs_kvs_of_flexarray
> > though, in other cases it may be perfectly valid to have an odd number
> > of items.
> > 
> > flexarray_vappend_pair() perhaps?
> 
> flexarray_vappend_pairs considering that it is going to take a variable
> number of pairs?

That was what I was thinking at the time...

> Otherwise we might as well have a flexarray_append_pair that doesn't use
> vargs at all.

... but I think this makes more sense. Anywhere which actually does use
vappend to append multiple pairs would probably be clearer as a series
of these anyway.

Ian.

Re: [PATCH] libxenlight: fix heap overflow when domid_to_name returns NULL

[Stefano Stabellini]

On Mon, 7 Mar 2011, Ian Campbell wrote:
> > flexarray_vappend_pairs considering that it is going to take a variable
> > number of pairs?
> 
> That was what I was thinking at the time...
> 
> > Otherwise we might as well have a flexarray_append_pair that doesn't use
> > vargs at all.
> 
> ... but I think this makes more sense. Anywhere which actually does use
> vappend to append multiple pairs would probably be clearer as a series
> of these anyway.

I agree

Re: [PATCH] libxenlight: fix heap overflow when domid_to_name returns NULL

[Eamon Walsh]

On 03/04/2011 05:02 AM, Ian Campbell wrote:
> On Thu, 2011-03-03 at 23:29 +0000, Eamon Walsh wrote:
>> The function flexarray_vappend() will stop at the first NULL
>> argument.  In libxl_device_vfb_add(), this has been observed
>> to result in keys being added to the backend array without
>> associated values in cases where the value can be NULL.
> If these values are NULL should we be writing them at all? e.g. for:
> 	flexarray_vappend(back, foo, bar);
> where bar may be NULL shouldn't it become:
> 	if (bar) 
> 		flexarray_vappend(back, foo, bar);
> or perhaps:
> 	flexarray_vappend(back, foo, bar ? bar : "");
> ?
>

If the value is NULL, the key is skipped and not written.  This is because of a patch I submitted to change the xs_writev() function, which was calling strlen(NULL) previously.  See:
http://lists.xensource.com/archives/html/xen-devel/2010-03/msg00703.html

However this behavior is not obvious.  Checking the value earlier and leaving it off the list makes sense.



-- 

Eamon Walsh 
National Security Agency

Re: [PATCH] libxenlight: fix heap overflow when domid_to_name returns NULL

[Stefano Stabellini]

On Mon, 7 Mar 2011, Eamon Walsh wrote:
> On 03/04/2011 05:02 AM, Ian Campbell wrote:
> > On Thu, 2011-03-03 at 23:29 +0000, Eamon Walsh wrote:
> >> The function flexarray_vappend() will stop at the first NULL
> >> argument.  In libxl_device_vfb_add(), this has been observed
> >> to result in keys being added to the backend array without
> >> associated values in cases where the value can be NULL.
> > If these values are NULL should we be writing them at all? e.g. for:
> > 	flexarray_vappend(back, foo, bar);
> > where bar may be NULL shouldn't it become:
> > 	if (bar) 
> > 		flexarray_vappend(back, foo, bar);
> > or perhaps:
> > 	flexarray_vappend(back, foo, bar ? bar : "");
> > ?
> >
> 
> If the value is NULL, the key is skipped and not written.  This is because of a patch I submitted to change the xs_writev() function, which was calling strlen(NULL) previously.  See:
> http://lists.xensource.com/archives/html/xen-devel/2010-03/msg00703.html
> 
> However this behavior is not obvious.  Checking the value earlier and leaving it off the list makes sense.

I think me and Ian reached an agreement on the introduction of
flexarray_append_pair that would be very similar to flexarray_append but
takes two ptr arguments.
flexarray_append_pair would be used instead of flexarray_vappend in
libxl_device_vfb_add.
What do you think? Would you be up for writing the patch?