Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
PKCS7 Parser - RFC 2315 #3431
PKCS7 Parser - RFC 2315 #3431
Changes from 30 commits
c9deb18
673a226
ca07f06
aa91d4e
106a0af
136c6aa
c448c94
390e61a
600bd30
6671841
6427b34
45525d3
5d881c3
8a10f66
3538479
62b2d7e
9f4fb3e
8a94de4
7089ce8
34d5e93
8ce1b1a
9512bde
5f9456f
7dbe852
73621ef
bb82ab7
5f39767
3951a4f
fc234b7
2364aae
89e82e1
f58172f
50e5616
ebd0caf
71565cf
a17d038
ae79fb2
12269e2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Given that there are a few assumptions in pkcs7.c (e.g., we don't support multiple digest algorithms), I think it would be sensible to have a comment describing the limitations of our PKCS7 support here.
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.
I agree this is a good idea, this is certainly only a partial implementation.
Are you saying we only support sha256 as digest alg or that we only allow the listing of one md alg?
DigestAlgorithmIdentifiers
field, I will add that to the list of assumptions here.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.
I meant the latter, thanks. I had assumed other algs would work but agreed, testing this assumption is probably desirable.
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.
Shouldn't pkcs7 and cert parameters also be 'const'? You certainly wouldn't expect the input certificate to get modified.
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.
Same 'const' comment as above.
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.
Is zeroizing needed for PKCS7 data? What secret data do we risk exposing?
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.
Up to you guys, nothing is exposed that is more confidential than a public key and and data that has been signed with the private key. I will remove these zeroize calls for now and if anyone can come up with an argument against it than we can debate adding them back.