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

EVP_PKEY_CTX_add1_hkdf_info() no longer works with providers that do not expose gettable parameters in ossl_kdf_hkdf_keyexch_functions array #24611

Closed
jmb202 opened this issue Jun 11, 2024 · 3 comments
Assignees
Labels
branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 branch: 3.2 Merge to openssl-3.2 branch: 3.3 Merge to openssl-3.3 severity: regression The issue/pr is a regression from previous released version triaged: bug The issue/pr is/fixes a bug

Comments

@jmb202
Copy link

jmb202 commented Jun 11, 2024

Upgrading from OpenSSL 3.0.13 to 3.0.14 has uncovered a breakage introduced by 4953ab1 when using a provider derived from OpenSSL 3.0.13 or earlier.

The newly introduced evp_pkey_ctx_add1_octet_string function (called by EVP_PKEY_CTX_add1_hkdf_info) in evp/pmeth_lib.c contains the following logic:

   /* Get the original value length */
    os_params[0] = OSSL_PARAM_construct_octet_string(param, NULL, 0);
    os_params[1] = OSSL_PARAM_construct_end();

    if (!EVP_PKEY_CTX_get_params(ctx, os_params))
        return 0;

    /* Older provider that doesn't support getting this parameter */
    if (os_params[0].return_size == OSSL_PARAM_UNMODIFIED)
        return evp_pkey_ctx_set1_octet_string(ctx, fallback, param, op, ctrl, data, datalen);

However, EVP_PKEY_CTX_get_params() will return 0 if ctx->op.kex.exchange->get_ctx_params is NULL and thus the subsequent check for older providers above does not get reached in the case where the provider does not expose a get_ctx_params function at all.

Any provider derived from older versions of OpenSSL will not expose get_ctx_params/gettable_params in the ossl_kdf_*_keyexch_functions array for key derivation algorithms. The commit referenced above caused these functions to be exposed: 4953ab1#diff-85fe36a1da6d2470d1a9ceb431d681dc458e600b994bb8df362e4564d943c698R246, so 3.0.14 is self-consistent.

We have worked around this locally by adjusting the code in 3.0.14 to check if any parameters are gettable when EVP_PKEY_CTX_get_params returns 0 in the logic above -- if none are, we assume that we're dealing with an old provider that does not expose get_params/gettable_params at all and fall back to evp_pkey_ctx_set1_octet_string in the same way as above.

@jmb202 jmb202 added the issue: bug report The issue was opened to report a bug label Jun 11, 2024
@nhorman
Copy link
Contributor

nhorman commented Jun 11, 2024

Thank you for the detailed report. this strikes me as a regression, @tmshort since you authored the referenced change, would you agree with that assessment.

@mattcaswell @levitte @t8m your input would be appreciated here as well.

for now I am assuming that it is, and marking it as such

@nhorman nhorman added the severity: regression The issue/pr is a regression from previous released version label Jun 11, 2024
@beldmit
Copy link
Member

beldmit commented Jun 12, 2024

@jmb202 may I ask you to publish your workaround? We also are hit by this issue

@jmb202
Copy link
Author

jmb202 commented Jun 12, 2024

@beldmit sure -- it's trivial:

--- a/crypto/evp/pmeth_lib.c
+++ b/crypto/evp/pmeth_lib.c
@@ -1061,8 +1061,13 @@ static int evp_pkey_ctx_add1_octet_string(EVP_PKEY_CTX *ctx, int fallback,
     os_params[0] = OSSL_PARAM_construct_octet_string(param, NULL, 0);
     os_params[1] = OSSL_PARAM_construct_end();
 
-    if (!EVP_PKEY_CTX_get_params(ctx, os_params))
+    if (!EVP_PKEY_CTX_get_params(ctx, os_params)) {
+        if (EVP_PKEY_CTX_gettable_params(ctx) == NULL) {
+            /* Older provider that doesn't support gettable parameters */
+            return evp_pkey_ctx_set1_octet_string(ctx, fallback, param, op, ctrl, data, datalen);
+        }
         return 0;
+    }
 
     /* Older provider that doesn't support getting this parameter */
     if (os_params[0].return_size == OSSL_PARAM_UNMODIFIED)

@t8m t8m added triaged: bug The issue/pr is/fixes a bug branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 branch: 3.2 Merge to openssl-3.2 branch: 3.3 Merge to openssl-3.3 branch: master Merge to master branch and removed issue: bug report The issue was opened to report a bug labels Jun 12, 2024
@t8m t8m self-assigned this Jun 17, 2024
t8m added a commit to t8m/openssl that referenced this issue Jun 17, 2024
If there is no get_ctx_params() implemented in the key exchange
provider implementation the fallback will not work. Instead
check the gettable_ctx_params() to see if the fallback should be
performed.

Fixes openssl#24611
openssl-machine pushed a commit that referenced this issue Jun 21, 2024
If there is no get_ctx_params() implemented in the key exchange
provider implementation the fallback will not work. Instead
check the gettable_ctx_params() to see if the fallback should be
performed.

Fixes #24611

Reviewed-by: Paul Dale <ppzgs1@gmail.com>
Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com>
(Merged from #24661)

(cherry picked from commit 663dbc9)
openssl-machine pushed a commit that referenced this issue Jun 21, 2024
If there is no get_ctx_params() implemented in the key exchange
provider implementation the fallback will not work. Instead
check the gettable_ctx_params() to see if the fallback should be
performed.

Fixes #24611

Reviewed-by: Paul Dale <ppzgs1@gmail.com>
Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com>
(Merged from #24661)

(cherry picked from commit 663dbc9)
openssl-machine pushed a commit that referenced this issue Jun 21, 2024
If there is no get_ctx_params() implemented in the key exchange
provider implementation the fallback will not work. Instead
check the gettable_ctx_params() to see if the fallback should be
performed.

Fixes #24611

Reviewed-by: Paul Dale <ppzgs1@gmail.com>
Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com>
(Merged from #24661)

(cherry picked from commit 663dbc9)
openssl-machine pushed a commit that referenced this issue Jun 21, 2024
If there is no get_ctx_params() implemented in the key exchange
provider implementation the fallback will not work. Instead
check the gettable_ctx_params() to see if the fallback should be
performed.

Fixes #24611

Reviewed-by: Paul Dale <ppzgs1@gmail.com>
Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com>
(Merged from #24661)

(cherry picked from commit 663dbc9)
gerrit-photon pushed a commit to vmware/photon that referenced this issue Jul 5, 2024
Refer: openssl/openssl#24611

Change-Id: Ie2cea6bf6359d3e6af0a2399473600816147a38b
Signed-off-by: Keerthana K <keerthana.kalyanasundaram@broadcom.com>
Reviewed-on: http://photon-jenkins.eng.vmware.com:8082/c/photon/+/24186
Tested-by: gerrit-photon <photon-checkins@vmware.com>
Reviewed-by: Srinidhi Rao <srinidhi.rao@broadcom.com>
Reviewed-by: Tapas Kundu <tapas.kundu@broadcom.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 branch: 3.2 Merge to openssl-3.2 branch: 3.3 Merge to openssl-3.3 severity: regression The issue/pr is a regression from previous released version triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants