Re: [PATCH spice-common 1/3] Fix integer overflows computing sizes

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> On 22 Mar 2018, at 12:02, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
> 
>>> 
>>> 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.

Ah. Did not realize that. In that case

Acked-by: Christophe de Dinechin <dinechin@xxxxxxxxxx>	

And I’ll over-patch later.

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




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]