[Spice-devel] [PATCH spice-common 2/3] Write a small test to test possible crash

[Jonathon Jongsma]

On Mon, 2018-03-19 at 10:06 +0000, Frediano Ziglio wrote:
> This small test prove a that current generated demarshaller code
> is not safe to integer overflows leading to buffer overflows.
> Actually from a quick look at the protocol it seems that client
> can't cause these overflows but server can quite easily at
> demonstrated by this test.
> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  tests/Makefile.am     | 14 +++++++++
>  tests/test-overflow.c | 83
> +++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 97 insertions(+)
>  create mode 100644 tests/test-overflow.c
> 
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 5abf239..d5ec1d7 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -67,4 +67,18 @@ EXTRA_DIST =				\
>  	test-marshallers.proto		\
>  	$(NULL)
>  
> +TESTS += test_overflow
> +test_overflow_SOURCES = test-overflow.c
> +test_overflow_CFLAGS = \
> +	-I$(top_srcdir) \
> +	$(GLIB2_CFLAGS) \
> +	$(SPICE_COMMON_CFLAGS) \
> +	$(PROTOCOL_CFLAGS) \
> +	$(NULL)
> +test_overflow_LDADD = \
> +	$(top_builddir)/common/libspice-common.la \
> +	$(top_builddir)/common/libspice-common-server.la \
> +	$(top_builddir)/common/libspice-common-client.la \
> +	$(NULL)
> +
>  -include $(top_srcdir)/git.mk
> diff --git a/tests/test-overflow.c b/tests/test-overflow.c
> new file mode 100644
> index 0000000..0364df7
> --- /dev/null
> +++ b/tests/test-overflow.c
> @@ -0,0 +1,83 @@
> +/*
> +   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.1 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/>.
> +*/
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <assert.h>
> +
> +#include <common/marshaller.h>
> +#include <common/generated_server_marshallers.h>
> +#include <common/client_demarshallers.h>
> +
> +#define NUM_CHANNELS 3u
> +
> +int main(void)
> +{
> +    SpiceMarshaller *m;
> +    SpiceMsgChannels *msg;
> +    uint8_t *data, *out;
> +    size_t len;
> +    int to_free = 0;
> +    spice_parse_channel_func_t func;
> +    unsigned int max_message_type, n;
> +    message_destructor_t free_output;
> +
> +    m = spice_marshaller_new();
> +    assert(m);
> +
> +    msg = (SpiceMsgChannels *) malloc(sizeof(SpiceMsgChannels) +
> +          NUM_CHANNELS * sizeof(SpiceChannelId));
> +    assert(msg);
> +
> +    // build a message and marshal it
> +    msg->num_of_channels = NUM_CHANNELS;
> +    for (n = 0; n < NUM_CHANNELS; ++n) {
> +        msg->channels[n] = (SpiceChannelId) { n + 1, n * 7 };
> +    }
> +    spice_marshall_msg_main_channels_list(m, msg);
> +
> +    // get linear data
> +    data = spice_marshaller_linearize(m, 0, &len, &to_free);
> +    assert(data);
> +
> +    printf("output len %lu\n", (unsigned long) len);
> +
> +    // hack, try to core
> +    *((uint32_t *) data) = 0x80000002u;

I think this requires more explanation. something like

// hack: setting the number of channels in the marshalled message to a
value that will cause overflow while parsing the message to make sure
that the parser can handle this situation

Or something similar? (assuming my understanding of what you're doing
here is correct?)

> +
> +    // extract the message
> +    func = spice_get_server_channel_parser(1, &max_message_type);

1? maybe use SPICE_CHANNEL_MAIN instead?

> +    assert(func);
> +    out = func(data, data+len, SPICE_MSG_MAIN_CHANNELS_LIST, 0,
> &len, &free_output);
> +    assert(out == NULL);
> +
> +    // cleanup
> +    if (to_free) {
> +        free(data);
> +    }
> +    if (out) {
> +        free_output(out);
> +    }
> +    free(msg);
> +
> +    return 0;
> +}
> +