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

[Aleksander Morgado]

On Wed, Oct 8, 2014 at 10:00 AM, Thomas Haller <thaller at redhat.com> wrote:
> On Tue, 2014-10-07 at 17:18 +0200, Aleksander Morgado wrote:
>> On Tue, Oct 7, 2014 at 1:41 PM, Aleksander Morgado
>> <aleksander at aleksander.es> wrote:
>> > On Tue, Oct 7, 2014 at 12:34 PM, Aleksander Morgado
>> > <aleksander at aleksander.es> wrote:
>> >> 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?
>> >
>> > Forget about the previous patch; this one looks better.
>> >
>> > Plus, I added a unit test to reproduce the issue. git master segfaults
>> > on the unit test, as it tries to read a guint16 from a buffer of 0
>> > bytes; while with the patch on we just get a warning and fully ignore
>> > the invalid TLV.
>>
>>
>> Some more context, that I just commented in IRC, and not sure Thomas got...
>
> Thanks, for some reason I didn't get it...
>
>
>> The code doesn't return a GError in validate(), just does a g_warning,
>> because we don't want to halt the full message parsing if we detect a
>> TLV with wrong contents. An incorrect TLV, or just a TLV which we
>> don't know how to parse, shouldn't make the parsing of other TLVs
>> break. validate() in this case is done for each TLV while we parse the
>> received message; and we do an initial parsing of the whole message by
>> default.
>>
>> Another, maybe better, approach would be to only parse those TLVs that
>> the user wants. i.e. the 'output' bundle would only fill in the parsed
>> fields when a given TLV is requested. And in that case, if a single
>> TLV is requested from the output bundle, we could then run validate()
>> and return a GError. IIRC this is more or less what I did in libmbim
>> actually, where the result of a client operation is not an already
>> parsed output bundle, but the actual message itself, and in that case
>> only the user-requested TLVs get parsed and built.
>
> The validation code uses g_warning() in case of failure. I think that is
> not a great way to report runtime errors/warnings. I think it's suitable
> to warn about bugs.
>
> One reason is that I find it useful to run applications with
> G_DEBUG=fatal-warnings. Which does not work if the application (or even
> one of it's libraries) uses g_warning for non-bugs.
>
> I think a library should not print anything (or possibly only when
> having a QMI_DEBUG variable defined).
>
> Anyway, never mind.
>
>
>> I'd suggest to leave the logic as is for now (for these bugs I mean),
>> but target the change for the next release. That change will not only
>> allow us to return errors if parsing a given TLV fails; it will also
>> end up speeding the logic a bit likely, as we won't be parsing the
>> full message always. even if the user doesn't want any TLV. Plus, I
>> don't think we would need any API change in libqmi-glib.
>
>
> Sure.
>
>
>
>
>
> I would add these two changes:
>
> diff --git a/build-aux/qmi-codegen/Field.py b/build-aux/qmi-codegen/Field.py
> index ddbcfe1..d424f1e 100644
> --- a/build-aux/qmi-codegen/Field.py
> +++ b/build-aux/qmi-codegen/Field.py
> @@ -335,6 +335,7 @@ class Field:
>              '    const guint8 *buffer,\n'
>              '    guint16 buffer_len)\n'
>              '{\n'
> +            '    G_STATIC_ASSERT (sizeof (guint) >= 4);\n'
>              '    guint expected_len = 0;\n'
>              '\n')
>          f.write(string.Template(template).substitute(translations))
> diff --git a/build-aux/qmi-codegen/VariableArray.py b/build-aux/qmi-codegen/VariableArray.py
> index 7f38202..c8a070f 100644
> --- a/build-aux/qmi-codegen/VariableArray.py
> +++ b/build-aux/qmi-codegen/VariableArray.py
> @@ -246,7 +246,7 @@ class VariableArray(Variable):
>              elif self.array_size_element.public_format == 'guint32':
>                  translations['array_size_element_size'] = '4'
>              else:
> -                translations['array_size_element_size'] = '0'
> +                raise Exception("Unknown array_size_element_format: %s" %s (self.array_size_element.public_format))
>
>              template = (
>                  '${lp}    ${array_size_element_format} ${common_var_prefix}_n_items;\n'
>
>
>
>
> otherwise v2 is fine!!
> Thank you.
>

Pushed the v2 patch and additional fixes for those suggestions

-- 
Aleksander
https://aleksander.es