Re: [Qemu-devel] [PATCH v2 1/5] util: add base64 decoding function

[Daniel P. Berrange]

On Tue, Nov 24, 2015 at 08:54:24AM -0700, Eric Blake wrote:
> On 11/24/2015 08:02 AM, Daniel P. Berrange wrote:
> > The standard glib provided g_base64_decode doesn't provide any
> > kind of sensible error checking on its input. Add a QEMU custom
> > wrapper qbase64_decode which can be used with untrustworthy
> > input that can contain invalid base64 characters, embedded
> > NUL characters, or not be NUL terminated at all.
> > 
> > Signed-off-by: Daniel P. Berrange <address@hidden>
> > ---
> 
> > +/**
> > + * qbase64_decode:
> > + * @input: the (possibly) base64 encoded text
> > + * @in_len: length of @input or -1 if NUL terminated
> > + * @out_len: filled with length of decoded data
> > + * @errpr: pointer to uninitialized error object
> 
> s/errpr/errp/
> 
> > + * Returns: the decoded data or NULL
> > + */
> > +uint8_t *qbase64_decode(const char *input,
> > +                        size_t in_len,
> > +                        size_t *out_len,
> > +                        Error **errp);
> > +
> 
> Is char* any easier to work with than uint8_t* as the return type?  I'm
> fine with either, and actually like that uint8_t doesn't cause
> unintentional sign-extension on 8-bit input, but just want to make sure
> we aren't forcing the majority of our callers to cast back to a more
> convenient type.

I think using 'char *' encourages dangerous behaviour as you should
not assume that the decoded data is a NUL terminated string without
embedded NULs. It is arbirary binary data, which should be sanitized
before using as a regular C 'char *' string. So uint8 forces the
callers to thing about this. Fortunately all callers do currently
handle this correctly, since g_base64_decode also returns a uint8_t
equivalent type.

> > +
> > +static void test_base64_embedded_nul(void)
> > +{
> > +    const char input[] = "There's no such\0thing as a free lunch.";
> > +
> > +    test_base64_bad(input, G_N_ELEMENTS(input) - 1);
> > +}
> > +
> > +
> > +static void test_base64_not_nul_terminated(void)
> > +{
> > +    char input[] = "There's no such\0thing as a free lunch.";
> > +    input[G_N_ELEMENTS(input) - 1] = '!';
> > +
> > +    test_base64_bad(input, G_N_ELEMENTS(input) - 1);
> > +}
> > +
> 
> Did you mean to have an embedded NUL in the second test, or should you
> change that \0 to space?

Sigh yes, there shouldn't be the embedded NUL in the second test.


> > +#include "qemu/base64.h"
> > +
> > +static const char *base64_valid_chars =
> > +    "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/=";
> 
> Do we want to allow newlines (perhaps by adding a bool parameter)?
> After all, although newline is not valid in base64, it is the one
> character that GNU coreutils special-cases (produce on output every
> --wrap columns, ignore on input without needing --ignore-garbage) to
> make base64 blocks easier to read by breaking into lines rather than one
> long string - and which may be relevant if someone is pasting output
> from base64(1) into QMP.

I'll test that glib correctly decodes base64 with newlines. Assuming
it does then we should definitely allow it and extend the unit test
to cover it too.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|