F-TEID unmarshalling and Enterprise IEs about go-pfcp HOT 7 CLOSED

Hi,

Sure, no problem. To respond to your points:

1/ The spec for octet 5 says:
"If this bit is set to "1", then the TEID, IPv4 address and IPv6 address fields shall not be present and the UP function shall assign an F-TEID with an IP4 or an IPv6 address if the V4 or V6 bit is set respectively."

So it is expected that the IPv4 bit is set yet there be no actual IP address is there. The bit in this case is not there to denote the presence of the IP address in subsequent bytes, but that the UPF needs to allocate an IPv4 address itself. The code currently increments offset by 4 bytes if CH is set and IPv4 is set.

As currently written, for example, 'f.ChooseID = b[offset:]' (line 235) is executed even if HasChId() is not set (but HasCh() is set instead, because of the || in the if statement). The offset will calculated wrong (because of the assumption above of there being an IP address present) then there is a likely slice index OOR panic.

2/ The payload length as recorded in the IE does indeed omit the first 4 octets, in the code at line 334 'i.Length' is indeed initially set to the the payload length. However in line 346 i.Length is semantically used as the 'length remaining' after whatever 'offset' is set to.

So I think the issue is really in line 346, being 'i.Payload = b[offset : offset+int(i.Length)]'. Increasing offset by 2 without reducing i.Length by 2 will result in the end index of that slice being out of bounds, ie beyond the end of the slice, so panics. In fact that slice assignment only works if offset is equal to 4 (since i.Length happens to be effectively initialised as len(b) - 4 already as that's what the encoded length hopefully is). Incrementing offset beyond 4, without reducing what i.Length holds, makes that i.Payload = assignment panic.

Thanks

from go-pfcp.

Sorry leaving this for so long! Please give me the pointer to why you think it's wrong makes it pretty much easier to investigate the issue, hopefully.

1:
Yup, the IP addresses are expected to be absent in that case, but it also checks the V4/V6 flags, which would be invalid if set. Do we have any real problem for that?

2:
The spec says the length excludes the first four octets, so I think we don't need to minus two.

Length: this field contains the length of the IE excluding the first four octets, which are common for all IEs 
(Type and Length) and is denoted "n" in Figure 8.1.1-1 and in Figure 8.1.1-2. Bit 8 of the lowest numbered octet 
is the most significant bit and bit 1 of the highest numbered octet is the least significant bit.

from go-pfcp.

I am also seeing a similar problem to (1). I am receiving a F-TEID with CHOOSE set, so no IP addresses present. The length field is therefore 1. However, in F-TEID.go line 208, it is checking for a minimum length of 2:

// UnmarshalBinary parses b into IE.
func (f *FTEIDFields) UnmarshalBinary(b []byte) error {
	l := len(b)
	**if l < 2 {**
		return io.ErrUnexpectedEOF
	}

IE as received:

F-TEID : 
    IE Type: F-TEID (21)
    IE Length: 1
    Flags: 0x04, CH (CHOOSE)
        0000 .... = Spare: 0
        .... 0... = CHID (CHOOSE_ID): False
        .... .1.. = CH (CHOOSE): True
        .... ..0. = V6 (IPv6): Not Present
        .... ...0 = V4 (IPv4): Not Present

from go-pfcp.

Fixed finally and added a new tag. Thank you for your report and patience :)

from go-pfcp.

Ah, no I haven’t addressed the second point(enterprise id). Let me take a look later.

from go-pfcp.

Fixed in #80.

from go-pfcp.

Let me know if you need anything additionally.

from go-pfcp.