Re: [Qemu-devel] [PATCH for-1.4 01/12] qmp: Fix design bug and read beyond buffer in memchar-write

[Luiz Capitulino]

On Wed, 06 Feb 2013 10:06:03 +0100
Markus Armbruster <address@hidden> wrote:

> Markus Armbruster <address@hidden> writes:
> 
> > Eric Blake <address@hidden> writes:
> >
> >> On 02/05/2013 09:22 AM, Markus Armbruster wrote:
> >>> Command memchar-write takes data and size parameter.  Begs the
> >>> question what happens when data doesn't match size.
> >>> 
> >>> With format base64, qmp_memchar_write() copies the full data argument,
> >>> regardless of size argument.
> >>> 
> >>> With format utf8, qmp_memchar_write() copies size bytes from data,
> >>> happily reading beyond data.  Copies crap from the heap or even
> >>> crashes.
> >>> 
> >>> Drop the size parameter, and always copy the full data argument.
> >>> 
> >>> Signed-off-by: Markus Armbruster <address@hidden>
> >>> ---
> >>>  hmp.c            | 4 +---
> >>>  qapi-schema.json | 4 +---
> >>>  qemu-char.c      | 8 +++-----
> >>>  qmp-commands.hx  | 4 +---
> >>>  4 files changed, 6 insertions(+), 14 deletions(-)
> >>
> >>>      if (has_format && (format == DATA_FORMAT_BASE64)) {
> >>>          write_data = g_base64_decode(data, &write_count);
> >>>      } else {
> >>>          write_data = (uint8_t *)data;
> >>> +        write_count = strlen(data);
> >>>      }
> >>
> >> Obviously, base64 is the only way to write an embedded NUL.  But what
> >
> > Consider the JSON string "this \\u0000 is fun".  But our JSON parser
> > chokes on it, so we can ignore it until that's fixed.  See my "[PATCH]
> > check-qjson: More thorough testing of UTF-8 in strings", specifically
> > the comment right at the beginning of utf8_string().
> >
> >> happens if the user requests base64 encoding, but the utf8 string that
> >> got passed in through JSON is not a valid base64-encoded string?  Does
> >> g_base64_decode report an error in that case, and should you be handling
> >> the error here?
> >
> > Good question.  I wish it had occured to GLib developers:
> > http://developer.gnome.org/glib/stable/glib-Base64-Encoding.html#g-base64-decode
> >
> > Seriously, I need to find out what the heck g_base64_decode() does when
> > it's fed crap.  If it fails in a reliable and detectable manner, I need
> > to handle the failure.  If not, I need to replace the function.
> >
> > Moreover, I should document which characters outside the base64 alphabet
> > are silently ignored, if any.  All whitespace?  Just newlines?
> 
> As far as I can tell, it never fails, but silently ignores characters
> outside the alphabet [A-Za-z0-9+/], as well as unpadded suffixes.  Oh,
> and it does something weird when padding appears in the middle.
> Craptastic.
> 
> We can either document this behavior as a feature, or we replace the
> function by one that accepts only valid base64.  If we do the latter, we
> better specify the language we want to accept now, but I guess we could
> choose to delay the actual checking post 1.4.
> 
> There's another use of g_base64_decode() in qmp_guest_file_write().
> Which means guest agent command guest-file-write is similarly
> ill-defined.  Mike, anything to be done there?

Long time ago Jan warned us about this and he even had patches for
adding a base64 encoder/decoder to qemu.

I think we won't have other option if we want reliable base64
support.