-
Notifications
You must be signed in to change notification settings - Fork 152
Reinstate support for constructed strings in crypto/asn1 #2310
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
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like collect_data is never called with buf == NULL, should this return 0 instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.