der/signed_data: introduce support for larger DER values. #73
Conversation
Codecov Report
@@ Coverage Diff @@
## main #73 +/- ##
==========================================
+ Coverage 95.59% 95.90% +0.30%
==========================================
Files 15 15
Lines 2953 3078 +125
==========================================
+ Hits 2823 2952 +129
+ Misses 130 126 -4
... and 2 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
I reduced the intermediate commits. Hopefully it's a little bit better now? I also realized I was mistakenly referring to things as "short form" lengths when they were in fact the long-form lengths and had miscalculated some boundaries based on thinking part of the leading byte represented length content when it was not the case. All of the above have been fixed throughout. Writing this comment I also realized I dropped the unit test coverage commit. I will fix that in a moment. Please hold review until that's in :-) |
Ok, re-added and ready for another review pass. Thanks! |
|
Given webpki does no-copy parsing and doesn't have a streaming interface, I think the amount of DOS attack surface we are avoiding by limiting the size of certificates is hopefully limited. In other words, to process a 24MB certificate the caller must already have the entire thing in contiguous memory. But keeping the existing constraint is a conservative choice, and one that won't be a problem until post-quantum certificates start being a thing. |
|
Working on test failures ⏳ |
|
@ctz I think this is ready for another pass when you have a chance. Since #44 landed I updated cc2a4a4 to apply the changes in this branch to CRL parsing. That's the only meaningful change outside of the tweaks suggested in your prev. review. Previously the only way to construct a CRL was through |
This commit lifts out the `read_tag_and_get_value` implementation from Ring's `io::der` package so that it can be customized for webpki's needs. In anticipation of modifying this function to allow specifying a maximum length for the DER value, we name it `read_tag_and_get_value_limited`. For now, as this `fn` is unused we allow `dead_code`.
Also updates the lifted `read_tag_and_get_value_limited` to use this convenient translation in place of a wrapper fn.
This commit introduces some constants for misc. DER values that are used when reading a tag and value. Additionally, some variable names are reworded to better clarify that what bytes are signifiers of the length encoding and what bytes constitute the encoded length data.
This commit updates the lifted `read_tag_and_get_value_limited` function to support reading long-form DER encoded lengths of three and four bytes in addition to *ring*'s existing support for short-form lengths and one and two byte long-form lengths. A two-byte long-form DER encoded length (the largest supported previously) allows values of up to 65,536 bytes (2^16) which is insufficient for some larger DER encoded objects such as signed CRL data. Four byte long-form DER lengths will allow reading values up to up to 4096MB (2^32). In a subsequent commit we will introduce a limit argument to this function to allow the caller to control how large the read value can be.
This commit adds a `size_limit: usize` parameter to `read_tag_and_get_value_limited`, allowing the caller to decide a maximum length for a value to be read. When the encoded length exceeds this limit `Error::BadDer` is returned and the reader is not used to fetch the tag's value. With the size limit in place, we can introduce a variant of `expect_tag_and_get_value` that allows specifying a maximum size limit. The existing `expect_tag_and_get_value` is limited to lengths that can be expressed in the long-form DER encoding of at most two bytes. This version can be used by callers that wish to control the maximum value size with their own limit. Next, we update `parse_signed_data` to allow the caller to specify a size limit for the outermost SEQUENCE of signed data. In the context of processing certificates, we maintain the existing implicit limit that was imposed by ring. Similarly `CertificateRevocationList::try_from` is updated to use the same default size limit as used for certificates. This allows keeping the `TryFrom` impl and provides a sensible default for users that don't know what to use. For CRLs where there is no risk of DOS attack we update the CRL `TryFrom` implementation to use the maximum supported DER size (based on the four-byte long-form encoding) as the limit. This allows parsing large CRLs that would exceed the upstream *ring* two-byte long-form limit.
|
Nope, I think I'm good. |
This branch updates the Webpki
dermodule to offer some function variants that allow reading a DER encoded tag and value where the value's length can be expressed as a long-form length of up to four bytes.To do this we have to lift the
ring::io::dermodule'sread_tag_and_get_valuefunction intowebpki, since it is hardcoded to support long-form lengths no longer than two bytes and the upstream is not responsive to patches at this time. Once the function is within thewebpkicode and can be modified we tweak it to support lengths up to five bytes, and to allow the caller to specify a maximumusizeto read.The primary use-case for larger values is for supporting using
parse_signed_dataon large CRLs. To enable this theparse_signed_datafunction is updated to accept asize_limit. The one existing caller of this function (in cert.rs, for parsing certificates) was previously using the implicit limit imposed by ring to read no more than (2^16)-1 bytes. This branch makes that implicit limit into an explicit limit so that there's no risk of DOS from an extremely large certificate. TheCertificateRevocationList::try_fromfn that implementsTryFromis updated to allow the maximum supported tagged DER value size (based on the four-byte long-form limit). This will allow loading large CRLs where there is no DOS risk.Unit test coverage is added both for the new size range and for existing error paths (e.g. high tag number form, non-canonical encodings).
Resolves #58