Skip to content

Fix parsing of missing OPEN/UPDATE messages - #86

Merged
albertodainotti merged 1 commit into
CAIDA:masterfrom
alistairking:fix-missing-open
Apr 15, 2021
Merged

Fix parsing of missing OPEN/UPDATE messages#86
albertodainotti merged 1 commit into
CAIDA:masterfrom
alistairking:fix-missing-open

Conversation

@alistairking

Copy link
Copy Markdown
Member

It seems that FRR is sending a BGP message with the OPEN type set, but
no OPEN message
(https://github.com/FRRouting/frr/blob/8baa41e571f4a741c29116b35b28a8f7dff586f7/bgpd/bgp_bmp.c#L395)

It looked like this:

kafka read 205: 4F 42 4D 50 01 07 00 5D 00 00 00 70 80 0C 60 56 DA 59 00 0C C3 1D BF 4F 07 2F 3A E1 49 D2 34 86 02 4B C3 DA DF C4 00 0A 69 73 2D 63 63 2D 62 6D 70 31 66 AE ED E9 0D 37 1A 67 C6 D3 99 5D 02 38 10 21 C0 21 FF 1E 00 00 00 00 00 00 00 00 00 00 00 00 00 05 73 66 6D 69 78 00 00 00 01 03 00 00 00 70 03 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 CE C5 BB 01 00 00 2F F4 CE C5 BB 01 60 39 CF 54 00 0B 1D 38 00 00 00 00 00 00 00 00 00 00 00 00 CE C5 BB 1C B3 00 85 4F FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF 00 13 01 FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF 00 13 01 00 00 00 02 4C 47
Assertion failed: ((len) >= (nread)), function parse_params, file parsebgp_bgp_open.c, line 165.

From my reading of the RFC this is incorrect, but previously we would try and parse the subsequent bytes (in this case the next OPEN message that is part of the BMP peer up) as if they were part of the OPEN message.

Also applies the same length check to the UPDATE message parsing since
they apparently cannot be empty either.

@kenkeys I think we have a bit of a problem with these potential integer underflows I created with all the remain - nread and similar subtractions. I fixed a couple here, but probably we need to fix these all across the library :(

@dteach-rv (and @kenkeys) is my reading of the RFC correct? Or should we be allowing the missing OPEN message? Assuming I'm correct, perhaps we should let the FRR devs know?

It seems that FRR is sending a BGP message with the OPEN type set, but
no OPEN message
(https://github.com/FRRouting/frr/blob/8baa41e571f4a741c29116b35b28a8f7dff586f7/bgpd/bgp_bmp.c#L395)
From my reading of the RFC this is incorrect, but previously we would
try and parse the subsequent bytes as if they were part of the OPEN
message.
Also applies the same length check to the UPDATE message parsing since
they apparently cannot be empty either.
@digizeph
digizeph requested a review from kenkeysApril 8, 2021 22:52

@kenkeyskenkeys left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@albertodainotti
albertodainotti merged commit 5daad75 into CAIDA:masterApr 15, 2021
Sign up for freeto 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.

3 participants

@alistairking@kenkeys@albertodainotti