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

[Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>]

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 
> explanation.
>
> 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
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel