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

[John Snow]

On 12/09/2015 09:08 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>
> ---
>  include/qemu/base64.h |  58 +++++++++++++++++++++++++++
>  tests/.gitignore      |   1 +
>  tests/Makefile        |   3 ++
>  tests/test-base64.c   | 109 
> ++++++++++++++++++++++++++++++++++++++++++++++++++
>  util/Makefile.objs    |   1 +
>  util/base64.c         |  60 +++++++++++++++++++++++++++
>  6 files changed, 232 insertions(+)
>  create mode 100644 include/qemu/base64.h
>  create mode 100644 tests/test-base64.c
>  create mode 100644 util/base64.c
> 
> diff --git a/include/qemu/base64.h b/include/qemu/base64.h
> new file mode 100644
> index 0000000..793708d
> --- /dev/null
> +++ b/include/qemu/base64.h
> @@ -0,0 +1,58 @@
> +/*
> + * QEMU base64 helpers
> + *
> + * Copyright (c) 2015 Red Hat, Inc.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see 
> <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#ifndef QEMU_BASE64_H__
> +#define QEMU_BASE64_H__
> +
> +#include "qemu-common.h"
> +
> +
> +/**
> + * 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 a NULL-initialized error object
> + *
> + * 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. The caller
> + * must use g_free() to free the returned data when
> + * it is no longer required.
> + *
> + * Returns: the decoded data or NULL
> + */
> +uint8_t *qbase64_decode(const char *input,
> +                        size_t in_len,
> +                        size_t *out_len,
> +                        Error **errp);
> +
> +
> +#endif /* QEMU_BUFFER_H__ */
> diff --git a/tests/.gitignore b/tests/.gitignore
> index 1e55722..00a7fed 100644
> --- a/tests/.gitignore
> +++ b/tests/.gitignore
> @@ -8,6 +8,7 @@ check-qom-interface
>  check-qom-proplist
>  rcutorture
>  test-aio
> +test-base64
>  test-bitops
>  test-blockjob-txn
>  test-coroutine
> diff --git a/tests/Makefile b/tests/Makefile
> index a1d03b4..7dc8437 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -84,6 +84,7 @@ check-unit-$(CONFIG_GNUTLS) += 
> tests/test-crypto-tlscredsx509$(EXESUF)
>  check-unit-$(CONFIG_GNUTLS) += tests/test-crypto-tlssession$(EXESUF)
>  check-unit-$(CONFIG_LINUX) += tests/test-qga$(EXESUF)
>  check-unit-y += tests/test-timed-average$(EXESUF)
> +check-unit-y += tests/test-base64$(EXESUF)
>  
>  check-block-$(CONFIG_POSIX) += tests/qemu-iotests-quick.sh
>  
> @@ -416,6 +417,8 @@ tests/test-vmstate$(EXESUF): tests/test-vmstate.o \
>       $(test-qom-obj-y)
>  tests/test-timed-average$(EXESUF): tests/test-timed-average.o qemu-timer.o \
>       $(test-util-obj-y)
> +tests/test-base64$(EXESUF): tests/test-base64.o \
> +     libqemuutil.a libqemustub.a
>  
>  tests/test-qapi-types.c tests/test-qapi-types.h :\
>  $(SRC_PATH)/tests/qapi-schema/qapi-schema-test.json 
> $(SRC_PATH)/scripts/qapi-types.py $(qapi-py)
> diff --git a/tests/test-base64.c b/tests/test-base64.c
> new file mode 100644
> index 0000000..f40eb94
> --- /dev/null
> +++ b/tests/test-base64.c
> @@ -0,0 +1,109 @@
> +/*
> + * QEMU base64 heper test
> + *

helper?

> + * Copyright (c) 2015 Red Hat, Inc.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see 
> <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#include <glib.h>
> +
> +#include "qemu/base64.h"
> +
> +
> +static void test_base64_good(void)
> +{
> +    const char input[] =
> +        "QmVjYXVzZSB3ZSBmb2N1c2VkIG9uIHRoZSBzbmFrZSwgd2UgbW\n"
> +        "lzc2VkIHRoZSBzY29ycGlvbi4=";
> +    const char expect[] = "Because we focused on the snake, "
> +        "we missed the scorpion.";
> +

:)

> +    size_t len;
> +    uint8_t *actual = qbase64_decode(input,
> +                                     -1,
> +                                     &len,
> +                                     &error_abort);
> +
> +    g_assert(actual != NULL);
> +    g_assert_cmpint(len, ==, strlen(expect));
> +    g_assert_cmpstr((char *)actual, ==, expect);
> +    g_free(actual);
> +}
> +
> +
> +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);
> +    g_assert(actual == NULL);
> +    g_assert_cmpint(len, ==, 0);
> +    error_free(err);
> +}
> +
> +
> +static void test_base64_embedded_nul(void)
> +{
> +    /* We put a NUL character in the middle of the base64
> +     * text which is invalid data, given the expected length */
> +    const char input[] =
> +        "QmVjYXVzZSB3ZSBmb2N1c2VkIG9uIHRoZSBzbmFrZSwgd2UgbW\0"
> +        "lzc2VkIHRoZSBzY29ycGlvbi4=";
> +
> +    test_base64_bad(input, G_N_ELEMENTS(input) - 1);
> +}
> +
> +
> +static void test_base64_not_nul_terminated(void)
> +{
> +    const char input[] =
> +        "QmVjYXVzZSB3ZSBmb2N1c2VkIG9uIHRoZSBzbmFrZSwgd2UgbW\n"
> +        "lzc2VkIHRoZSBzY29ycGlvbi4=";
> +
> +    /* Using '-2' to make us drop the trailing NUL, thus
> +     * creating an invalid base64 sequence for decoding */
> +    test_base64_bad(input, G_N_ELEMENTS(input) - 2);
> +}
> +
> +
> +static void test_base64_invalid_chars(void)
> +{
> +    /* We put a single quote character in the middle
> +     * of the base64 text which is invalid data */
> +    const char input[] =
> +        "QmVjYXVzZSB3ZSBmb2N1c2VkIG9uIHRoZSBzbmFrZSwgd2UgbW'"
> +        "lzc2VkIHRoZSBzY29ycGlvbi4=";
> +
> +    test_base64_bad(input, strlen(input));
> +}
> +
> +
> +int main(int argc, char **argv)
> +{
> +    g_test_init(&argc, &argv, NULL);
> +    g_test_add_func("/util/base64/good", test_base64_good);
> +    g_test_add_func("/util/base64/embedded-nul", test_base64_embedded_nul);
> +    g_test_add_func("/util/base64/not-nul-terminated",
> +                    test_base64_not_nul_terminated);
> +    g_test_add_func("/util/base64/invalid-chars", test_base64_invalid_chars);
> +    return g_test_run();
> +}
> diff --git a/util/Makefile.objs b/util/Makefile.objs
> index 89dd80e..8620a80 100644
> --- a/util/Makefile.objs
> +++ b/util/Makefile.objs
> @@ -30,3 +30,4 @@ util-obj-y += qemu-coroutine-sleep.o
>  util-obj-y += coroutine-$(CONFIG_COROUTINE_BACKEND).o
>  util-obj-y += buffer.o
>  util-obj-y += timed-average.o
> +util-obj-y += base64.o
> diff --git a/util/base64.c b/util/base64.c
> new file mode 100644
> index 0000000..f82caa7
> --- /dev/null
> +++ b/util/base64.c
> @@ -0,0 +1,60 @@
> +/*
> + * QEMU base64 helpers
> + *
> + * Copyright (c) 2015 Red Hat, Inc.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see 
> <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#include <config-host.h>
> +
> +#include "qemu/base64.h"
> +
> +static const char *base64_valid_chars =
> +    "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/=\n";
> +
> +uint8_t *qbase64_decode(const char *input,
> +                        size_t in_len,
> +                        size_t *out_len,
> +                        Error **errp)
> +{
> +    *out_len = 0;
> +
> +    if (in_len != -1) {
> +        /* Lack of NUL terminator is an error */
> +        if (input[in_len] != '\0') {
> +            error_setg(errp, "Base64 data is not NUL terminated");
> +            return NULL;
> +        }
> +        /* Check there's no NULs embedded since we expect
> +         * this to be valid base64 data */
> +        if (memchr(input, '\0', in_len) != NULL) {
> +            error_setg(errp, "Base64 data contains embedded NUL characters");
> +            return NULL;
> +        }
> +
> +        /* Now we know its a valid nul terminated string
> +         * strspn is safe to use... */
> +    } else {
> +        in_len = strlen(input);
> +    }
> +
> +    if (strspn(input, base64_valid_chars) != in_len) {
> +        error_setg(errp, "Base64 data contains invalid characters");
> +        return NULL;
> +    }
> +
> +    return g_base64_decode(input, out_len);
> +}
>