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

[Eric Blake]

On 11/27/2015 09:30 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
> + * @errp: pointer to uninitialized error object

That almost implies that I could do:

Error *err;
qbase64_decode(,&err);

In reality, it should be the NULL-initialized error object, as in:

Error *err = NULL;

but I don't know if there is a better way to represent it in text.  At
any rate, that phrase exists elsewhere, so it would be easier to do a
tree-wide cleanup if we have a better terminology to use, and I won't
hold up review on this patch for it.

> + *
> + * Attempt to decode the (possibly) base64 encoded
> + * text provided in @input. If the @input text may
> + * contain embedded NUL characters, or may not be
> + * NUL terminated, then @in_len must be set to the
> + * known size of the @input buffer.
> + *
> + * Note that embedded NULs, or lack of a NUL terminator
> + * are considered invalid base64 data and errors
> + * will be reported to this effect.
> + *
> + * If decoding is successful, the decoded data will
> + * be returned and @out_len set to indicate the
> + * number of bytes in the decoded data.
> + *

Maybe mention that caller must g_free() the successful result?

> + * Returns: the decoded data or NULL
> + */
> +uint8_t *qbase64_decode(const char *input,
> +                        size_t in_len,
> +                        size_t *out_len,
> +                        Error **errp);
> +
> +

> +++ b/tests/test-base64.c
> @@ -0,0 +1,98 @@

> +static void test_base64_bad(const char *input,
> +                            size_t input_len)
> +{
> +    size_t len;
> +    Error *err = NULL;
> +    uint8_t *actual = qbase64_decode(input,
> +                                     input_len,
> +                                     &len,
> +                                     &err);
> +
> +    g_assert(err != NULL);

Could use &error_abort in the original call instead of a second check
for err != NULL; but that's cosmetic.

> +    g_assert(actual == NULL);
> +    g_assert_cmpint(len, ==, 0);

So you are testing that we initialize output length even on error,
rather than leaving it uninitialized.  That's fair.


> +
> +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);
> +}
> +

This asserts that you have a failure, but doesn't say what that failure
would be...

> +
> +static void test_base64_not_nul_terminated(void)
> +{
> +    char input[] = "There's no such thing as a free lunch.";
> +    input[G_N_ELEMENTS(input) - 1] = '!';
> +
> +    test_base64_bad(input, G_N_ELEMENTS(input) - 1);
> +}
> +
> +
> +static void test_base64_invalid_chars(void)
> +{
> +    const char *input = "There's no such thing as a free lunch.";
> +
> +    test_base64_bad(input, strlen(input));
> +}

...and this same input already fails because it doesn't match base64,
regardless of whether NUL bytes are mishandled.  I wonder if
test_base64_embedded_nul() and test_base64_not_nul_terminated() should
be using variations of your known-good base64 string
("QmVjYXVzZSB3ZSBmb2N1c2VkIG9uIHRoZSBzbmFrZSwgd2UgbW\n"
"lzc2VkIHRoZSBzY29ycGlvbi4="), to prove that they are failing purely
because of the NUL handling and not because of invalid base64 content.
But that's only a weak complaint - because by peering into the black
box, I see that you are fully testing all code paths (that is, the black
box checks for NUL abuse prior to checking for valid base64 data), so
I'm not going to insist on a respin.

Reviewed-by: Eric Blake <address@hidden>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature