bgpd: handle fs nlri over 240 bytes #6242
Conversation
Continuous Integration Result: SUCCESSFULContinuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-11909/ This is a comment from an automated CI system. Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: |
Outdated results 🚧Basic BGPD CI results: Partial FAILURE, 1 tests failed
For details, please contact louberger |
| @@ -108,13 +108,6 @@ 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) { | |||
There was a problem hiding this comment.
Don't we need to validate this? I.e., check should stay but should just be larger (4095 instead of 240)?
There was a problem hiding this comment.
this should be done as part of regular bgp processing ( I think in bgp_io.c, length is checked)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
@qlyoung are you happy with the code? If so I'll push it in( or you can ) |
the nlri flowspec above 240 bytes size was not handled. Over 240 bytes, the length is 2 bytes length, and a calculation must be done to obtain the real length. This commit handles it appropriately. Signed-off-by: Philippe Guibert <philippe.guibert@6wind.com>
pguibert6WIND
force-pushed
the
flowspec_nlri_too_big
branch
from
6902e8c to
3255e75
Compare
There was a problem hiding this comment.
Thanks for your contribution to FRR!
Click for style suggestions
To apply these suggestions:
curl -s https://gist.githubusercontent.com/polychaeta/f025b01c33d99757967cee50336f461c/raw/15a55bba11897936f5d64d55212e15a7d39e5a63/cr_6242_1587550355.diff | git apply
diff --git a/bgpd/bgp_flowspec_private.h b/bgpd/bgp_flowspec_private.h
index cec244c16..80f277108 100644
--- a/bgpd/bgp_flowspec_private.h
+++ b/bgpd/bgp_flowspec_private.h
@@ -20,7 +20,7 @@
#define _FRR_BGP_FLOWSPEC_PRIVATE_H
#define FLOWSPEC_NLRI_SIZELIMIT 240
-#define FLOWSPEC_NLRI_SIZELIMIT_EXTENDED 4095
+#define FLOWSPEC_NLRI_SIZELIMIT_EXTENDED 4095
/* Flowspec raffic action bit*/
#define FLOWSPEC_TRAFFIC_ACTION_TERMINAL 1
If you are a new contributor to FRR, please see our contributing guidelines.
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-11984/ This is a comment from an automated CI system. Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: |
the nlri flowspec above 240 bytes size was not handled.
Over 240 bytes, the length is 2 bytes length, and a calculation must be
done to obtain the real length. This commit handles it appropriately.
Signed-off-by: Philippe Guibert philippe.guibert@6wind.com