cloudflare / gortr

The RPKI-to-Router server used at Cloudflare
https://rpki.cloudflare.com
BSD 3-Clause "New" or "Revised" License
309 stars 39 forks source link

Fix: unbounded alloc and slice out of bounds crashes #78

Closed ShimmerGlass closed 4 years ago

ShimmerGlass commented 4 years ago

In rtrlib.Decode():

I'm not super familiar with the protocol, I feel 2k max message size is enough but please correct me if I'm wrong. I also took the liberty to change error returns to be more idiomatic.

lspgn commented 4 years ago

Thanks! I checked if the RFC specify a maximum length. It allocates 4 bytes for the length but there is no reason a PDU should be 4GB.

   Length:  A 32-bit unsigned integer that has as its value the count of
      the bytes in the entire PDU, including the eight bytes of header
      that end with the length field.

But later in the text, it mentions excessive length implying the maximum size would be 32 (IPv6 PDU).

   If the error is associated with a PDU of excessive length, i.e., too
   long to be any legal PDU other than another Error Report, or a
   possibly corrupt length, the Erroneous PDU field MAY be truncated.

So maximum size could be 32 bytes. I'd rather not hardcode it (for development purposes) but putting it as a default.

I'll start reviewing/testing soon then will merge.

ShimmerGlass commented 4 years ago

Hi @lspgn, thanks for the quick reply,

I have a feeling that 32B is not enough for the PDU_ID_ERROR_REPORT txt error message but I might be wrong here. Looking at it some more, a 800B (hardcoded: https://github.com/cloudflare/gortr/blob/master/lib/server.go#L783) buffer is allocated for reading packets from the network conn, so the PDU cannot exceed that length anyway. (this however does not limit allocation further down so we still need the check from this PR).

ShimmerGlass commented 4 years ago

Thank you !