Skip to content

Conversation

@fujita
Copy link
Member

@fujita fujita commented Feb 7, 2025

No description provided.

ivg added 4 commits February 7, 2025 10:10
 1 func (m *BGP4MPHeader) decodeFromBytes(data []byte) ([]byte, error) {
 2     if m.isAS4 && len(data) < 8 {
 3         return nil, errors.New("not all BGP4MPMessageAS4 bytes available")
 4     } else if !m.isAS4 && len(data) < 4 {
 5         return nil, errors.New("not all BGP4MPMessageAS bytes available")
 6     }
 7
 8     if m.isAS4 {
 9         m.PeerAS = binary.BigEndian.Uint32(data[:4])
10         m.LocalAS = binary.BigEndian.Uint32(data[4:8])
11         data = data[8:]
12     } else {
13         m.PeerAS = uint32(binary.BigEndian.Uint16(data[:2]))
14         m.LocalAS = uint32(binary.BigEndian.Uint16(data[2:4]))
15         data = data[4:]
16     }
17     m.InterfaceIndex = binary.BigEndian.Uint16(data[:2])
18     m.AddressFamily = binary.BigEndian.Uint16(data[2:4])
19     switch m.AddressFamily {
20     case bgp.AFI_IP:
21         m.PeerIpAddress = net.IP(data[4:8]).To4()
22         m.LocalIpAddress = net.IP(data[8:12]).To4()
23         data = data[12:]
24     case bgp.AFI_IP6:
25         m.PeerIpAddress = net.IP(data[4:20])
26         m.LocalIpAddress = net.IP(data[20:36])
27         data = data[36:]
28     default:
29         return nil, fmt.Errorf("unsupported address family: %d",
m.AddressFamily)
30     }
31     return data, nil
32 }

The check  at lines 2-6 is sufficient only to safely extract `PeerAS`
and `LocalAS`, after that slice is rebound to the leftover bytes,
i.e., the length of the slice is not 8 or 4 bytes less, depending on
the it is AS4 or not. That means that it could be empty, therefore any
line beyond 16 is vulnerable. E.g., we need to check for at least 4
bytes for lines 17 and 18, and then depending on the address family,
for 12 or 36 bytes.
case EC_SUBTYPE_FLOWSPEC_REDIRECT_IP6:
   ipv6 := net.IP(data[2:18]).String()
   localAdmin := binary.BigEndian.Uint16(data[18:20])
   return NewRedirectIPv6AddressSpecificExtended(ipv6, localAdmin), nil

Note that the `data` length is only checked for being at least 8
bytes, so any message with the given subtype but less than 20 bytes
will crash the application.
…length

func (c *CapSoftwareVersion) DecodeFromBytes(data []byte) error {
c.DefaultParameterCapability.DecodeFromBytes(data)
data = data[2:]
if len(data) < 2 {
return NewMessageError(BGP_ERROR_OPEN_MESSAGE_ERROR, BGP_ERROR_SUB_UNSUPPORTED_CAPABILITY, nil, "Not all CapabilitySoftwareVersion bytes allowed")
}
softwareVersionLen := uint8(data[0])
if len(data[1:]) < int(softwareVersionLen) || softwareVersionLen > 64 {
return NewMessageError(BGP_ERROR_OPEN_MESSAGE_ERROR, BGP_ERROR_SUB_UNSUPPORTED_CAPABILITY, nil, "invalid length of software version capablity")
}
c.SoftwareVersionLen = softwareVersionLen
c.SoftwareVersion = string(data[1:c.SoftwareVersionLen]) // ivg: note the crash is here
return nil
}

Notice that `softwareVersionLen` is not checked for `0`, so
`data[1:c.SoftwareVersionLen]` becomes `data[1:0]`, which leads to a
runtime panic.
@fujita fujita merged commit 08a001e into osrg:master Feb 7, 2025
39 checks passed
@fujita fujita deleted the fix-parser branch February 7, 2025 01:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants