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

Support for parsing large ASN.1 values #58

Closed
Tracked by #57
cpu opened this issue Apr 27, 2023 · 1 comment · Fixed by #73
Closed
Tracked by #57

Support for parsing large ASN.1 values #58

cpu opened this issue Apr 27, 2023 · 1 comment · Fixed by #73

Comments

@cpu
Copy link
Member

cpu commented Apr 27, 2023

Promoting my comment from #44 into an issue:

Ring's read_tag_and_get_value function from io/der.rs only supported values with lengths that could be encoded in at most two bytes. I couldn't find a definitive reason in the development history but suspect this was a cautious choice that worked for the domain of smaller certificate data Ring was operating with. Unfortunately, CRLs can be quite large (up to 50mb in my corpus) and thus require processing DER values with lengths that may span three or four bytes. To unblock my testing I made a crude patch to ring and found no further roadblocks with this branch. I don't think this patch is an appropriate change as-is, we likely want to restrict these large values to the domain of CRL processing and not allow large values in other domains.

Possible options:

  • Patch Ring's read_tag_and_get_value to allow larger reads as I did in ae80a1.
  • Implement a second, more targeted version of this function, or introduce a parameter, so that we can allow large reads but only for the CRL context, not certificates
  • Something else I'm not thinking of

Open questions:

  • Do we want to impose a size-limit on CRLs? I haven't done an analysis of other implementations to see what they do but CRLs in the 50 ... 100mb range do seem to exist in the wild. I've seen references to DoD CRLs that are particularly chonky.
This was referenced Apr 27, 2023
@djc
Copy link
Member

djc commented May 16, 2023

Maybe some generics along the lines of rustls' TlsListElements::SIZE_LEN?

I think in general we want size limits on everything just to avoid DoS shenanigans, but maybe this particular size limit should be supplied by the caller? (In which case we should clearly document some expected useful values for common scenarios.)

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 a pull request may close this issue.

2 participants