Skip to content
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

Fixed crash when loading a PKCS#7 bundle with no certificates #9926

Merged
merged 1 commit into from
Nov 27, 2023

Conversation

alex
Copy link
Member

@alex alex commented Nov 27, 2023

No description provided.

@reaperhulk reaperhulk enabled auto-merge (squash) November 27, 2023 14:41
@reaperhulk reaperhulk merged commit 1e7b4d0 into pyca:main Nov 27, 2023
57 checks passed
@alex alex deleted the pkcs7-no-certs branch November 27, 2023 18:12
alex added a commit to alex/cryptography that referenced this pull request Nov 27, 2023
reaperhulk pushed a commit that referenced this pull request Nov 27, 2023
* Fixed crash when loading a PKCS#7 bundle with no certificates (#9926)

* Version bump for 41.0.6

* Temporarily allow a new clippy warning (#9835)

* Temporarily allow a new clippy warning

* Update lib.rs

* Update lib.rs

* Need to accept this to skip test

* It's a word
@@ -1111,9 +1111,12 @@ def _load_pkcs7_certificates(self, p7) -> list[x509.Certificate]:
_Reasons.UNSUPPORTED_SERIALIZATION,
)

certs: list[x509.Certificate] = []
if p7.d.sign == self._ffi.NULL:
return certs
Copy link
Contributor

@davidben davidben Nov 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this fix is quite right. I believe this should actually raise an exception here, not return the empty list, because it's analogous to the block above, where you reject content types other than signed data.

Specifically, this is NULL if you've got a content-less ContentInfo:
https://datatracker.ietf.org/doc/html/rfc2315#section-7

The spec says:

    o    content is the content. The field is optional, and
         if the field is not present, its intended value must be
         supplied by other means. Its type is defined along with the
         object identifier for contentType.

It honestly doesn't make any sense for the signed-data ContentInfos that you're parsing here. It's more for the data ContentInfo inside the signed-data ContentInfo. As the spec says later on here:

    3.   The optional omission of the content field makes
         it possible to construct "external signatures," for
         example, without modification to or replication of the
         content to which the signatures apply. In the case of
         external signatures, the content being signed would be
         omitted from the "inner" encapsulated ContentInfo value
         included in the signed-data content type.

That is, an omitted one means "this is a signed data, but I am omitting the signature, certificates, etc., with the expectation that you have it out of band somewhere".

Setting aside whether there are any use cases for that, it certainly doesn't make sense in the context of your function. By returning the empty list, you are telling the caller "yup, this contained zero certificates!". But this actually may have contained a ton of certificates, you just didn't have them available. So this is actually a PKCS#7 blob that isn't of the type you expect.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should add that, as far as compatibility goes, nothing can possibly be relying on you returning the empty list here because it used to crash. :-)

reaperhulk added a commit to reaperhulk/cryptography that referenced this pull request Dec 1, 2023
as davidben points out in pyca#9926 we are calling a specific load
certificates function and an empty value doesn't necessarily mean empty
because PKCS7 contains multitudes. erroring is more correct.
alex added a commit that referenced this pull request Dec 1, 2023
…loading (#9947)

* raise an exception instead of returning an empty list

as davidben points out in #9926 we are calling a specific load
certificates function and an empty value doesn't necessarily mean empty
because PKCS7 contains multitudes. erroring is more correct.

* changelog

* Update CHANGELOG.rst

Co-authored-by: Alex Gaynor <alex.gaynor@gmail.com>

---------

Co-authored-by: Alex Gaynor <alex.gaynor@gmail.com>
icanhasmath added a commit to ActiveState/cryptography that referenced this pull request Jan 18, 2024
* Fixed crash when loading a PKCS#7 bundle with no certificates (pyca#9926)
pyca#9926
frenzymadness pushed a commit to frenzymadness/cryptography that referenced this pull request Feb 7, 2024
…loading (pyca#9947)

* raise an exception instead of returning an empty list

as davidben points out in pyca#9926 we are calling a specific load
certificates function and an empty value doesn't necessarily mean empty
because PKCS7 contains multitudes. erroring is more correct.

* changelog

* Update CHANGELOG.rst

Co-authored-by: Alex Gaynor <alex.gaynor@gmail.com>

---------

Co-authored-by: Alex Gaynor <alex.gaynor@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants