Fix several bugs relating to OID encoding and decoding #4063
Conversation
randombit
force-pushed
the
jack/fix-4023
branch
4 times, most recently
from
2444010 to
a2e57dc
Compare
There was a problem hiding this comment.
Looks good overall 👍.
I'm not sure if it helps to clarify, that Botan only really supports OIDs in the context of X.503v3 certificates. In theory OIDs can have infinitely large components, while RFC5280 (Appendix B) restricts them to only 28 bits.
I think negative tests would be beneficial as well: Using invalid encodings and seeing what happens (hopefully an exception). I found some test vectors on this page: https://misc.daniel-marschall.de/asn.1/oid_facts.html#chap3
| // This last is a limitation of using 32 bit integers when decoding | ||
| // not a limitation of ASN.1 object identifiers in general | ||
| BOTAN_ARG_CHECK(oid[1] <= 0xFFFFFFAF, "OID second arc too large"); |
There was a problem hiding this comment.
Where's that 0xFFFFFFAF constant coming from? The comment doesn't explain why that's the limit. I imagine it has something to do with the variable length encoding?
You can go even further and restrict it to just 28 bits (0x0FFF'FFFF), if you only want to support X.509v3:
This specification mandates support for OIDs that have arc elements
with values that are less than 2^28, that is, they MUST be between 0
and 268,435,455, inclusive.
There was a problem hiding this comment.
It's (2**32-1) - 80 which ensures that we can hold the combined first + second in a single uint32_t
Indeed. In the short term we can't fix this because we have some APIs (which become deprecated in this PR) which return the OID components as 32 bit integers. So it's not possible for us to support anything larger that this in the meantime.
Good point thanks, I'll add this. |
The handling for OID encoding did not correctly handle OIDs that begin with 2.x where x >= 40. Fixes #4023
Even BER requires that the first byte of a multibyte encoding be greater than zero, so don't accept this.
| @@ -159,28 +170,33 @@ void OID::encode_into(DER_Encoder& der) const { | |||
| throw Invalid_Argument("OID::encode_into: OID is invalid"); | |||
There was a problem hiding this comment.
Side note: If we were able to get rid of the default constructor, we could simply assert that an OID object must be valid. All other constructors call oid_valid_check() and therefore throw during construction of an invalid OID.
There was a problem hiding this comment.
Unfortunately with how the ASN.1 library works right now any type you decode has to be default constructable. :/
There was a problem hiding this comment.
(I'm working on a new ASN.1 library but this is a very long term project, Botan 4 at earliest)
Co-authored-by: René Meusel <[email protected]>
|
Thanks for fixing this! 👏 |
The handling for OID encoding did not correctly handle OIDs that begin with 2.x where x >= 40.
Fixes #4023