Fix PropExperimenter deserialization out-of-bounds read #66
Conversation
Signed-off-by: Aidar Fattakhov <[email protected]>
Signed-off-by: Aidar Fattakhov <[email protected]>
Signed-off-by: Aidar Fattakhov <[email protected]>
|
@wenyingd Could you take a look? |
| @@ -162,7 +162,7 @@ func (g *GroupMod) UnmarshalBinary(data []byte) (err error) { | |||
| g.CommandBucketId = binary.BigEndian.Uint32(data[n:]) | |||
| n += 4 | |||
|
|
|||
| for n < g.Header.Length { | |||
| for n < g.BucketArrayLen+24 { | |||
There was a problem hiding this comment.
this seems unrelated to the rest of the PR?
There was a problem hiding this comment.
Properties (including experimenter ones) are encoded after buckets. But because of n < g.Header.Length here we're trying to decode property as a bucket and fail.
There was a problem hiding this comment.
Not feel the change is necessary. GroupMod.Heander.Length should describe the size of this complete message including the buckets. If not, it may be some issue when marshaling the message, e.g., a bug of OVS to encoding the message, or a bug to read bytes from the OpenFlow connection.
There was a problem hiding this comment.
GroupMod.Header.Lengthshould describe the size of this complete message including the buckets
That is true, but because there can be data after the buckets this is not correct. In current code version we're trying to decode experimental fields as buckets and it fails.
| return n | ||
| } | ||
|
|
||
| func (p *PropExperimenter) MarshalBinary() (data []byte, err error) { | ||
| data = make([]byte, int(p.Len())) | ||
| p.Header.Length = 8 + uint16(len(p.Data)*4) | ||
| p.Header.Length = p.Header.Len() + 8 + uint16(len(p.Data)*4) |
There was a problem hiding this comment.
It is easier to use p.Header.Length = p.Len().
| @@ -162,7 +162,7 @@ func (g *GroupMod) UnmarshalBinary(data []byte) (err error) { | |||
| g.CommandBucketId = binary.BigEndian.Uint32(data[n:]) | |||
| n += 4 | |||
|
|
|||
| for n < g.Header.Length { | |||
| for n < g.BucketArrayLen+24 { | |||
There was a problem hiding this comment.
Not feel the change is necessary. GroupMod.Heander.Length should describe the size of this complete message including the buckets. If not, it may be some issue when marshaling the message, e.g., a bug of OVS to encoding the message, or a bug to read bytes from the OpenFlow connection.
Property example (from ovs, https://github.com/openvswitch/ovs/blob/dc7663f13ce73e69e2983af77a0342f216467b31/lib/ofp-group.c#L1210 )
[255 255 0 40 0 0 21 77 0 0 0 1 0 0 0 0 104 97 115 104 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0]