>
> > On 21 Mar 2018, at 13:03, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
> >
> >>> On 20 Mar 2018, at 11:41, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
> >>>
> >>>>>
> >>>>> On 19 Mar 2018, at 11:06, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
> >>>>>
> >>>>> Make code safe using both 32 and 64 bit machine.
> >>>>> Consider that this code can be compiled for machines with 32 bit.
> >>>>> There are some arrays length which are 32 bit.
> >>>>>
> >>>>> If size_t this can cause easily an overflow. For instance message_len
> >>>>> sending SPICE_MSG_NOTIFY messages are 32 bit and code add a small
> >>>>> constant (currently 24) before doing the test for size. Now passing
> >>>>> (uint32_t) -20 as message_len would lead to a size of 4 after the
> >>>>> addition. This overflow does not happen on 64 bit machine as the length
> >>>>> is converted to size_t.
> >>>>
> >>>> Why not use size_t instead of uint64_t then?
> >>>>
> >>>
> >>> A multiplication between 32 bit integer and a 32 bit integer can
> >>> cause overflow if the result is a 32 bit. Using a 64 bit integer
> >>> to multiply a 32 bit integer by a 32 bit integer avoids the overflow.
> >>> On 32 bit systems usually size_t is a 32 bit.
> >>
> >> You totally missed my point then.
> >>
> >
> > Well, from "Why not use size_t instead of uint64_t then?" is quite
> > hard to understand.
>
> The question was “why not use size_t”. I was expecting you to think long and
> hard about why you could not use size_t, not to explain to me the
> differences between 32-bit and 64-bit machines :-)
>
> The reason I asked this specific question is that if you have to do size
> computations using something else than size_t, its most often a sign that
> something is wrong. So you have to justify it. You will find below the
> detailed analysis that I expected you to do in response to my question (in
> particular after the “frob” hints).
>
> >
> >> The problem you identified with overflow is real, and must be fixed. But
> >> it’s
> >> an input validation problem, not an arithmetic problem.
> >>
> >> Consider:
> >>
> >> void *allocate_frobs(uint32_t n)
> >> {
> >> if (n > MAX_FROBS) // input validation
> >> return NULL;
> >> size_t size = n * sizeof(frob) + sizeof(header);
> >> return malloc(size);
> >> }
> >>
> >> This is better than:
> >>
> >> void *allocate_frobs(uint32_t n)
> >> {
> >> uint64_t size = n * sizeof(frob) + sizeof(header);
> >> if (size > UINT32_MAX)
> >> return NULL;
> >> return malloc(size);
> >> }
> >>
> >> And if you had the problem with a uint64_t as input, you would certainly
> >> not
> >> “fix” it with
> >>
> >> void *allocate_frobs(uint64_t n)
> >> {
> >> double_or_uint128_t size = n * sizeof(frob) + sizeof(header);
> >> if (size > UINT64_MAX)
> >> return NULL;
> >> return malloc(size);
> >> }
> >>
> >
> > There's no such case in our protocol. Not sure if this is checked by the
> > Python code (I think it should).
>
> It’s not an exact match, but it’s really close.
>
> If you look at the generated code in generated_clients_demarshallers.c, the
> first function where your changes really matter is parse_msg_notify, where
> there is a 32-bit length value being read. It looks like this (removing
> irrelevant code):
>
> static uint8_t * parse_msg_notify(uint8_t *message_start, uint8_t
> *message_end, SPICE_GNUC_UNUSED int minor, size_t *size,
> message_destructor_t *free_message)
> {
> #ifndef FREDIANO
> size_t nw_size;
> size_t mem_size;
> size_t message__nw_size, message__mem_size;
> uint32_t message__nelements;
> #else /* FREDIANO */
> uint64_t nw_size;
> uint64_t mem_size;
> uint64_t message__nw_size, message__mem_size;
> uint64_t message__nelements;
> #endif /* FREDIANO */
>
> { /* message */
> uint32_t message_len__value;
> message_len__value = read_uint32(pos);
> message__nelements = message_len__value;
>
> message__nw_size = message__nelements;
> message__mem_size = sizeof(uint8_t) * message__nelements;
> }
>
> nw_size = 24 + message__nw_size;
> mem_size = sizeof(SpiceMsgNotify) + message__mem_size;
>
> /* Check if message fits in reported side */
> #ifndef FREDIANO
> if (start + nw_size > message_end) {
> #else /* FREDIANO */
> if (nw_size > (uintptr_t) (message_end - start)) {
> #endif /* FREDIANO */
> return NULL;
> }
>
> /* Validated extents and calculated size */
> #ifndef FREDIANO
> data = (uint8_t *)malloc(mem_size);
> #else /* FREDIANO */
> data = (uint8_t *)(mem_size > UINT32_MAX ? NULL : malloc(mem_size));
> #endif /* FREDIANO */
> }
>
> So we have the core elements of my short “frob” examples, i.e. a size
> computation with a multiplication, an input validation that already exists,
> and then a malloc. It’s really not that different from my frob example, only
> longer.
>
> What I am pointing out is that the overflow in that case happens because of a
> missing input validation in the part commented as /* message */. This is
> where you need an additional input validation, so that you have changes that
> look more like:
>
> size_t nw_size;
> size_t mem_size;
> size_t message__nw_size, message__mem_size;
> uint32_t message__nelements;
>
> { /* message */
> uint32_t message_len__value;
> message_len__value = read_uint32(pos);
> message__nelements = message_len__value;
> #ifdef C3D
> if (SPICE_UNLIKELY(message__nelements) > SPICE_OVERFLOW_PROTECTION) {
> goto error;
> }
> #endif
>
> message__nw_size = message__nelements;
> message__mem_size = sizeof(uint8_t) * message__nelements;
> }
>
> nw_size = 24 + message__nw_size;
> mem_size = sizeof(SpiceMsgNotify) + message__mem_size;
>
> /* Check if message fits in reported side */
> if (start + nw_size > message_end) {
> return NULL;
> }
>
> data = (uint8_t *)malloc(mem_size);
> }
>
> where you set SPICE_OVERFLOW_PROTECTION to be small enough to detect invalid
> messages and avoid any overflow during the following computations.
>
> You could generate it automatically, e.g. by checking what you multiply by
> and dividing UINT32_MAX by that. However, I would strongly advise against
> it. I would much rather have a specific “max” value for SPICE that not only
> blocks any risk of buffer overrun, but also detects “smaller” message
> corruptions. You don’t need your message len to be 0x80000002 to know that
> the message is bogus. Having 10MB in a “notify” message is already quite
> suspect, and letting values higher larger than that pass through could
> easily cause a 32-bit machine to run out of memory or run into other kinds
> of DOS-style behaviors.
>
> >
> >> Once you do the input validation on the incoming uint32_t, the rest of the
> >> computations should be done as size_t, i.e. 32-bit on a 32-bit machine.
> >>
> >
> > The problem is that there's no MAX_FROBS or better MAX_FROBS == UINT32_MAX.
>
> So let’s invent one. It’s a one-line addition of a constant to the SPICE
> protocol. If you are wary of changing the protocol, you could simply
> document that as an implementation-defined limit.
>
> > The protocol mandate that the array is contained in the message so at the
> > end
> > this patch make sure this not requiring additional protocol specification
> > and
> > the behaviour is coherent between 32 and 64 bit machines.
>
> I believe that the additional protocol specification, e.g. stating that no
> message has more than 10M elements, would be worth the additional protection
> (not to mention not having to force 32-bit machines to do more expensive
> 64-bit computations). But as I said, if you don’t want this, you can simply
> make it an implementation limit. Actually, you could make that limit
> dependent on whether the machine is 32-bit or 64-bit.
>
No problem but I have no time to do it now, the patch was written more than
2 years and a half ago to fix a security issue that got a CVE, so feel
free to provide patches.
Frediano
> >
> >>>
> >>>>>
> >>>>> There are also some array length where some item are bigger than 1
> >>>>> byte.
> >>>>> For instance SPICE_MAIN_CHANNELS_LIST message have a number of channels
> >>>>> and each channel is composed by 2 bytes. Now the code generated try to
> >>>>> do
> >>>>> length * 2 where length is still a 32 bit so if we put a value like
> >>>>> 0x80000002u we get 4 as length. This will cause an overflow as code
> >>>>> will
> >>>>> allocate very few bytes but try to fill with a huge number of elements.
> >>>>> This overflow happen in both 32 and 64 bit machine.
> >>>>>
> >>>>> To avoid all these possible overflows this patch use only 64 bit for
> >>>>> nelements (number of elements), nw_size (network size) and mem_size
> >>>>> (memory size needed) checking the sizes to avoid other overflows
> >>>>> (like pointers conversions under 32 bit machines).
> >>>>>
> >>>>> Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> >>>>> ---
> >>>>> python_modules/demarshal.py | 38 +++++++++++++++++++-------------------
> >>>>> 1 file changed, 19 insertions(+), 19 deletions(-)
> >>>>>
> >>>>> diff --git a/python_modules/demarshal.py b/python_modules/demarshal.py
> >>>>> index da87d44..7e73985 100644
> >>>>> --- a/python_modules/demarshal.py
> >>>>> +++ b/python_modules/demarshal.py
> >>>>> @@ -101,7 +101,7 @@ def write_parser_helpers(writer):
> >>>>> writer.variable_def("uint64_t", "offset")
> >>>>> writer.variable_def("parse_func_t", "parse")
> >>>>> writer.variable_def("void **", "dest")
> >>>>> - writer.variable_def("uint32_t", "nelements")
> >>>>> + writer.variable_def("uint64_t", "nelements")
> >>>>> writer.end_block(semicolon=True)
> >>>>>
> >>>>> def write_read_primitive(writer, start, container, name, scope):
> >>>>> @@ -186,7 +186,7 @@ def write_validate_switch_member(writer, mprefix,
> >>>>> container, switch_member, scop
> >>>>>
> >>>>> all_as_extra_size = m.is_extra_size() and want_extra_size
> >>>>> if not want_mem_size and all_as_extra_size and not
> >>>>> scope.variable_defined(item.mem_size()):
> >>>>> - scope.variable_def("uint32_t", item.mem_size())
> >>>>> + scope.variable_def("uint64_t", item.mem_size())
> >>>>>
> >>>>> sub_want_mem_size = want_mem_size or all_as_extra_size
> >>>>> sub_want_extra_size = want_extra_size and not
> >>>>> all_as_extra_size
> >>>>> @@ -219,7 +219,7 @@ def write_validate_struct_function(writer, struct):
> >>>>> scope = writer.function(validate_function, "static intptr_t",
> >>>>> "uint8_t
> >>>>> *message_start, uint8_t *message_end, uint64_t offset,
> >>>>> SPICE_GNUC_UNUSED int minor")
> >>>>> scope.variable_def("uint8_t *", "start = message_start + offset")
> >>>>> scope.variable_def("SPICE_GNUC_UNUSED uint8_t *", "pos")
> >>>>> - scope.variable_def("size_t", "mem_size", "nw_size")
> >>>>> + scope.variable_def("uint64_t", "mem_size", "nw_size")
> >>>>> num_pointers = struct.get_num_pointers()
> >>>>> if num_pointers != 0:
> >>>>> scope.variable_def("SPICE_GNUC_UNUSED intptr_t", "ptr_size")
> >>>>> @@ -236,7 +236,7 @@ def write_validate_struct_function(writer, struct):
> >>>>>
> >>>>> writer.newline()
> >>>>> writer.comment("Check if struct fits in reported side").newline()
> >>>>> - writer.error_check("start + nw_size > message_end")
> >>>>> + writer.error_check("nw_size > (uintptr_t) (message_end - start)")
> >>>>>
> >>>>> writer.statement("return mem_size")
> >>>>>
> >>>>> @@ -264,26 +264,26 @@ def write_validate_pointer_item(writer,
> >>>>> container,
> >>>>> item, scope, parent_scope, st
> >>>>> # if array, need no function check
> >>>>>
> >>>>> if target_type.is_array():
> >>>>> - writer.error_check("message_start + %s >= message_end" %
> >>>>> v)
> >>>>> + writer.error_check("%s >= (uintptr_t) (message_end -
> >>>>> message_start)" % v)
> >>>>>
> >>>>>
> >>>>> assert target_type.element_type.is_primitive()
> >>>>>
> >>>>> array_item = ItemInfo(target_type, "%s__array" % item.prefix,
> >>>>> start)
> >>>>> - scope.variable_def("uint32_t", array_item.nw_size())
> >>>>> + scope.variable_def("uint64_t", array_item.nw_size())
> >>>>> # don't create a variable that isn't used, fixes
> >>>>> -Werror=unused-but-set-variable
> >>>>> need_mem_size = want_mem_size or (
> >>>>> want_extra_size and not item.member.has_attr("chunk")
> >>>>> and not target_type.is_cstring_length())
> >>>>> if need_mem_size:
> >>>>> - scope.variable_def("uint32_t", array_item.mem_size())
> >>>>> + scope.variable_def("uint64_t", array_item.mem_size())
> >>>>> if target_type.is_cstring_length():
> >>>>> writer.assign(array_item.nw_size(), "spice_strnlen((char
> >>>>> *)message_start + %s, message_end - (message_start +
> >>>>> %s))"
> >>>>> % (v, v))
> >>>>> writer.error_check("*(message_start + %s + %s) != 0" %
> >>>>> (v,
> >>>>> array_item.nw_size()))
> >>>>> else:
> >>>>> write_validate_array_item(writer, container, array_item,
> >>>>> scope, parent_scope, start,
> >>>>> True,
> >>>>> want_mem_size=need_mem_size,
> >>>>> want_extra_size=False)
> >>>>> - writer.error_check("message_start + %s + %s >
> >>>>> message_end"
> >>>>> % (v, array_item.nw_size()))
> >>>>> + writer.error_check("%s + %s > (uintptr_t) (message_end
> >>>>> -
> >>>>> message_start)" % (v, array_item.nw_size()))
> >>>>>
> >>>>> if want_extra_size:
> >>>>> if item.member and item.member.has_attr("chunk"):
> >>>>> @@ -321,11 +321,11 @@ def write_validate_array_item(writer, container,
> >>>>> item, scope, parent_scope, star
> >>>>> nelements = "%s__nbytes" %(item.prefix)
> >>>>> real_nelements = "%s__nelements" %(item.prefix)
> >>>>> if not parent_scope.variable_defined(real_nelements):
> >>>>> - parent_scope.variable_def("uint32_t", real_nelements)
> >>>>> + parent_scope.variable_def("uint64_t", real_nelements)
> >>>>> else:
> >>>>> nelements = "%s__nelements" %(item.prefix)
> >>>>> if not parent_scope.variable_defined(nelements):
> >>>>> - parent_scope.variable_def("uint32_t", nelements)
> >>>>> + parent_scope.variable_def("uint64_t", nelements)
> >>>>>
> >>>>> if array.is_constant_length():
> >>>>> writer.assign(nelements, array.size)
> >>>>> @@ -420,10 +420,10 @@ def write_validate_array_item(writer, container,
> >>>>> item, scope, parent_scope, star
> >>>>> element_nw_size = element_item.nw_size()
> >>>>> element_mem_size = element_item.mem_size()
> >>>>> element_extra_size = element_item.extra_size()
> >>>>> - scope.variable_def("uint32_t", element_nw_size)
> >>>>> - scope.variable_def("uint32_t", element_mem_size)
> >>>>> + scope.variable_def("uint64_t", element_nw_size)
> >>>>> + scope.variable_def("uint64_t", element_mem_size)
> >>>>> if want_extra_size:
> >>>>> - scope.variable_def("uint32_t", element_extra_size)
> >>>>> + scope.variable_def("uint64_t", element_extra_size)
> >>>>>
> >>>>> if want_nw_size:
> >>>>> writer.assign(nw_size, 0)
> >>>>> @@ -556,7 +556,7 @@ def write_validate_container(writer, prefix,
> >>>>> container,
> >>>>> start, parent_scope, wan
> >>>>> sub_want_nw_size = want_nw_size and not m.is_fixed_nw_size()
> >>>>> sub_want_mem_size = m.is_extra_size() and want_mem_size
> >>>>> sub_want_extra_size = not m.is_extra_size() and
> >>>>> m.contains_extra_size()
> >>>>> - defs = ["size_t"]
> >>>>> + defs = ["uint64_t"]
> >>>>> name = prefix_m(prefix, m)
> >>>>> if sub_want_nw_size:
> >>>>>
> >>>>> @@ -697,7 +697,7 @@ def read_array_len(writer, prefix, array, dest,
> >>>>> scope,
> >>>>> is_ptr):
> >>>>> if dest.is_toplevel() and scope.variable_defined(nelements):
> >>>>> return nelements # Already there for toplevel, need not
> >>>>> recalculate
> >>>>> element_type = array.element_type
> >>>>> - scope.variable_def("uint32_t", nelements)
> >>>>> + scope.variable_def("uint64_t", nelements)
> >>>>> if array.is_constant_length():
> >>>>> writer.assign(nelements, array.size)
> >>>>> elif array.is_identifier_length():
> >>>>> @@ -1053,9 +1053,9 @@ def write_msg_parser(writer, message):
> >>>>> parent_scope.variable_def("SPICE_GNUC_UNUSED uint8_t *", "pos")
> >>>>> parent_scope.variable_def("uint8_t *", "start = message_start")
> >>>>> parent_scope.variable_def("uint8_t *", "data = NULL")
> >>>>> - parent_scope.variable_def("size_t", "nw_size")
> >>>>> + parent_scope.variable_def("uint64_t", "nw_size")
> >>>>> if want_mem_size:
> >>>>> - parent_scope.variable_def("size_t", "mem_size")
> >>>>> + parent_scope.variable_def("uint64_t", "mem_size")
> >>>>> if not message.has_attr("nocopy"):
> >>>>> parent_scope.variable_def("uint8_t *", "in", "end")
> >>>>> num_pointers = message.get_num_pointers()
> >>>>> @@ -1073,7 +1073,7 @@ def write_msg_parser(writer, message):
> >>>>> writer.newline()
> >>>>>
> >>>>> writer.comment("Check if message fits in reported side").newline()
> >>>>> - with writer.block("if (start + nw_size > message_end)"):
> >>>>> + with writer.block("if (nw_size > (uintptr_t) (message_end -
> >>>>> start))"):
> >>>>> writer.statement("return NULL")
> >>>>>
> >>>>> writer.newline().comment("Validated extents and calculated
> >>>>> size").newline()
> >>>>> @@ -1084,7 +1084,7 @@ def write_msg_parser(writer, message):
> >>>>> writer.assign("*size", "message_end - message_start")
> >>>>> writer.assign("*free_message", "nofree")
> >>>>> else:
> >>>>> - writer.assign("data", "(uint8_t *)malloc(mem_size)")
> >>>>> + writer.assign("data", "(uint8_t *)(mem_size > UINT32_MAX ?
> >>>>> NULL
> >>>>> :
> >>>>> malloc(mem_size))")
> >>>>> writer.error_check("data == NULL")
> >>>>> writer.assign("end", "data + %s" % (msg_sizeof))
> >>>>> writer.assign("in", "start").newline()
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel