packet/bmp: bound the mirrored BGP message by its Route Mirroring TLV length - #3608
Conversation
… length BMPRouteMirrTLVBGPMsg.ParseValue handed the whole remainder of the Route Mirroring body to bgp.ParseBGPMessage instead of the Length octets the TLV declared. Every other value parser in this file slices data[:s.Length] first, so this one is the odd one out: the embedded BGP header's own length field, not the TLV framing, decided where the mirrored message ended. RFC 7854 Section 4.7 says the BGP Message TLV carries a complete BGP PDU, and that a monitoring station mirrors PDUs it may not have been able to process, so the length inside the mirrored message is not trustworthy. The enclosing loop still advances by the declared tl.Length, so a message announcing more octets than its TLV holds was decoded out of the TLVs that follow it, and those same octets were then parsed a second time as the next TLV. Concretely, a Route Mirroring body of a 19-octet BGP Message TLV whose embedded header claims 23 octets, followed by a 2-octet Information TLV, decoded without error into a ROUTE-REFRESH for AFI 1 / SAFI 2. Both the AFI and the SAFI came from the four header bytes of the Information TLV, which was then also returned as a second TLV of its own. Slicing the value to s.Length makes ParseBGPMessage see 19 octets for a message that declares 23 and reject it, which is what the wire actually contains. Test_TLV16RejectsShortLength already locks down the same "read past its own value into the following TLV" mistake for the fixed-width Termination Reason and Route Mirroring Information TLVs. Test_RouteMirroringBGPMsgTLVHonoursItsLength covers the BGP Message TLV and also asserts that a mirrored message that does fit inside its TLV still decodes, together with the TLV that follows it.
|
The zebra check is red on this PR and I don't believe it belongs to the change, so here is what I found rather than leaving it for whoever picks this up. The failure is The changed function cannot run in that container. What I cannot show you is the same job red on the base branch, and I want to be plain about that: zebra is green on the latest master run (35037455750) and on each of the previous nineteen pull requests I checked, so this reads as a one-off rather than a known-flaky job, and I have no reproduction of it. If it is worth settling, a re-run of the zebra job would do it — Found by a defect-hunting pipeline I build and run (Dev-next-gen), using Claude Code with Anthropic's Claude Opus 5. |
|
I said in my previous comment that I had no reproduction of the zebra failure. I have one now, and the mechanism is already named in the tree: the comment at The process stayed up, and the dial happened 23 ms after What I still cannot show is that this is what happened to g1 in run 35039726767. The harness sends gobgpd's log to the container's shared volume and the job never prints it, so the one line that would settle it is not in the log I can read. This is the only mechanism I found that produces an empty route list instead of a wrong one, and it is unrelated to this pull request either way. Found by a defect-hunting pipeline I build and run (Dev-next-gen), using Claude Code with Anthropic's Claude Opus 5. |
|
Thanks |
I was reading the Route Mirroring TLV parsers in
pkg/packet/bmp/bmp.goafter seeingTest_TLV16RejectsShortLength, which locks down the case where a fixed-width TLV reads past its own value into the following TLV. The BGP Message TLV turns out to have the same shape of mistake, just from the other direction.BMPRouteMirrTLVBGPMsg.ParseValuecallsbgp.ParseBGPMessage(data)on the whole remainder of the body. Every otherParseValuein the file slicesdata[:s.Length]first. BecauseParseBGPMessagetrusts the length field inside the message it is given, the embedded BGP header ends up deciding where the mirrored PDU ends, not the TLV that contains it. The enclosing loop still advances by the declaredtl.Length, so the bytes the over-read consumed are parsed a second time as the next TLV.RFC 7854 Section 4.7 is explicit that the BGP Message TLV carries a PDU the monitoring station is mirroring, including ones it could not process itself, so the length inside that PDU is exactly the field you cannot rely on. The TLV Length is the framing that matters.
Here is what I measured on master. A Route Mirroring body made of a BGP Message TLV with
Length = 19whose embedded header claims 23 octets, followed by an Information TLV (type = 1, length = 2, value = 0x0001):ParseBodyreturned no error. The mirrored ROUTE-REFRESH is a fabrication: its AFI and its SAFI are the four header bytes00 01 00 02of the Information TLV, which was then also returned as a TLV in its own right. Nineteen octets of value produced a 23-octet message.The change slices the value to
s.Lengthbefore parsing, with the same "value length is not enough" guard the sibling parsers use. With it,ParseBGPMessagesees 19 octets for a message that declares 23 and rejects it, which is what the wire actually holds.Test_RouteMirroringBGPMsgTLVHonoursItsLengthcovers that body, and also asserts the other half: a mirrored ROUTE-REFRESH that does fit inside its TLV still decodes, and the Information TLV after it is still parsed on its own. It fails on master at therequire.Errorand passes with the change.go test ./pkg/... ./internal/...is green, as aregofmt -landgo veton the package.Found by a defect-hunting pipeline I build and run (Dev-next-gen), using Claude Code with Anthropic's Claude Opus 5.