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

[Thomas Haller]

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.

Thomas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.freedesktop.org/archives/libqmi-devel/attachments/20141008/d89616d3/attachment.sig>