bgpd: Avoid cluster list attribute truncation#22081
Conversation
RFC 4271 4.3 says: The fourth high-order bit (bit 3) of the Attribute Flags octet is the Extended Length bit. It defines whether the Attribute Length is one octet (if set to 0) or two octets (if set to 1). The lower-order four bits of the Attribute Flags octet are unused. They MUST be zero when sent and MUST be ignored when received. If the Extended Length bit of the Attribute Flags octet is set to 0, the third octet of the Path Attribute contains the length of the attribute data in octets. If the Extended Length bit of the Attribute Flags octet is set to 1, the third and fourth octets of the path attribute contain the length of the attribute data in octets. Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
Greptile SummaryThis PR fixes a silent truncation bug in the BGP cluster list attribute encoding: when the total cluster list value exceeds 255 bytes, the old code called
Confidence Score: 5/5Safe to merge. The change is a targeted bug fix for a well-understood truncation path, follows an existing pattern already in the file, and the receive side already handles extended-length for all attributes. The fix is self-contained, touches only the cluster list attribute encoding branch, and mirrors the identical extended-length pattern already applied to communities, large communities, and ecommunities in the same function. The receiving parser's extended-length handling at the attribute-loop level means no receive-side change is needed, and the flag-validation code already masks out EXTLEN before comparing. No edge-case regressions were identified. No files require special attention. Important Files Changed
Reviews (1): Last reviewed commit: "bgpd: Avoid cluster list attribute trunc..." | Re-trigger Greptile |
|
@Mergifyio backport stable/10.6 stable/10.5 stable/10.4 stable/10.3 stable/10.2 |
✅ Backports have been createdDetails
|
| else | ||
| stream_put_in_addr(s, &bgp->router_id); | ||
| stream_putc(s, BGP_ATTR_FLAG_OPTIONAL); | ||
| stream_putc(s, BGP_ATTR_CLUSTER_LIST); |
There was a problem hiding this comment.
Why not move these two lines outside the if/else and remove lines 5730 and 5731
There was a problem hiding this comment.
That's valid, but IMO, this doesn't do anything with the relevant fix. We should then fix this together with BGP_ATTR_LARGE_COMMUNITIES, BGP_ATTR_COMMUNITIES, etc. (the same pattern).
bgpd: Avoid cluster list attribute truncation (backport #22081)
bgpd: Avoid cluster list attribute truncation (backport #22081)
bgpd: Avoid cluster list attribute truncation (backport #22081)
bgpd: Avoid cluster list attribute truncation (backport #22081)
bgpd: Avoid cluster list attribute truncation (backport #22081)
3 participants
No description provided.