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

AES256 support for PKCS#12 #7043

Closed
jobec opened this issue Apr 5, 2022 · 14 comments · Fixed by #7560
Closed

AES256 support for PKCS#12 #7043

jobec opened this issue Apr 5, 2022 · 14 comments · Fixed by #7560

Comments

@jobec
Copy link

jobec commented Apr 5, 2022

I came across this piece of code in the openssl backend:

# PKCS12 encryption is hopeless trash and can never be fixed.
# This is the least terrible option.
nid_cert = self._lib.NID_pbe_WithSHA1And3_Key_TripleDES_CBC
nid_key = self._lib.NID_pbe_WithSHA1And3_Key_TripleDES_CBC
# At least we can set this higher than OpenSSL's default
pkcs12_iter = 20000
# mac_iter chosen for compatibility reasons, see:
# https://www.openssl.org/docs/man1.1.1/man3/PKCS12_create.html
# Did we mention how lousy PKCS12 encryption is?

It's part of what gets called when creating a PKXS#12 file and It uses 3DES for the encryption.

Is there any way to switch this to something like AES256?

When exporting to a PFX in windows 10, you can do this as mentioned here

Also, you can make such PFX file though openSSL as mentioned here (and here for v1.1.1):

C:\>openssl pkcs12 -export -in cert.pem-inkey private.key -out some.pfx -certpbe AES-256-CBC -keypbe AES-256-CBC
Enter pass phrase for key_private_pem.key:
Enter Export Password:
Verifying - Enter Export Password:

C:\>openssl pkcs12 -noout -info -in some.pfx
Enter Import Password:
MAC: sha1, Iteration 2048
MAC length: 20, salt length: 8
PKCS7 Encrypted data: PBES2, PBKDF2, AES-256-CBC, Iteration 2048, PRF hmacWithSHA256
Certificate bag
PKCS7 Data
Shrouded Keybag: PBES2, PBKDF2, AES-256-CBC, Iteration 2048, PRF hmacWithSHA256
@jobec
Copy link
Author

jobec commented Apr 5, 2022

If I locally patch Cryptography and change these lines

nid_cert = self._lib.NID_pbe_WithSHA1And3_Key_TripleDES_CBC
nid_key = self._lib.NID_pbe_WithSHA1And3_Key_TripleDES_CBC
# At least we can set this higher than OpenSSL's default
pkcs12_iter = 20000

into

 nid_cert = 427 
 nid_key = 427 
 # At least we can set this higher than OpenSSL's default 
 pkcs12_iter = 2048

(427 being the value of NID_aes_256_cbc)
The created PKCS#12 PFX file is AES-256-CBC encrypted:

c:\>openssl pkcs12 -noout -info -in some.pfx
Enter Import Password:
MAC: sha1, Iteration 1
MAC length: 20, salt length: 8
PKCS7 Encrypted data: PBES2, PBKDF2, AES-256-CBC, Iteration 2048, PRF hmacWithSHA256
Certificate bag
Certificate bag
Certificate bag
PKCS7 Data
Shrouded Keybag: PBES2, PBKDF2, AES-256-CBC, Iteration 2048, PRF hmacWithSHA256

And Windows seems to load and import it into it's certificate store just fine.

@alex
Copy link
Member

alex commented Apr 5, 2022

@reaperhulk is there any reason not to move to AES for PKCS#12? I assume one concern is compatibility, but with what?

@reaperhulk
Copy link
Member

We originally chose this because PKCS12 is trash and trying to use AES resulted in lots of things being unable to read the files we created. However, OpenSSL 3.0 defaults to NID_aes_256_cbc so it seems likely we could make this change as well. Changelog entry from OpenSSL:

pkcs12 now uses defaults of PBKDF2, AES and SHA-256, with a MAC iteration count of PKCS12_DEFAULT_ITER.

@alex
Copy link
Member

alex commented Apr 6, 2022 via email

@alex alex added this to the Thirty Seventh Release milestone Apr 6, 2022
@jobec
Copy link
Author

jobec commented Apr 6, 2022

We originally chose this because PKCS12 is trash and trying to use AES resulted in lots of things being unable to read the files we created. However, OpenSSL 3.0 defaults to NID_aes_256_cbc so it seems likely we could make this change as well. Changelog entry from OpenSSL:

pkcs12 now uses defaults of PBKDF2, AES and SHA-256, with a MAC iteration count of PKCS12_DEFAULT_ITER.

Regarding compatibility: It might be an idea to let people select 3DES still. For example NoEncryption , LegacyEncryption and BestAvailableEncryption

I can also send in a merge request with the things above changed. But I have no experience with the rest of Cryptography's code base and certainly not the Rust/C part of it.

So, happy to send in a merge request but not sure if I can finish it 'till the end.

@jobec
Copy link
Author

jobec commented Apr 6, 2022

I've also done some tests regarding compatibility with AES-256-CBC encrypted PKCS12 files:

  1. ❌ Windows server 2016
  2. ✔ Windows Server 2019
  3. ✔ Windows 10
  4. ❌ keytool of Java 1.8.0 (ref: https://bugs.openjdk.java.net/browse/JDK-8153005)
  5. ✔ keytool of Java 11.0.14 (ref: https://bugs.openjdk.java.net/browse/JDK-8153005)
  6. ✔ F5 LTM

@reaperhulk
Copy link
Member

Thanks for doing those tests, that's really helpful.

I think the path forward here is implementing a new, more flexible encryption option that lets you pick TDES or AES and then maybe have a one release window where we warn people that the encryption default will change in the next release for BestAvailable. LegacyEncryption is tempting due to simplicity, but not flexible enough IMO.

What that API should look like is unclear to me though. There was a past experiment with expanding what PKCS8 encryption could do (#5656) but it never defined a public API.

@AHalvar
Copy link

AHalvar commented Apr 9, 2022

While you are at it, please consider also adding support for selecting the MAC algorithm, which is done like so on the openssl command line:

openssl pkcs12 -export -macalg SHA256 -certpbe AES-256-CBC -keypbe AES-256-CBC -in cert.cer -inkey private.key -certfile ca_chain -out out.p12

I expected I would be able to implement a subclass to KeySerializationEncryption to which I could provide the algorithms from the openssl command line and hand that subclass instance to pkcs12.serialize_key_and_certificate but, alas, no.

Using the above command line gives the following info:

% openssl pkcs12 -in out.p12 -passin pass:password -noout -info
 MAC: sha256, Iteration 2048
 MAC length: 32, salt length: 8
 PKCS7 Encrypted data: PBES2, PBKDF2, AES-256-CBC, Iteration 2048, PRF hmacWithSHA256
 ...

@jobec
Copy link
Author

jobec commented May 9, 2022

It seems like @AHalvar 's remark about being able to set the MAC algo is an important one.

With v37.0's move to OpenSSL 3, it also changed to it's defaults. So now the PKCS12 functions create files with 3DES encryption but with a SHA256 MAC.

Guess what... Windows 2016 server cannot read such files 🤦 .it must have a MAC of SHA1.

So, it has become impossible as of version 37 to create windows 2016 and older compatible PFX files.

I tried adding this code to the serialize_key_and_certificates_to_pkcs12 function, but seems PKCS12_set_mac isn't exposed.

from cryptography.hazmat.primitives.hashes import SHA1
...
md = self._evp_md_non_null_from_algorithm(SHA1)
self._lib.PKCS12_set_mac(
    p12, password_buf, -1, self._ffi.NULL, 0, mac_iter, md
)

@reaperhulk
Copy link
Member

Server 2016 should be ashamed of itself, but here we are.

There's some work going on in #7178 but we need to land some new features in boringssl for that to merge. It does not currently do anything with the MAC, but it would appear it needs to.

@jobec
Copy link
Author

jobec commented May 12, 2022

Would it be possible to expose PKCS12_set_mac in a next release? Then I can try myself to override a few things.

I tried compiling on windows but I can't manage to get OpenSSL bindings to work. Keeps complaining about DLLs that aren't found and the _openssl.pyd file is also just a couple of hundred KB.

@reaperhulk
Copy link
Member

Adding that binding is fine, although it's not available in all versions we support so the standard set of conditionals will have to be used.

@jobec
Copy link
Author

jobec commented Jun 27, 2022

@reaperhulk any chance for the extra binding to be available in a release any time soon?

I see it didn't got included in 37.0.3

I now need to tell users to get OpenSSL installed and then to run a bunch of cryptic openSSL commands to create usable PKCS#12 files.

@reaperhulk
Copy link
Member

It will be in 38.0, but I also intend to diagnose and fix this generally before we release 38.0. We could potentially backport for 37.0.4 as well, although we’re waiting on openssl for 3.0.5 there.

@alex alex added this to the Thirty Eighth Release milestone Aug 25, 2022
@alex alex closed this as completed in #7560 Sep 5, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.