> > 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@xxxxxxxxxx> > > --- > > 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 _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel