openssl / openssl

TLS/SSL and crypto library
https://www.openssl.org
Apache License 2.0
25.39k stars 10.05k forks source link

OpenSSL 3.0.7: Unable to set ivlen >12 via params field of EVP_EncryptInit_ex2 for AES-GCM #19822

Closed dskern-github closed 10 months ago

dskern-github commented 1 year ago

I work on an application that has used a wide variety of cryptographic libraries on an absurd number of different platforms over the years and uses automation including cryptographic test vectors to ensure parity across different libraries and platforms.

Our AES-GCM support has successfully used the legacy three-call pattern with vectors from IEEE P1619.1 and http://csrc.nist.gov/groups/STM/cavp/ with IV lengths of 1, 12, 16, 17, and 128 bytes.

New, fetch (fips provider), and error handling removed for clarity:

EVP_EncryptInit_ex (ctx, cipher, NULL, NULL, NULL);
EVP_CIPHER_CTX_set_params (ctx, params); /* Set IV length */
EVP_EncryptInit_ex (ctx, NULL, NULL, key, iv);

I recently noticed that EVP_EncryptInit_ex had been deprecated while making an unrelated change and started working on upgrading from ex to ex2. The documentation for EVP_EncryptInit_ex2 states that "The parameters params will be set on the context after initialisation.", implying that one could collapse the three calls above into a single call:

EVP_EncryptInit_ex2 (ctx, cipher, key, iv, params);

This approach works for the test vectors with 1 and 12 byte IVs but fails for the test vectors with 16, 17, and 128 byte IVs, apparently because the IV length is extracted from the context before the parameters are actually set on the context:

evp_enc.c line 218:

        return ctx->cipher->einit(ctx->algctx,
                                  key,
                                  key == NULL ? 0
                                              : EVP_CIPHER_CTX_get_key_length(ctx),
                                  iv,
                                  iv == NULL ? 0
                                             : EVP_CIPHER_CTX_get_iv_length(ctx),
                                  params);

Either the documentation should be corrected to state that the key length and IV length are extracted from the context before the params are applied, or the params should be applied after the context is initialized and before the call to EVP_CIPHER_CTX_get_iv_length().

On a related note, the "AEAD Interface" section of https://www.openssl.org/docs/man3.0/man3/EVP_CIPHER_CTX_new.html still shows the deprecated EVP_CIPHER_CTX_ctrl calls, forcing callers who wish to follow the new patterns to bounce between the "AEAD Interface", "Controls", and "Parameters" sections of that page, plus https://www.openssl.org/docs/man3.0/man3/OSSL_PARAM.html and https://www.openssl.org/docs/man3.0/man3/OSSL_PARAM_int.html. A simple example of the new calling patterns for AES in AEAD mode in https://www.openssl.org/docs/man3.0/man3/EVP_CIPHER_CTX_new.html would make the upgrade process much easier and less error prone.

Thanks!

t8m commented 1 year ago

You could collapse the first two calls at least?

If so, I would make it just a documentation issue.

dskern-github commented 1 year ago

Yes, I just confirmed that

EVP_EncryptInit_ex2 (ctx, cipher, NULL, NULL, params);
EVP_EncryptInit_ex2 (ctx, NULL, key, iv, NULL);

works as expected. It's less ideal than the single call implied by the current doc, but better than what we had before. I'm fine with changing from a bug report to a documentation issue, but don't seem to have the ability to do so myself.

Thanks!

t8m commented 1 year ago

Please also note that EVP_EncryptInit_ex is NOT deprecated. The ex2 function is just a convenient (but perhaps not so convenient given the limitations from above) shortcut.

dskern-github commented 1 year ago

Ah - "legacy", not "deprecated". Thanks for the clarification.

EVP_EncryptInit_ex()
This legacy function is similar to EVP_EncryptInit_ex2() when impl is NULL. The implementation of the type from the impl engine will be used if it exists.
slontis commented 1 year ago

The documentation looks correct to me.. By collapsing the code you changed the order of the calls. The documentation says that it runs the params last. With the 3 calls case it runs the params in the middle. Setting the ivlen after the iv has already been set will not work. Internally it does not cache everything, so the order does matter.