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

Re: [Xen-devel] [RFC 02/19] xen: guestcopy: Provide an helper to copy string from guest

On 06/17/2014 05:23 AM, Julien Grall wrote:

On 17/06/14 10:17, Jan Beulich wrote:
On 17.06.14 at 11:09, <julien.grall@xxxxxxxxxx> wrote:
On 17/06/14 09:01, Jan Beulich wrote:
On 16.06.14 at 18:17, <julien.grall@xxxxxxxxxx> wrote:
+    /* Add an extra +1 to append \0. We can't assume the guest will
+     * provide a valid string */

Now this is the case for flask, but for a generic string copying
routine I don't think this is desirable. It seems especially wrong to
aid the guest with putting a NUL where none was. If you really
want this, I guess you would be better off adding two variants:
One which demands the string to be NUL-terminated (in which
case passing in a size is sort of bogus), and one which takes a
size and inserts a NUL.

I'm not sure why you would want a string copy-in function to not
NUL-terminate the strings it copies in.  If you don't want the strings
to be NUL-terminated at all, I would call it buffer copy-in function
(and copy_from_guest seems to cover buffer copy-in better).  If you want
the strings to be NUL-terminated and the guest has passed you a length,
it's simpler to have the hypervisor add the NUL instead of copying it
and then checking that it is there.  The current toolstack code for
XSM/FLASK relies on the hypervisor to add the NUL terminator, since it
often passes in (s, strlen(s)).

A malicious guest could pass a big buffer without a NUL-terminated. If
we don't limit the size and check the NUL-terminated character the guest
could respectively exhaust Xen memory and exploit it.

Therefore we can't rely on the guest to provide a valid string. This
solution will avoid to check in every caller that the string is
correctly terminated.

You seem to imply that by not passing in a size I also meant not
passing in a maximum size - I didn't say that, though. You absolutely
have to limit the string length for security reasons, but it's clearly a
difference whether you silently NUL-terminate the value after the
maximum number of characters, or return with an error.

I didn't understand in this way your previous mail. Thank you for the 

It looks like for my use case it's better to throw an error if we don't have 
enough place. It would help us if one day the path start to be very long.

I'm wondering if we can also make this change for flask... Daniel, any though?

Silently cropping the string at some maximum would be a problem for
FLASK (and probably other places), as it could result in a valid label
that has a different meaning than intended - better to return an error
and force the caller to deal with it.  Otherwise, I don't think there
is any change to be made here, unless I missed something?

Daniel De Graaf
National Security Agency

Xen-devel mailing list



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