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

pkcs8: An easier way to pass in scrypt parameters? #1205

Closed
chenxiaolong opened this issue Aug 27, 2023 · 9 comments · Fixed by #1207
Closed

pkcs8: An easier way to pass in scrypt parameters? #1205

chenxiaolong opened this issue Aug 27, 2023 · 9 comments · Fixed by #1207

Comments

@chenxiaolong
Copy link

Currently, when calling RsaPrivateKey.to_pkcs8_encrypted_pem(), it seems to use the scrypt crate's default parameters, which are: n=32768, r=8, p=1, len=32. Unfortunately, openssl can't load these files and fails with:

❯ openssl rsa -in /tmp/test.key
Enter pass phrase for /tmp/test.key:
Could not read private key from /tmp/test.key
001E5B249B7F0000:error:030000AC:digital envelope routines:scrypt_alg:memory limit exceeded:providers/implementations/kdfs/scrypt.c:482:
001E5B249B7F0000:error:030000AB:digital envelope routines:PKCS5_v2_scrypt_keyivgen_ex:illegal scrypt parameters:crypto/asn1/p5_scrypt.c:260:

Looking at openssl's manpage, it seems that they default to a lower n value:

       -scrypt
           Uses the scrypt algorithm for private key encryption using default parameters: currently N=16384,
           r=8 and p=1 and AES in CBC mode with a 256 bit key. These parameters can be modified using the
           -scrypt_N, -scrypt_r, -scrypt_p and -v2 options.

Currently, I'm able to match that and create openssl-compatible key files with:

let key: &RsaPrivateKey = <...>;

let mut rng = rand::thread_rng();

let mut salt = [0u8; 16];
rng.fill_bytes(&mut salt);

let mut iv = [0u8; 16];
rng.fill_bytes(&mut iv);

// 14 = log_2(16384), 32 bytes = 256 bits
let scrypt_params = scrypt::Params::new(14, 8, 1, 32).unwrap();
let pbes2_params = pbes2::Parameters::scrypt_aes256cbc(scrypt_params, &salt, &iv).unwrap();

let plain_text_der = key.to_pkcs8_der().unwrap();
let private_key_info = PrivateKeyInfo::try_from(plain_text_der.as_bytes()).unwrap();

let secret_doc = private_key_info.encrypt_with_params(pbes2_params, passphrase).unwrap();

let encrypted_pem = secret_doc.to_pem(EncryptedPrivateKeyInfo::PEM_LABEL, LineEnding::LF).unwrap();

pkcs8's defaults are more secure, so I don't think they should be changed, but could a more convenient API for specifying the parameters be added? (or does such a thing already exist?)

Thanks!

@tarcieri
Copy link
Member

It's bad if OpenSSL can't support our defaults. I don't suppose you have a full repro code around?

What version of OpenSSL are you using? If 3 doesn't support it we should probably consider degrading the parameters until it can.

@chenxiaolong
Copy link
Author

chenxiaolong commented Aug 27, 2023

Thanks for the reply! Yep, I have a simple reproducer here: https://github.com/chenxiaolong/RustCrypto-formats-1205

I'm using the latest available openssl version on my Linux distro: openssl-3.0.9-2.fc38.x86_64

EDIT: Just to rule out my distro's openssl version being the issue, I compiled openssl's master branch (commit 7a5f58b2cf0d7b2fa0451603a88c3976c657dae9) and the same thing happens there too.

@tarcieri
Copy link
Member

tarcieri commented Aug 27, 2023

Your repro works, and I've additionally confirmed I get the scrypt_alg:memory limit exceeded error even if I just increase log_n from 14 to 15. So it seems like that's the best we can do and still support interop with OpenSSL.

It seems like we should probably add a method to pbes2::Parameters like scrypt_with_defaults (or even just scrypt) which passes the minimum recommended parameters.

@tarcieri
Copy link
Member

FYI, I'm working on a PR for this, should have it opened shortly

@tarcieri
Copy link
Member

Oof, just realized that #1195 is really needed to add an easy API. I can open a PR but it will be rebased on top of that and can't be merged until that's merged first (and it has breaking changes)

@chenxiaolong
Copy link
Author

Got it. Thanks for looking into this so quickly!

@tarcieri
Copy link
Member

#1207 provides an easier API, but depends on #1195

@bgrieder
Copy link

just an FYI, I encountered the same incompatibility issue using PBKDF2.

For example

let pbes2_params =
            pbes2::Parameters::pbkdf2_sha256_aes128cbc(Default::default(), &salt, &iv)?;

@tarcieri
Copy link
Member

Ugh, OpenSSL is why we can't have nice things

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 a pull request may close this issue.

3 participants