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

Ignore, done, was confused by the other test.

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