Reinstate indefinite length and [UNIVERSAL 0] support in crypto/asn1 #2306
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2306 +/- ##
==========================================
+ Coverage 78.73% 78.76% +0.02%
==========================================
Files 616 619 +3
Lines 107584 107861 +277
Branches 15286 15319 +33
==========================================
+ Hits 84709 84959 +250
- Misses 22220 22249 +29
+ Partials 655 653 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| const std::vector<uint8_t> zero_tag_sequence = {0x30, 0x02, 0x00, 0x00}; | ||
| const std::vector<uint8_t> zero_tag_set_any = {0x31, 0x02, 0x00, 0x00}; |
There was a problem hiding this comment.
OpenSSL uses a variant of this that we might want to test as well:
const std::vector<uint8_t> zero_tag_bad_sequence = {0x31, 0x02, 0x02, 0x00};
There was a problem hiding this comment.
Ahh nice, I hadn't thought of looking into OpenSSL's tests. I'll scour for more if there's any.
crypto/asn1/tasn_dec.c
Outdated
| if (inf) { | ||
| expected_eoc++; |
There was a problem hiding this comment.
- OpenSSL performs an extra check here. (OpenSSL also declares
expected_eocas auint32_t.):
if (expected_eoc == UINT32_MAX) {
ASN1err(ASN1_F_ASN1_FIND_END, ERR_R_NESTED_ASN1_ERROR);
return 0;
}
There was a problem hiding this comment.
Good catch. I was mainly focused on reverting the commit, but new changes around the code in OpenSSL definitely need to be taken note of as well. I'll look around to see if there's more.
| if (expected) { | ||
| EXPECT_TRUE(obj); | ||
| } else { | ||
| EXPECT_FALSE(obj); |
There was a problem hiding this comment.
should we also check that the error queue is populated here?
There was a problem hiding this comment.
Sure! The errors will be different for each specific circumstance, but we can check that the error type is of ASN1 here.
| // If indefinite length constructed find the real end. | ||
| if (inf) { | ||
| if (!asn1_find_end(&p, plen, inf)) { | ||
| goto err; |
There was a problem hiding this comment.
this is uncovered. can we add a test case that drops the 00 00 trailer?
| if (expected_eoc == 0) { | ||
| break; | ||
| } | ||
| len -= 2; |
There was a problem hiding this comment.
this is uncovered. is there a nested-indefinite test SEQUENCE we can construct that will exercise this?
| OPENSSL_PUT_ERROR(ASN1, ASN1_R_NESTED_ASN1_ERROR); | ||
| return 0; | ||
| } | ||
| if (inf) { |
There was a problem hiding this comment.
the true case here is uncovered. can we add a test case to exercise it?
| if (expected) { | ||
| EXPECT_TRUE(obj); | ||
| } else { | ||
| EXPECT_FALSE(obj); |
There was a problem hiding this comment.
Sure! The errors will be different for each specific circumstance, but we can check that the error type is of ASN1 here.
Issues:
Addresses
CryptoAlg-3037Description of changes:
This is the first step of trying to reinstate support for BER in our OpenSSL ASN1 macro parsers. Proper support for BER consists of two parts, "indefinite length BER" and "implicitly tagged constructed BER strings". This is being done to properly support BER in our PKCS7 parsers, as we've made the transition to switch from CBB -> ASN1 macros for PKCS7 for now: 621bceb. We were using
CBS_asn1_ber_to_derto translate BER -> DER ford2i_PKCS7, but the function has a caveat with implicitly-tagged constructed strings that causes encoding translations to be incomplete. We've decided to restore BER support in the macro parsers to resolve the PKCS7 compatibility issue and provide better OpenSSL compatibility for possible applications using OpenSSL's ASN.1 functions with BER encoding.This commit is a revert of the two prior commits below. All "new logic" in
crypto/asn1/tasn_dec.cwas taken from these two commits.The only smaller new changes are the following:
asn1_get_object_maybe_indefiniteto callASN1_get_object. This aligns with the original code in OpenSSL. I think we can clean up some of the changes done in cfe86a3, but I'd prefer to do so in a separate commit.Testing:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.