Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bgpd: handle fs nlri over 240 bytes #6242

Merged
merged 1 commit into from
Apr 22, 2020
+7 −2
Merged

bgpd: handle fs nlri over 240 bytes #6242

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions bgpd/bgp_flowspec.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ int bgp_nlri_parse_flowspec(struct peer *peer, struct attr *attr,
return BGP_NLRI_PARSE_ERROR_FLOWSPEC_IPV6_NOT_SUPPORTED;
}

if (packet->length >= FLOWSPEC_NLRI_SIZELIMIT) {
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need to validate this? I.e., check should stay but should just be larger (4095 instead of 240)?

Copy link
Member Author

Choose a reason for hiding this comment

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

this should be done as part of regular bgp processing ( I think in bgp_io.c, length is checked)

Copy link
Member

Choose a reason for hiding this comment

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

It's not checked there. That code checks that the message size is within the BGP message limits. NLRI checks are different and done in bgp_packet.c IIRC. I am asking the question because I want to make sure we are not introducing a security issue by removing a bounds check here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was talking about https://github.com/FRRouting/frr/blob/master/bgpd/bgp_io.c#L226
this said, there is a limit in rfc:
'
(0xfnnn). The highest value that can be represented with this
encoding is 4095. The value 241 is encoded as 0xf0f1.
'
so one can add a check instead.

if (packet->length >= FLOWSPEC_NLRI_SIZELIMIT_EXTENDED) {
flog_err(EC_BGP_FLOWSPEC_PACKET,
"BGP flowspec nlri length maximum reached (%u)",
packet->length);
Expand All @@ -124,7 +124,11 @@ int bgp_nlri_parse_flowspec(struct peer *peer, struct attr *attr,
return BGP_NLRI_PARSE_ERROR_PACKET_OVERFLOW;

psize = *pnt++;

if (psize >= FLOWSPEC_NLRI_SIZELIMIT) {
psize &= 0x0f;
psize = psize << 8;
psize |= *pnt++;
}
/* When packet overflow occur return immediately. */
if (pnt + psize > lim) {
flog_err(
Expand Down
1 change: 1 addition & 0 deletions bgpd/bgp_flowspec_private.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#define _FRR_BGP_FLOWSPEC_PRIVATE_H

#define FLOWSPEC_NLRI_SIZELIMIT 240
#define FLOWSPEC_NLRI_SIZELIMIT_EXTENDED 4095

/* Flowspec raffic action bit*/
#define FLOWSPEC_TRAFFIC_ACTION_TERMINAL 1
Expand Down