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

[Frediano Ziglio]

> 
> 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
> 

Sounds perfect.

> 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?
> 

This is a specific test protocol, is in tests/test-marshallers.proto,
I don't generate the enumerations so I don't have the mnemonic constant.

I could use a "#define SPICE_CHANNEL_MAIN 1" at the beginning of the C file.

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

Frediano