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

MGF1 digest not set correctly when configuring RSA EVP_PKEY_CTX with OSSL_PARAMS #19290

Closed
scaro-axway opened this issue Sep 28, 2022 · 2 comments
Labels
branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch good first issue Bite size change that could be a good start triaged: bug The issue/pr is/fixes a bug

Comments

@scaro-axway
Copy link

Hi,

I came across an issue when using OpenSSL 3.0.5 API to encrypt and decrypt RSA data.
When using a single OSSL_PARAM array to configure both OAEP_DIGEST and MGF1_DIGEST, the mgf1 digest algorithm is not set as expected.

Here is a code sample showing the issue: mgf1 is set to "Sha256" instead of "Sha1".

#include <openssl/evp.h>
#include <openssl/core_names.h>
#include <openssl/rsa.h>
#include <stdio.h>

void main(int argc, char **argv)
{
  EVP_PKEY *key = EVP_RSA_gen(1024);
  EVP_PKEY_CTX *keyCtx = EVP_PKEY_CTX_new_from_pkey(0, key, 0);

  { // Set params
      int padding = RSA_PKCS1_OAEP_PADDING;
      OSSL_PARAM params[4];
      params[0] = OSSL_PARAM_construct_int(OSSL_SIGNATURE_PARAM_PAD_MODE, & padding);
      params[1] = OSSL_PARAM_construct_utf8_string(OSSL_ASYM_CIPHER_PARAM_OAEP_DIGEST,
                                                   SN_sha256, 0);
      params[2] = OSSL_PARAM_construct_utf8_string(OSSL_ASYM_CIPHER_PARAM_MGF1_DIGEST,
                                                   SN_sha1, 0);
      params[3] = OSSL_PARAM_construct_end();

      EVP_PKEY_encrypt_init_ex(keyCtx, params);
  }
  { // Read params
      OSSL_PARAM params[3];
      char       oaepmd[30] = { '\0' };
      char       mgf1md[30] = { '\0' };

      params[0] = OSSL_PARAM_construct_utf8_string(OSSL_ASYM_CIPHER_PARAM_OAEP_DIGEST,
                                                   oaepmd, sizeof(oaepmd));
      params[1] = OSSL_PARAM_construct_utf8_string(OSSL_ASYM_CIPHER_PARAM_MGF1_DIGEST,
                                                   mgf1md, sizeof(mgf1md));
      params[2] = OSSL_PARAM_construct_end();

      EVP_PKEY_CTX_get_params(keyCtx, params);
      printf("oaep = %s / mgf1 = %s\n", oaepmd, mgf1md);
  }
}

Trying to debug the issue lead to code located at rsa_enc.c:449.
In this use case, the 'str' pointer is no longer pointing to 'mdname': it was changed when processing the OAEP digest parameter at rsa_enc.c:440.

@scaro-axway scaro-axway added the issue: bug report The issue was opened to report a bug label Sep 28, 2022
@paulidale paulidale added branch: master Merge to master branch triaged: bug The issue/pr is/fixes a bug branch: 3.0 Merge to openssl-3.0 branch good first issue Bite size change that could be a good start and removed issue: bug report The issue was opened to report a bug labels Sep 28, 2022
@jamuir
Copy link
Member

jamuir commented Oct 4, 2022

I have a fix for this and will push it up shortly.

jamuir added a commit to jamuir/openssl that referenced this issue Oct 4, 2022
update rsa_set_ctx_params() so that the digest function used in the
MGF1 construction is set correctly.

Testing:
scaro-axway gave code to reproduce the defect in the github issue.
The code is supposed to set the rsa-oaep hash function to SHA2-256 and
mgf1 hash function to SHA1.  Here is the code:

  #include <openssl/evp.h>
  #include <openssl/core_names.h>
  #include <openssl/rsa.h>
  #include <stdio.h>

  void main(int argc, char **argv)
  {
    EVP_PKEY *key = EVP_RSA_gen(1024);
    EVP_PKEY_CTX *keyCtx = EVP_PKEY_CTX_new_from_pkey(0, key, 0);

    { // Set params
        int padding = RSA_PKCS1_OAEP_PADDING;
        OSSL_PARAM params[4];
        params[0] = OSSL_PARAM_construct_int(OSSL_SIGNATURE_PARAM_PAD_MODE, & padding);
        params[1] = OSSL_PARAM_construct_utf8_string(OSSL_ASYM_CIPHER_PARAM_OAEP_DIGEST,
                                                     SN_sha256, 0);
        params[2] = OSSL_PARAM_construct_utf8_string(OSSL_ASYM_CIPHER_PARAM_MGF1_DIGEST,
                                                     SN_sha1, 0);
        params[3] = OSSL_PARAM_construct_end();

        EVP_PKEY_encrypt_init_ex(keyCtx, params);
    }
    { // Read params
        OSSL_PARAM params[3];
        char       oaepmd[30] = { '\0' };
        char       mgf1md[30] = { '\0' };

        params[0] = OSSL_PARAM_construct_utf8_string(OSSL_ASYM_CIPHER_PARAM_OAEP_DIGEST,
                                                     oaepmd, sizeof(oaepmd));
        params[1] = OSSL_PARAM_construct_utf8_string(OSSL_ASYM_CIPHER_PARAM_MGF1_DIGEST,
                                                     mgf1md, sizeof(mgf1md));
        params[2] = OSSL_PARAM_construct_end();

        EVP_PKEY_CTX_get_params(keyCtx, params);
        printf("oaep = %s / mgf1 = %s\n", oaepmd, mgf1md);
    }
  }

before this commit:

  openssl/tmp$ make -B && ./issue-19290
  gcc -Wall -g -O0 -I../include -c issue-19290.c -o issue-19290.o
  gcc -Wall -g -O0 -I../include issue-19290.o ../libcrypto.a -o issue-19290
  oaep = SHA2-256 / mgf1 = SHA2-256

after this commit:

  openssl/tmp$ make -B && ./issue-19290
  gcc -Wall -g -O0 -I../include -c issue-19290.c -o issue-19290.o
  gcc -Wall -g -O0 -I../include issue-19290.o ../libcrypto.a -o issue-19290
  oaep = SHA2-256 / mgf1 = SHA1
@jamuir jamuir mentioned this issue Oct 4, 2022
2 tasks
jamuir added a commit to jamuir/openssl that referenced this issue Oct 5, 2022
Fixes openssl#19290

update rsa_set_ctx_params() so that the digest function used in the
MGF1 construction is set correctly.  Add a test for this to
evp_extra_test.c based on the code scaro-axway provided in openssl#19290.
jamuir added a commit to jamuir/openssl that referenced this issue Oct 5, 2022
Fixes openssl#19290

update rsa_set_ctx_params() so that the digest function used in the
MGF1 construction is set correctly.  Add a test for this to
evp_extra_test.c based on the code scaro-axway provided in openssl#19290.
jamuir added a commit to jamuir/openssl that referenced this issue Oct 5, 2022
Fixes openssl#19290

update rsa_set_ctx_params() so that the digest function used in the
MGF1 construction is set correctly.  Add a test for this to
evp_extra_test.c based on the code scaro-axway provided in openssl#19290.
openssl-machine pushed a commit that referenced this issue Oct 7, 2022
Fixes #19290

update rsa_set_ctx_params() so that the digest function used in the
MGF1 construction is set correctly.  Add a test for this to
evp_extra_test.c based on the code scaro-axway provided in #19290.

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #19342)

(cherry picked from commit e5a7536)
beldmit pushed a commit to beldmit/openssl that referenced this issue Dec 26, 2022
Fixes openssl#19290

update rsa_set_ctx_params() so that the digest function used in the
MGF1 construction is set correctly.  Add a test for this to
evp_extra_test.c based on the code scaro-axway provided in openssl#19290.

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#19342)
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 good first issue Bite size change that could be a good start triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants
@jamuir @paulidale @scaro-axway and others