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.

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

Increase maximum PDU length that is decoded #121

Merged
merged 2 commits into from
Jun 26, 2024

Conversation

ties
Copy link
Collaborator

@ties ties commented Apr 5, 2024

Limit the maximum PDU length that is decoded to a size that is both safe for the software as well as unrealistic in the DFZ. This path is only hit when using rtrdump, rtrmon, or when using starter as a library.

It is very feasible to create a ASPA PDU > 2k.

@cjeker this is more relevant in real BGP speakers.

Limit the maximum PDU length that is decoded to a size that is both
safe for the software as well as unrealistic in the DFZ.
@ties ties requested a review from job April 5, 2024 09:30
@cjeker
Copy link
Contributor

cjeker commented Apr 6, 2024

I think your new limit is far to big. In no sensible world one AS would have every other AS of the DFZ as a provider.

rpki-client will limit the ASPA SPAS to 10'000 elements so there is no need for a 1MB PDU size.
I will bump OpenBGPD's max PDU size to 64k which should be enough for 10'000 SPAS in one PDU.

I dislike that the ASPA profile did not limit the number of SPAS. This idea that computers run with infinite resources is very dumb especially for a security critical application. We should prevent random operators to generate ASPA PDUs that will inflict high resource usage on every boarder router.

@ties
Copy link
Collaborator Author

ties commented Apr 6, 2024 via email

@benjojo
Copy link
Collaborator

benjojo commented Apr 6, 2024 via email

@ties
Copy link
Collaborator Author

ties commented Apr 6, 2024

Outside of the general conversation of this PDU size, I suspect there is value in having a massive ASPA entry shoved into the system early on so that systems are forced to deal with this

I agree. There is value in having non-abusive edge cases in the system early on to ensure safe behaviour.

@cjeker
Copy link
Contributor

cjeker commented Apr 8, 2024

I agree that the limit is big, but given the protocol, I do not think there is any sane value that would prevent the denial-of-service-by-disconnect gap.

I think the RTR caches should prevent the generation of huge PDUs. Up until now all RTR PDU were minimal in size but
the unfinished darft-8210bis code introduces a PDU with a much much bigger possible payload. Right now I stopped caring about darft-8210bis. That draft in its current form is not making it through WGLC.

So maybe it is time to add some sensible limits to the RFC so implementations don't diverge too much.

rpki-client will limit the ASPA SPAS to 10'000 elements so there is no need for a 1MB PDU size.

64kb definitely is an improved value. But unfortunately that limit is per ASPA, which allows multiple objects to be combined for a larger number of SPAS for an AS. This would require active "experiments"/attacks in the public RPKI, but that is the status quo.
1MB is manageable for systems and a large enough number to definitely cover what is realistic in the DFZ. I think this may need RPs to limit the total number of SPAS for an AS (over all ASPAs), or alternatively, a protocol change.

rpki-client added a per VAP limit of 10'000 elements as well. We see no point in pushing more SPAS down the pipeline and to be honest 10'000 is already more than a magnitude more than what is sensible. It seems the biggest AS need maybe a few hundred SPAS and that's for probably a handful of very special ASes.

@ties
Copy link
Collaborator Author

ties commented May 4, 2024

I've adjusted it to the upper bound discussed in a mail thread. This is big, but I don't want rtrmon/rtrdump to break when they are monitoring/debugging aids.

@ties
Copy link
Collaborator Author

ties commented May 4, 2024

@benjojo what do you think?

I've skipped a metric (series, { _sum, _count, _max}) on the PDU size of now, I see limited value there.

@ties ties merged commit bf5793c into bgp:master Jun 26, 2024
3 checks passed
@ties ties deleted the increase-max-message-size branch June 26, 2024 14:52
Sign up for free to 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