[PATCH 1/3] fix code generation for emit_size_read() to check for buffer overflow

[Aleksander Morgado]

Hey Thomas,

emit_size_read() is only used during validate(), and therefore we can
safely add code to return FALSE (indicating message invalid) at any
point. So, instead of going on with the parsing, if we detect that the
we don't have enough bytes in the buffer to read the string/array
size-variable, we can just g_warning() and return an error.

See attached, patch; what do you think?



On Mon, Oct 6, 2014 at 3:15 PM, Thomas Haller <thaller at redhat.com> wrote:
> Code generation via emit_size_read() creates the _validate() functions.
> The generated code for strings and arrays used to read the length prefix
> without checking that the provided buffer is large enough.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1031738
>
> Reported-by: Florian Weimer <fweimer at redhat.com>
> Signed-off-by: Thomas Haller <thaller at redhat.com>
> ---
>  build-aux/qmi-codegen/VariableArray.py  | 22 ++++++++++++++--------
>  build-aux/qmi-codegen/VariableString.py | 22 ++++++++++++++--------
>  2 files changed, 28 insertions(+), 16 deletions(-)
>
> diff --git a/build-aux/qmi-codegen/VariableArray.py b/build-aux/qmi-codegen/VariableArray.py
> index c402da1..4a9f770 100644
> --- a/build-aux/qmi-codegen/VariableArray.py
> +++ b/build-aux/qmi-codegen/VariableArray.py
> @@ -235,7 +235,8 @@ class VariableArray(Variable):
>              translations['fixed_size'] = self.fixed_size
>
>              template = (
> -                '${lp}    guint16 ${common_var_prefix}_n_items = ${fixed_size};\n')
> +                '${lp}    {\n'
> +                '${lp}        guint16 ${common_var_prefix}_n_items = ${fixed_size};\n')
>              f.write(string.Template(template).substitute(translations))
>          else:
>              translations['array_size_element_format'] = self.array_size_element.public_format
> @@ -250,10 +251,14 @@ class VariableArray(Variable):
>
>              template = (
>                  '${lp}    ${array_size_element_format} ${common_var_prefix}_n_items;\n'
> -                '${lp}    const guint8 *${common_var_prefix}_aux_buffer = &${buffer_name}[${variable_name}];\n'
> -                '${lp}    guint16 ${common_var_prefix}_aux_buffer_len = ${buffer_len} - ${variable_name};\n'
>                  '\n'
> -                '${lp}    ${variable_name} += ${array_size_element_size};\n')
> +                '${lp}    if ((${variable_name} >= ${buffer_len}) || (${buffer_len} - ${variable_name} < ${array_size_element_size})) {\n'
> +                '${lp}        ${variable_name} += ${array_size_element_size};\n'
> +                '${lp}    } else {\n'
> +                '${lp}        const guint8 *${common_var_prefix}_aux_buffer = &${buffer_name}[${variable_name}];\n'
> +                '${lp}        guint16 ${common_var_prefix}_aux_buffer_len = ${buffer_len} - ${variable_name};\n'
> +                '\n'
> +                '${lp}        ${variable_name} += ${array_size_element_size};\n')
>
>              if self.array_sequence_element != '':
>                  if self.array_sequence_element.public_format == 'guint8':
> @@ -266,21 +271,22 @@ class VariableArray(Variable):
>                      translations['array_sequence_element_size'] = '0'
>                  template += (
>                      '\n'
> -                    '${lp}    ${variable_name} += ${array_sequence_element_size};\n')
> +                    '${lp}        ${variable_name} += ${array_sequence_element_size};\n')
>
>              f.write(string.Template(template).substitute(translations))
>
> -            self.array_size_element.emit_buffer_read(f, line_prefix + '    ', common_var_prefix + '_n_items', common_var_prefix + '_aux_buffer', common_var_prefix + '_aux_buffer_len')
> +            self.array_size_element.emit_buffer_read(f, line_prefix + '        ', common_var_prefix + '_n_items', common_var_prefix + '_aux_buffer', common_var_prefix + '_aux_buffer_len')
>
>          template = (
>              '\n'
> -            '${lp}    for (${common_var_prefix}_i = 0; ${common_var_prefix}_i < ${common_var_prefix}_n_items; ${common_var_prefix}_i++) {\n'
> +            '${lp}        for (${common_var_prefix}_i = 0; ${common_var_prefix}_i < ${common_var_prefix}_n_items; ${common_var_prefix}_i++) {\n'
>              '\n')
>          f.write(string.Template(template).substitute(translations))
>
> -        self.array_element.emit_size_read(f, line_prefix + '        ', variable_name, buffer_name, buffer_len)
> +        self.array_element.emit_size_read(f, line_prefix + '            ', variable_name, buffer_name, buffer_len)
>
>          template = (
> +            '${lp}        }\n'
>              '${lp}    }\n'
>              '${lp}}\n')
>          f.write(string.Template(template).substitute(translations))
> diff --git a/build-aux/qmi-codegen/VariableString.py b/build-aux/qmi-codegen/VariableString.py
> index faa2085..a813a6f 100644
> --- a/build-aux/qmi-codegen/VariableString.py
> +++ b/build-aux/qmi-codegen/VariableString.py
> @@ -120,21 +120,27 @@ class VariableString(Variable):
>          elif self.length_prefix_size == 8:
>              template = (
>                  '${lp}{\n'
> -                '${lp}    guint8 size8;\n'
> -                '${lp}    const guint8 *aux_buffer = &${buffer_name}[${variable_name}];\n'
> -                '${lp}    guint16 aux_buffer_len = ${buffer_len} - ${variable_name};\n'
> +                '${lp}    guint8 size8 = 0;\n'
>                  '\n'
> -                '${lp}    qmi_utils_read_guint8_from_buffer (&aux_buffer, &aux_buffer_len, &size8);\n'
> +                '${lp}    if (${variable_name} < ${buffer_len}) {\n'
> +                '${lp}        const guint8 *aux_buffer = &${buffer_name}[${variable_name}];\n'
> +                '${lp}        guint16 aux_buffer_len = ${buffer_len} - ${variable_name};\n'
> +                '\n'
> +                '${lp}        qmi_utils_read_guint8_from_buffer (&aux_buffer, &aux_buffer_len, &size8);\n'
> +                '${lp}    }\n'
>                  '${lp}    ${variable_name} += (1 + size8);\n'
>                  '${lp}}\n')
>          elif self.length_prefix_size == 16:
>              template = (
>                  '${lp}{\n'
> -                '${lp}    guint16 size16;\n'
> -                '${lp}    const guint8 *aux_buffer = &${buffer_name}[${variable_name}];\n'
> -                '${lp}    guint16 aux_buffer_len = ${buffer_len} - ${variable_name};\n'
> +                '${lp}    guint16 size16 = 0;\n'
> +                '\n'
> +                '${lp}    if (${variable_name} < ${buffer_len}) {\n'
> +                '${lp}        const guint8 *aux_buffer = &${buffer_name}[${variable_name}];\n'
> +                '${lp}        guint16 aux_buffer_len = ${buffer_len} - ${variable_name};\n'
>                  '\n'
> -                '${lp}    qmi_utils_read_guint16_from_buffer (&aux_buffer, &aux_buffer_len, QMI_ENDIAN_LITTLE, &size16);\n'
> +                '${lp}        qmi_utils_read_guint16_from_buffer (&aux_buffer, &aux_buffer_len, QMI_ENDIAN_LITTLE, &size16);\n'
> +                '${lp}    }\n'
>                  '${lp}    ${variable_name} += (2 + size16);\n'
>                  '${lp}}\n')
>          f.write(string.Template(template).substitute(translations))
> --
> 1.9.3
>



-- 
Aleksander
https://aleksander.es
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-qmi-codegen-ensure-enough-buffer-available-to-read-s.patch
Type: text/x-patch
Size: 4111 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/libqmi-devel/attachments/20141007/76678773/attachment.bin>