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

[GHSA-9v9h-cgj8-h64p] Issue summary: Processing a maliciously formatted PKCS12... #3472

Conversation

m3t3kh4n
Copy link

@m3t3kh4n m3t3kh4n commented Feb 4, 2024

Updates

  • Affected products
  • CWEs
  • References
  • Summary

Comments
Change Title
Add Reference URLs
Add Affected Product
Add CWE

@github-actions github-actions bot changed the base branch from main to m3t3kh4n/advisory-improvement-3472 February 4, 2024 22:56
@darakian
Copy link
Contributor

darakian commented Feb 5, 2024

Hey @m3t3kh4n, thanks for the PR, but I'm not sure I see the connection between pyca/cryptography@3519591 and this advisory. Any chance you can elaborate?

@m3t3kh4n
Copy link
Author

m3t3kh4n commented Feb 6, 2024

Hey @darakian , they bumped OpenSSL version in CI (pyca/cryptography@f7032bd), that's the reason why I added this change.

@m3t3kh4n
Copy link
Author

m3t3kh4n commented Feb 6, 2024

It is a little bit contradictory, I agree. But I saw that several other vulnerability resources bind this CVE with pyca/cryptography CPEs.

@darakian
Copy link
Contributor

darakian commented Feb 6, 2024

Well lets dig a little to avoid just replicating what could be false assumptions. Looking at the advisory it calls out

OpenSSL APIs that are vulnerable to this are: PKCS12_parse(), PKCS12_unpack_p7data(), PKCS12_unpack_p7encdata(), PKCS12_unpack_authsafes() and PKCS12_newpass().

and it looks like the cryptography library only uses one of those. Namely PKCS12_parse
https://github.com/search?q=repo%3Apyca%2Fcryptography+PKCS12_parse&type=code
The others all come up with blank search results
https://github.com/search?q=repo%3Apyca%2Fcryptography+PKCS12_unpack_p7data&type=code
https://github.com/search?q=repo%3Apyca%2Fcryptography+PKCS12_unpack_p7encdata&type=code
https://github.com/search?q=repo%3Apyca%2Fcryptography+PKCS12_unpack_authsafes&type=code
https://github.com/search?q=repo%3Apyca%2Fcryptography+PKCS12_newpass&type=code

PKCS12_parse looks to only have one call as well
https://github.com/pyca/cryptography/blob/aade784e71a1b91e1a5a8e239c4d9861feb4193d/src/cryptography/hazmat/backends/openssl/backend.py#L612
which is wrapped in load_pkcs12

Do you happen to have a test input that I could pass to this function to trigger the crash? There's some code in the function that looks like it might guard against this attack

p12 = self._lib.d2i_PKCS12_bio(bio.bio, self._ffi.NULL)
        if p12 == self._ffi.NULL:
            self._consume_errors()
            raise ValueError("Could not deserialize PKCS12 data")

@darakian
Copy link
Contributor

darakian commented Feb 16, 2024

Ok, I got some time to test this and using the three pkcs12 files which were added as tests to openssl here
https://github.com/openssl/openssl/pull/23362/files
All three did in fact crash my python interpreter when I attempted to load them.
Ex.

(pkcs12-testing-venv) jon~/pkcs12-dol❯❯❯ python3
Python 3.11.7 (main, Dec  4 2023, 18:10:11) [Clang 15.0.0 (clang-1500.1.0.2.5)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import cryptography
>>> from cryptography.hazmat.backends.openssl import backend
>>> bad3 = open("bad3.p12", "rb").read()
>>> backend.load_pkcs12(bad3, None)
fish: Job 1, 'python3' terminated by signal SIGSEGV (Address boundary error)
(pkcs12-testing-venv) jon~/pkcs12-dol❯❯❯ pip show cryptography
Name: cryptography
Version: 42.0.0
Summary: cryptography is a package which provides cryptographic recipes and primitives to Python developers.
Home-page:
Author:
Author-email: The Python Cryptographic Authority and individual contributors <cryptography-dev@python.org>
License: Apache-2.0 OR BSD-3-Clause
Location: /some/path/pkcs12-testing-venv/lib/python3.11/site-packages
Requires: cffi
Required-by:

So, it would appear that you are correct. I'll go ahead and get this merged.

Edit:
For the sake of posterity here's bad3.p12 from the example above

(pkcs12-testing-venv) jon~/pkcs12-dol❯❯❯ hexdump -C bad3.p12
00000000  30 66 02 01 03 30 1e 06  09 2a 86 48 86 f7 0d 01  |0f...0...*.H....|
00000010  07 01 a0 11 04 0f 30 0d  30 0b 06 09 2a 86 48 86  |......0.0...*.H.|
00000020  f7 0d 01 07 01 30 41 30  31 30 0d 06 09 60 86 48  |.....0A010...`.H|
00000030  01 65 03 04 02 01 05 00  04 20 0b 59 64 a3 16 55  |.e....... .Yd..U|
00000040  d2 c4 c7 a8 c3 5e c5 4f  9e d9 a5 58 fc 22 d0 df  |.....^.O...X."..|
00000050  7f 9e ed 95 cb 35 f0 1e  0f c3 04 08 66 5d 7e 96  |.....5......f]~.|
00000060  a3 fc f3 76 02 02 08 00                           |...v....|
00000068

@advisory-database advisory-database bot merged commit d5baf7e into m3t3kh4n/advisory-improvement-3472 Feb 16, 2024
2 checks passed
@advisory-database advisory-database bot deleted the m3t3kh4n-GHSA-9v9h-cgj8-h64p branch February 16, 2024 20:48
@advisory-database
Copy link
Contributor

Hi @m3t3kh4n! Thank you so much for contributing to the GitHub Advisory Database. This database is free, open, and accessible to all, and it's people like you who make it great. Thanks for choosing to help others. We hope you send in more contributions in the future!

@m3t3kh4n
Copy link
Author

Hello @darakian, sorry for the late response. This email thread lost in my inbox. I am happy that you checked and confirmed the vulnerability. Thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants