[Spice-devel] [PATCH spice-common 1/3] Fix integer overflows computing sizes
Christophe de Dinechin
cdupontd at redhat.com
Thu Mar 22 11:03:21 UTC 2018
> On 22 Mar 2018, at 12:02, Frediano Ziglio <fziglio at redhat.com> wrote:
>
>>>
>>> On 21 Mar 2018, at 13:03, Frediano Ziglio <fziglio at redhat.com> wrote:
>>>
>>>>> On 20 Mar 2018, at 11:41, Frediano Ziglio <fziglio at redhat.com> wrote:
>>>>>
>>>>>>>
>>>>>>> On 19 Mar 2018, at 11:06, Frediano Ziglio <fziglio at redhat.com> 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 at redhat.com>
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 at redhat.com>
>>>>>>> ---
>>>>>>> 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()
More information about the Spice-devel
mailing list