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