[spice-common,4/4] codegen: Check validity of array members

Submitted by Frediano Ziglio on Aug. 13, 2019, 4:56 p.m.

Details

Message ID 20190813165608.32249-4-fziglio@redhat.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Frediano Ziglio Aug. 13, 2019, 4:56 p.m.
Check that combination of fields for an array does not
lead to unsafe code.
check_valid method came from generate_c_declaration with
some more check and it's use in demarshaller to validate
the array if the structure is not generated.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
---
 python_modules/demarshal.py |  2 ++
 python_modules/ptypes.py    | 18 +++++++++++++++++-
 2 files changed, 19 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/python_modules/demarshal.py b/python_modules/demarshal.py
index acd4b6f..3736976 100644
--- a/python_modules/demarshal.py
+++ b/python_modules/demarshal.py
@@ -315,6 +315,8 @@  def write_validate_pointer_item(writer, container, item, scope, parent_scope, st
 def write_validate_array_item(writer, container, item, scope, parent_scope, start,
                               want_nw_size, want_mem_size, want_extra_size):
     array = item.type
+    if item.member:
+        array.check_valid(item.member)
     is_byte_size = False
     element_type = array.element_type
     if array.is_bytes_length():
diff --git a/python_modules/ptypes.py b/python_modules/ptypes.py
index 311ce3d..7a23bca 100644
--- a/python_modules/ptypes.py
+++ b/python_modules/ptypes.py
@@ -485,7 +485,23 @@  class ArrayType(Type):
     def c_type(self):
         return self.element_type.c_type()
 
+    def check_valid(self, member):
+        if member.has_attr("chunk") or member.has_attr("as_ptr"):
+            return
+        if member.has_attr("to_ptr") or member.has_attr("ptr_array"):
+            if not (self.is_identifier_length() or self.is_constant_length()):
+                raise Exception("Unsecure, no length of array")
+            return
+        if member.has_end_attr():
+            return
+        if self.is_remaining_length():
+            raise Exception('C output array is not allocated')
+        if self.is_constant_length() or self.is_identifier_length():
+            return
+        raise NotImplementedError('unknown array %s' % str(self))
+
     def generate_c_declaration(self, writer, member):
+        self.check_valid(member)
         name = member.name
         if member.has_attr("chunk"):
             return writer.writeln('SpiceChunks *%s;' % name)
@@ -497,7 +513,7 @@  class ArrayType(Type):
             return writer.writeln('%s *%s;' % (self.c_type(), name))
         if member.has_attr("ptr_array"):
             return writer.writeln('%s *%s[0];' % (self.c_type(), name))
-        if member.has_end_attr() or self.is_remaining_length():
+        if member.has_end_attr():
             return writer.writeln('%s %s[0];' % (self.c_type(), name))
         if self.is_constant_length():
             return writer.writeln('%s %s[%s];' % (self.c_type(), name, self.size))

Comments

On 8/13/19 7:56 PM, Frediano Ziglio wrote:
> Check that combination of fields for an array does not
> lead to unsafe code.
> check_valid method came from generate_c_declaration with
> some more check and it's use in demarshaller to validate
> the array if the structure is not generated.
> 
> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> ---
>   python_modules/demarshal.py |  2 ++
>   python_modules/ptypes.py    | 18 +++++++++++++++++-
>   2 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/python_modules/demarshal.py b/python_modules/demarshal.py
> index acd4b6f..3736976 100644
> --- a/python_modules/demarshal.py
> +++ b/python_modules/demarshal.py
> @@ -315,6 +315,8 @@ def write_validate_pointer_item(writer, container, item, scope, parent_scope, st
>   def write_validate_array_item(writer, container, item, scope, parent_scope, start,
>                                 want_nw_size, want_mem_size, want_extra_size):
>       array = item.type
> +    if item.member:
> +        array.check_valid(item.member)
>       is_byte_size = False
>       element_type = array.element_type
>       if array.is_bytes_length():
> diff --git a/python_modules/ptypes.py b/python_modules/ptypes.py
> index 311ce3d..7a23bca 100644
> --- a/python_modules/ptypes.py
> +++ b/python_modules/ptypes.py
> @@ -485,7 +485,23 @@ class ArrayType(Type):
>       def c_type(self):
>           return self.element_type.c_type()
>   

Would be nice to add a comment saying what checks are made.
(Even though there are not many comments
> +    def check_valid(self, member):
> +        if member.has_attr("chunk") or member.has_attr("as_ptr"):
> +            return
> +        if member.has_attr("to_ptr") or member.has_attr("ptr_array"):

Does 'to_ptr' have identifier/constant_length ?  It's just a pointer.


Uri.

> +            if not (self.is_identifier_length() or self.is_constant_length()):
> +                raise Exception("Unsecure, no length of array")
> +            return
> +        if member.has_end_attr():
> +            return
> +        if self.is_remaining_length():
> +            raise Exception('C output array is not allocated')
> +        if self.is_constant_length() or self.is_identifier_length():
> +            return
> +        raise NotImplementedError('unknown array %s' % str(self))
> +
>       def generate_c_declaration(self, writer, member):
> +        self.check_valid(member)
>           name = member.name
>           if member.has_attr("chunk"):
>               return writer.writeln('SpiceChunks *%s;' % name)
> @@ -497,7 +513,7 @@ class ArrayType(Type):
>               return writer.writeln('%s *%s;' % (self.c_type(), name))
>           if member.has_attr("ptr_array"):
>               return writer.writeln('%s *%s[0];' % (self.c_type(), name))
> -        if member.has_end_attr() or self.is_remaining_length():
> +        if member.has_end_attr():
>               return writer.writeln('%s %s[0];' % (self.c_type(), name))
>           if self.is_constant_length():
>               return writer.writeln('%s %s[%s];' % (self.c_type(), name, self.size))
>
> 
> On 8/13/19 7:56 PM, Frediano Ziglio wrote:
> > Check that combination of fields for an array does not
> > lead to unsafe code.
> > check_valid method came from generate_c_declaration with
> > some more check and it's use in demarshaller to validate
> > the array if the structure is not generated.
> > 
> > Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> > ---
> >   python_modules/demarshal.py |  2 ++
> >   python_modules/ptypes.py    | 18 +++++++++++++++++-
> >   2 files changed, 19 insertions(+), 1 deletion(-)
> > 
> > diff --git a/python_modules/demarshal.py b/python_modules/demarshal.py
> > index acd4b6f..3736976 100644
> > --- a/python_modules/demarshal.py
> > +++ b/python_modules/demarshal.py
> > @@ -315,6 +315,8 @@ def write_validate_pointer_item(writer, container,
> > item, scope, parent_scope, st
> >   def write_validate_array_item(writer, container, item, scope,
> >   parent_scope, start,
> >                                 want_nw_size, want_mem_size,
> >                                 want_extra_size):
> >       array = item.type
> > +    if item.member:
> > +        array.check_valid(item.member)
> >       is_byte_size = False
> >       element_type = array.element_type
> >       if array.is_bytes_length():
> > diff --git a/python_modules/ptypes.py b/python_modules/ptypes.py
> > index 311ce3d..7a23bca 100644
> > --- a/python_modules/ptypes.py
> > +++ b/python_modules/ptypes.py
> > @@ -485,7 +485,23 @@ class ArrayType(Type):
> >       def c_type(self):
> >           return self.element_type.c_type()
> >   
> 
> Would be nice to add a comment saying what checks are made.
> (Even though there are not many comments
> > +    def check_valid(self, member):
> > +        if member.has_attr("chunk") or member.has_attr("as_ptr"):
> > +            return
> > +        if member.has_attr("to_ptr") or member.has_attr("ptr_array"):
> 
> Does 'to_ptr' have identifier/constant_length ?  It's just a pointer.
> 

to_ptr means that the array is stored in the structure as a pointer.
The issue here is that the declaration is potentially buggy as there's
no way to know the length of the array so buffer overflow...
I'll explain the reasons adding comments

> 
> Uri.
> 
> > +            if not (self.is_identifier_length() or
> > self.is_constant_length()):
> > +                raise Exception("Unsecure, no length of array")
> > +            return
> > +        if member.has_end_attr():
> > +            return
> > +        if self.is_remaining_length():
> > +            raise Exception('C output array is not allocated')
> > +        if self.is_constant_length() or self.is_identifier_length():
> > +            return
> > +        raise NotImplementedError('unknown array %s' % str(self))
> > +
> >       def generate_c_declaration(self, writer, member):
> > +        self.check_valid(member)
> >           name = member.name
> >           if member.has_attr("chunk"):
> >               return writer.writeln('SpiceChunks *%s;' % name)
> > @@ -497,7 +513,7 @@ class ArrayType(Type):
> >               return writer.writeln('%s *%s;' % (self.c_type(), name))
> >           if member.has_attr("ptr_array"):
> >               return writer.writeln('%s *%s[0];' % (self.c_type(), name))
> > -        if member.has_end_attr() or self.is_remaining_length():
> > +        if member.has_end_attr():
> >               return writer.writeln('%s %s[0];' % (self.c_type(), name))
> >           if self.is_constant_length():
> >               return writer.writeln('%s %s[%s];' % (self.c_type(), name,
> >               self.size))
> > 
> 
>