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

set MGF1 digest correctly #19342

Closed
wants to merge 1 commit into from
Closed

set MGF1 digest correctly #19342

wants to merge 1 commit into from

Conversation

jamuir
Copy link
Member

@jamuir jamuir commented Oct 4, 2022

update rsa_set_ctx_params() so that the digest function used in the MGF1 construction is set correctly.

Fixes #19290

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
Checklist
  • documentation is added or updated
  • tests are added or updated

@jamuir jamuir changed the title set MGF1 digest correctly (#19290) set MGF1 digest correctly Oct 4, 2022
@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Oct 4, 2022
@t8m t8m added branch: master Merge to master branch approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member triaged: bug The issue/pr is/fixes a bug branch: 3.0 Merge to openssl-3.0 branch labels Oct 4, 2022
@t8m
Copy link
Member

t8m commented Oct 4, 2022

Instead of having the code to reproduce the issue in the git commit message, would you like to add it as a testcase somewhere - for example to evp_extra_test.c?

Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving, although having a testcase would be nice. Also the commit message should be amended as it is not necessary to have the reproducer there. And there should be Fixes #19290 on a separate line in the commit message instead of having the reference in the commit message title.

@t8m t8m removed the approval: otc review pending This pull request needs review by an OTC member label Oct 4, 2022
@jamuir
Copy link
Member Author

jamuir commented Oct 4, 2022

Thanks, Tomas. I will add a test-case and update the commit message.

@jamuir
Copy link
Member Author

jamuir commented Oct 5, 2022

@t8m : I've made the updates.

Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few minor adjustments needed for the testcase.

test/evp_extra_test.c Outdated Show resolved Hide resolved
test/evp_extra_test.c Outdated Show resolved Hide resolved
test/evp_extra_test.c Show resolved Hide resolved
test/evp_extra_test.c Outdated Show resolved Hide resolved
test/evp_extra_test.c Outdated Show resolved Hide resolved
test/evp_extra_test.c Outdated Show resolved Hide resolved
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 jamuir requested review from t8m and beldmit and removed request for t8m October 5, 2022 15:08
Copy link
Member

@beldmit beldmit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@beldmit beldmit added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Oct 5, 2022
Copy link
Member

@slontis slontis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few other places that use the 'str = ', like this, but they seem to do the right thing..

@openssl-machine openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Oct 6, 2022
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

openssl-machine pushed a commit that referenced this pull request 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)
@t8m
Copy link
Member

t8m commented Oct 7, 2022

Merged to master and 3.0 branches. Thank you for your contribution.

@t8m t8m closed this Oct 7, 2022
openssl-machine pushed a commit that referenced this pull request 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)
@openssl openssl deleted a comment Oct 8, 2022
beldmit pushed a commit to beldmit/openssl that referenced this pull request 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
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch severity: fips change The pull request changes FIPS provider sources triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MGF1 digest not set correctly when configuring RSA EVP_PKEY_CTX with OSSL_PARAMS
5 participants