der: request to increase MAX_LENGTH for DER Length

[ramenguy99]

I am running into an Overflow error when parsing CMS messages with content larger than 256MiB in DER format. OpenSSL produces and parses messages of this size without problems, and for my application it's crucial to handle files larger than this limit.

The value of MAX_LENGTHthat results in this overflow is specified here:

formats/der/src/length.rs

Lines 10 to 11 in 057aa1f

	/// Maximum length as a `u32` (256 MiB).
	const MAX_U32: u32 = 0xfff_ffff;

Some years ago this commit bumped the max size from 1MiB to 256MiB. Mentioning that it would be possible to bump this value again if needed in the future. Since the value is parsed from up to 4 octets I don't see an obvious reason to limit this to less than 4GiB.

Would it be possible to increase this value to at least 1 GiB?

[tarcieri]

Since the value is parsed from up to 4 octets I don't see an obvious reason to limit this to less than 4GiB.

It's a sanity limit to prevent DoS attacks using extremely large documents.

Perhaps it could apply only to DER documents instead of BER documents. Or we could add an "unchecked" API which doesn't have a limit.

[turbocool3r]

This is something that I find useful as well for my Image4 format parser (https://gitlab.com/turbocooler/image4-rs). These files can be larger than 256 MiB.

[turbocool3r]

After thinking about this more I wonder if this limit is something that would help to protect crate users against DoS attacks. In my experience at least DoS attacks happen due to a large number of objects rather than the size of the input buffer, and you can fit a lot of DER-ecnoded objects into a 256 MiB buffer. I can see how these 256 MiB will turn into tens of gigabytes of RAM with a creatively constructed X.509 certificate. I think the size limit should be defined by the application and not the library or at least not by the der library itself.

Removing this limit (an hopefully making Length a wrapper for usize) will enable more uses for the library. Large CMS messages and embedded device firmwares might be good enough use cases to justify the limit removal. Also this might be a good time since 0.8.0 is not released yet.

[tarcieri]

The main concern driving the limitation is trying to cap the amount of memory that can be allocated by parsing an input message. The number of objects is less of a concern.

der is primarily intended for cryptographic applications which is why we try to have secure and restrictive defaults, though I agree there should be some way to bypass the limit for use cases where large objects are expected.

[turbocool3r]

I understand these concerns so I took some time to make a certificate that is a bit smaller than 256 MiB to demonstrate the effects of what I mean. The small test program consumes 2,5-3 GiB of memory while parsing this certificate. It could be optimized to account for vector allocation specifics and have more OIDs inside to increase memory usage.

Code: https://gist.github.com/turbocool3r/0b93b46ab57d66e71ff25c1deb4129a2.

cert.der.gz

[turbocool3r]

there should be some way to bypass the limit for use cases where large objects are expected.

Is there still interest in this?

[ramenguy99]

Hi, yes, I am still running a patched version to work around this limitation. An option to bypass the limit would be great.

[tarcieri]

I guess we can just remove the limit

[turbocool3r]

Thanks for considering that, @tarcieri! Should the Length type be left or can it be replaced with usize?

[tarcieri]

The Length type has myriad other uses including being this library's strategy for always performing checked arithmetic on length calculations, not to mention encoding/decoding lengths as DER. It can't be easily "removed".

If you want to attempt to remove at least some uses, I would need to see a PR before I would consider if it's acceptable.

[dishmaker]

Fixed in #1726

formats/der/src/length.rs

Lines 28 to 29 in 500cd65

	/// Maximum length (`u32::MAX`).
	pub const MAX: Self = Self(u32::MAX);