fix: call validatePathAttributeFlags after parsing and setting path attribute length

[floatingstatic]

I recently started using gobgp package libraries to parse BMP messages from a third party source (BIRD). BIRD has a bug whereby the BMP messages it emits do not have the transitive flag set on well known path attributes on route monitoring messages. GoBGP correctly checks for this condition here. There is a subtle bug that shows up when parsing a BGP update message that fails this check which is that PathAttribute.DecodeFromBytes() exits early before it sets the PathAttribute.Length.

Highlighting this with an example this is a byte slice from an update pack after gobgp has stripped out everything from the packet byte slice leaving Path Attributes and NLRI:

Byte slice for the packet in the picture above looks like this (path attributes and NLRI is all that remains):

[0 1 1 0 64 2 6 2 1 0 0 253 232 0 3 4 10 0 2 100 0 4 4 0 0 0 98 0 5 4 0 0 0 50 24 198 51 100]

0 1 1 0 at the beginning of the slice is the origin attribute. 0 are the flags which is invalid given the transitive flag should be set but it is not. As a result of this in the current gobgp master branch we return an error but this does not exit the loop processing all path attributes (the function waits for the end and returns the strongest error. Instead we continue and slice the byte slice by the Len() function here:

gobgp/pkg/packet/bgp/bgp.go

Line 14358 in 1b975be

data = data[p.Len(options...):]

But if the Length attribute is not set we end up slicing by 3 + int(p.Length) (3 + 0) in my case instead of 4 which is incorrect:

gobgp/pkg/packet/bgp/bgp.go

Lines 10009 to 10014 in 1b975be

	func (p *PathAttribute) Len(options ...*MarshallingOption) int {
	if p.Flags&BGP_ATTR_FLAG_EXTENDED_LENGTH != 0 {
	return 4 + int(p.Length)
	}
	return 3 + int(p.Length)
	}

This means we cannot decode the next attribute because we have sliced incorrectly. The next attribute is unknown BGPAttrType(64) as the slice now looks like this:

[0 64 2 6 2 1 0 0 253 232 0 3 4 10 0 2 100 0 4 4 0 0 0 98 0 5 4 0 0 0 50 24 198 51 100]

In the above it thinks the flags for the next attribute is actually the type field

But should instead be:

[64 2 6 2 1 0 0 253 232 0 3 4 10 0 2 100 0 4 4 0 0 0 98 0 5 4 0 0 0 50 24 198 51 100]

This is easy to resolve by moving the path attribute flag validations a few lines down after we have recorded the length of a path attribute.

[floatingstatic]

Not sure why the unit test fails. I cannot reproduce the data race in local tests and seems unrelated to my change.

[fujita]

Thanks!