Skip to content
This repository has been archived by the owner on Apr 22, 2024. It is now read-only.

Length fields are not being checked #314

Open
erickvermot opened this issue Mar 20, 2017 · 4 comments
Open

Length fields are not being checked #314

erickvermot opened this issue Mar 20, 2017 · 4 comments

Comments

@erickvermot
Copy link
Contributor

length fields are not beeing checked.
(pg 50) - "OpenFlow is a simple binary protocol, therefore invalid OpenFlow messages
generated by OpenFlow implementations will in general result in the message
having the wrong length or some fields having invalid values (see 7.1.3). It
is recommended to monitor those conditions to detect non-compliant
implementations."

  • when length is passed as a parameter, as in flow_match, maybe length
    should be computed locally, or checked for validity before message is sent.
@erickvermot erickvermot added this to the 2017.1b1 milestone Mar 20, 2017
@erickvermot erickvermot self-assigned this Mar 20, 2017
@erickvermot
Copy link
Contributor Author

erickvermot commented Mar 20, 2017

I propose some simple length and bit checks on unpack methods like in the examples bellow. Do you guys want this kind of implementation? @diraol @beraldoleal ?

  • check Pad unpacking. Check if length is correct and bits are 0 simply checking if buffer slice equals result of pack() method.
    def unpack(self, buff, offset=0):
        if not buff[offset:offset + self._length] == self.pack():
            raise UnpackException
  • reimplement DPID unpacking. Check if buffer length is correct by trying to unpack the hole buffer slice. ( update: I realized now buffer is not beeing sliced prior to unpacking the attributes... So with this approach, my method may not work very wel...)
    old method:
    def unpack(self, buff, offset=0):
        begin = offset
        hexas = []
        while begin < offset + 8:
            number = struct.unpack("!B", buff[begin:begin + 1])[0]
            hexas.append("%.2x" % number)
            begin += 1
        self._value = ':'.join(hexas)

proposition:

    def unpack(self, buff, offset=0):
        hexa_values = struct.unpack("!8B", buff[offset:offset + 8])
        self._value = ':'.join(('{:02x}'.format(number)
                                for number in hexa_values))

should I continue making theese kind of changes? or is this not what you have in mind?

@erickvermot
Copy link
Contributor Author

@beraldoleal
Copy link
Member

beraldoleal commented Mar 21, 2017

I agree, we must check this validations.

However we have already an issue for a more open discussion, please see #96 .

We need to find a solution for a more generic implementation. I'm removing the milestone for now. But for sure we need to think this soon.

@beraldoleal beraldoleal removed this from the 2017.1b1 milestone Mar 21, 2017
@erickvermot
Copy link
Contributor Author

( update: I realized now buffer is not beeing sliced prior to unpacking the attributes... So with this approach, my method may not work very well...)
I'll leave this for now so we can discuss later.

@diraol diraol added this to the 2017.2 milestone Jun 20, 2017
@beraldoleal beraldoleal removed this from the 2017.2 milestone Dec 15, 2017
@hdiogenes hdiogenes changed the title length fields are not beeing checked. Length fields are not being checked Jun 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants