Reinstate support for constructed strings in crypto/asn1 #2310
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2310 +/- ##
=======================================
Coverage 78.77% 78.77%
=======================================
Files 620 620
Lines 107874 107936 +62
Branches 15323 15337 +14
=======================================
+ Hits 84977 85030 +53
- Misses 22239 22249 +10
+ Partials 658 657 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
369254a to
994fb2a
Compare
994fb2a to
2e17942
Compare
crypto/asn1/tasn_dec.c
Outdated
Hide resolved
2e17942 to
35967f8
Compare
989dcbb
35967f8 to
989dcbb
Compare
|
|
||
| static int collect_data(BUF_MEM *buf, const unsigned char **p, long plen) { | ||
| int len; | ||
| if (buf) { |
There was a problem hiding this comment.
It seems like collect_data is never called with buf == NULL, should this return 0 instead?
There was a problem hiding this comment.
Great question, I did some digging around. But judging from the comment in asn1_collect which mentions "If no buffer and not indefinite length constructed just pass over the encoded data", it does seem like there's the possibility of buf being NULL and there being operations that need to be done under that circumstance.
I'll leave it as is for the time being, OpenSSL's ASN1 implementation is also like this today.
Issues:
Resolves
CryptoAlg-3037Description of changes:
This is the last 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". The first step of supporting indefinite length BER can be found in eafd940.
This commit is a revert of the prior commit below. All "new logic" in
crypto/asn1/tasn_dec.cwas taken from this.The only smaller new changes are the following:
free_contparameter to pass intoasn1_ex_c2i. This indicates whether a buffer should be reused or newly allocated. This made things a bit funky and the actual performance upgrade for reusing a buffer seems negligible. Removing the use offree_contaltogether helps isolate the ASN1 code changes to just one static function incrypto/asn1/tasn_dec.cand cleans up logic.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.