apps/ca.c: Handle EVP_PKEY_get_default_digest_name() returning 1 with "UNDEF" #20460

levitte · 2023-03-08T11:04:27Z

EVP_PKEY_get_default_digest_name() may return 1 with the returned digest
name "UNDEF". This case hasn't been documented, and the meaning has been
left undefined, until now.

Related to #20428

… "UNDEF" EVP_PKEY_get_default_digest_name() may return 1 with the returned digest name "UNDEF". This case hasn't been documented, and the meaning has been left undefined, until now.

levitte · 2023-03-08T11:05:08Z

I felt a little unsure if this should go to 3.0, even though #20430 did...

levitte · 2023-03-08T11:08:41Z

@baentsch, please have a look here

levitte · 2023-03-08T11:12:19Z

doc/man3/EVP_PKEY_get_default_digest_nid.pod

-ending NUL byte.  The name could be C<"UNDEF">, signifying that no digest
-should be used.
+ending NUL byte.  The name could be C<"UNDEF">, signifying that a digest
+must (for return value 2) or may (for return value 1) be left unspecified.


Strictly speaking, this was possible with EVP_PKEY_get_default_digest_nid() as well. No EVP_PKEY_ASN1_METHOD that I know actually did so, but it's perfectly possible to have the ctrl function to *pnid = NID_undef and return 1.

t8m · 2023-03-08T11:49:54Z

Should there be a test?

If it can be treated as bug fix, then it should go to 3.0 as well.

apps/ca.c

doc/man3/EVP_PKEY_get_default_digest_nid.pod

baentsch · 2023-03-08T12:56:21Z

@baentsch, please have a look here

Just did. What I tried to test this out was to return only "OSSL_PKEY_PARAM_DEFAULT_DIGEST" from oqsprovider (value "UNDEF"). This now passes the previously "suboptimal" checks at

openssl/apps/ca.c

Lines 810 to 824 in 9313694

    
           if (def_ret == 2 && strcmp(def_dgst, "UNDEF") == 0) { 
        
               /* The signing algorithm requires there to be no digest */ 
        
               dgst = NULL; 
        
           } else if (dgst == NULL 
        
                      && (dgst = lookup_conf(conf, section, ENV_DEFAULT_MD)) == NULL) { 
        
               goto end; 
        
           } else { 
        
               if (strcmp(dgst, "default") == 0) { 
        
                   if (def_ret <= 0) { 
        
                       BIO_puts(bio_err, "no default digest\n"); 
        
                       goto end; 
        
                   } 
        
                   dgst = def_dgst; 
        
               } 
        
           }

updated via this PR (good!) but unfortunately now triggers a problem at

openssl/crypto/evp/m_sigver.c

Lines 205 to 214 in 9313694

    
           reinitialize: 
        
              if (pctx != NULL) 
        
                  *pctx = locpctx; 
        
              if (type != NULL) { 
        
                  ctx->reqdigest = type; 
        
                  if (mdname == NULL) 
        
                      mdname = canon_mdname(EVP_MD_get0_name(type)); 
        
              } else { 
        
                  if (mdname == NULL && !reinit) {

: (bad): It is now possible (don't know exactly why, though) to hit this label with an mdname="UNDEF". Patching this as follows:

diff --git a/crypto/evp/m_sigver.c b/crypto/evp/m_sigver.c
index 630d339c35..3286826ba9 100644
--- a/crypto/evp/m_sigver.c
+++ b/crypto/evp/m_sigver.c
@@ -206,6 +206,7 @@ static int do_sigver_init(EVP_MD_CTX *ctx, EVP_PKEY_CTX **pctx,
     if (pctx != NULL)
         *pctx = locpctx;
 
+    mdname = canon_mdname(mdname);
     if (type != NULL) {
         ctx->reqdigest = type;
         if (mdname == NULL)
@@ -218,7 +219,6 @@ static int do_sigver_init(EVP_MD_CTX *ctx, EVP_PKEY_CTX **pctx,
                 mdname = canon_mdname(locmdname);
             }
         }

has everything working fine (an openssl ca test built into oqsprovider).

levitte · 2023-03-08T14:47:15Z

diff --git a/crypto/evp/m_sigver.c b/crypto/evp/m_sigver.c
index 630d339c35..3286826ba9 100644
--- a/crypto/evp/m_sigver.c
+++ b/crypto/evp/m_sigver.c
@@ -206,6 +206,7 @@ static int do_sigver_init(EVP_MD_CTX *ctx, EVP_PKEY_CTX **pctx,
     if (pctx != NULL)
         *pctx = locpctx;
 
+    mdname = canon_mdname(mdname);
     if (type != NULL) {
         ctx->reqdigest = type;
         if (mdname == NULL)
@@ -218,7 +219,6 @@ static int do_sigver_init(EVP_MD_CTX *ctx, EVP_PKEY_CTX **pctx,
                 mdname = canon_mdname(locmdname);
             }
         }

That looks a bit late... how about this?

diff --git a/crypto/evp/m_sigver.c b/crypto/evp/m_sigver.c
index 630d339c35..40312d4d79 100644
--- a/crypto/evp/m_sigver.c
+++ b/crypto/evp/m_sigver.c
@@ -88,6 +88,7 @@ static int do_sigver_init(EVP_MD_CTX *ctx, EVP_PKEY_CTX **pctx,
         goto err;
     }
 
+    mdname = canon_mdname(mdname);
     if (!reinit) {
         evp_pkey_ctx_free_old_ops(locpctx);
     } else {

… 1 with "UNDEF"

levitte · 2023-03-08T14:52:53Z

Should there be a test?

I was afraid you were going to ask this.... I'm afraid that writing a test for this is a large body of work.

baentsch · 2023-03-08T15:09:37Z

That looks a bit late... how about this?

LGTM (and allows my CA test suite to pass).

baentsch · 2023-03-08T15:19:40Z

I'm afraid that writing a test for this is a large body of work.

Completely concur. What could be called for are tests for all openssl commands utilizing providers -- times all their parameter options -- that's a true "test case explosion". The underlying issue for this PR has been triggered by openssl ca -provider .....

t8m · 2023-03-08T16:09:18Z

Should there be a test?

I was afraid you were going to ask this.... I'm afraid that writing a test for this is a large body of work.

Then tests: exempted would apply.

t8m · 2023-03-08T16:10:58Z

diff --git a/crypto/evp/m_sigver.c b/crypto/evp/m_sigver.c
index 630d339c35..3286826ba9 100644
--- a/crypto/evp/m_sigver.c
+++ b/crypto/evp/m_sigver.c
@@ -206,6 +206,7 @@ static int do_sigver_init(EVP_MD_CTX *ctx, EVP_PKEY_CTX **pctx,
     if (pctx != NULL)
         *pctx = locpctx;
 
+    mdname = canon_mdname(mdname);
     if (type != NULL) {
         ctx->reqdigest = type;
         if (mdname == NULL)
@@ -218,7 +219,6 @@ static int do_sigver_init(EVP_MD_CTX *ctx, EVP_PKEY_CTX **pctx,
                 mdname = canon_mdname(locmdname);
             }
         }

That looks a bit late... how about this?

diff --git a/crypto/evp/m_sigver.c b/crypto/evp/m_sigver.c
index 630d339c35..40312d4d79 100644
--- a/crypto/evp/m_sigver.c
+++ b/crypto/evp/m_sigver.c
@@ -88,6 +88,7 @@ static int do_sigver_init(EVP_MD_CTX *ctx, EVP_PKEY_CTX **pctx,
         goto err;
     }
 
+    mdname = canon_mdname(mdname);
     if (!reinit) {
         evp_pkey_ctx_free_old_ops(locpctx);
     } else {

The question is why the UNDEF mdname is passed to the do_sigver_init() at all.

levitte · 2023-03-08T17:08:24Z

The question is why the UNDEF mdname is passed to the do_sigver_init() at all.

Someone may have received it by calling EVP_PKEY_get_default_digest_name() and passed it to any of the functions EVP_DigestSignInit_ex(), EVP_DigestSignInit(), EVP_DigestVerifyInit_ex() or EVP_DigestVerifyInit().

t8m · 2023-03-08T17:50:25Z

In that case I would consider the change suggested in #20460 (comment) as a API convenience improvement. However the question is why @baentsch sees it in his tests?

openssl-machine · 2023-04-08T00:09:23Z

This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago

levitte · 2023-04-15T06:40:57Z

In that case I would consider the change suggested in #20460 (comment) as a API convenience improvement. However the question is why @baentsch sees it in his tests?

I don't understand this question. UNDEF is a documented valid value under certain circumstances, just like NID_undef is a valid value when EVP_PKEY_get_default_digest_nid() is called, and @baentsch triggers exactly those circumstances... except his specific circumstance was undocumented, because no one has thought of the case where the return value is 1 and the passed back digest nid/name is NID_undef/UNDEF... except we do handle that in code:

openssl/crypto/evp/p_lib.c

Lines 1332 to 1338 in 7eab768

    
           int EVP_PKEY_get_default_digest_name(EVP_PKEY *pkey, 
        
                                                char *mdname, size_t mdname_sz) 
        
           { 
        
               if (pkey->ameth == NULL) 
        
                   return evp_keymgmt_util_get_deflt_digest_name(pkey->keymgmt, 
        
                                                                 pkey->keydata, 
        
                                                                 mdname, mdname_sz);

openssl/crypto/evp/keymgmt_lib.c

Lines 560 to 566 in 7eab768

    
           } else if (OSSL_PARAM_modified(params)) { 
        
               if (params[0].return_size <= 1) /* Only a NUL byte */ 
        
                   result = SN_undef; 
        
               else 
        
                   result = mddefault; 
        
               rv = 1; 
        
           }

openssl-machine · 2023-05-05T21:00:18Z

This pull request is ready to merge

t8m · 2023-05-09T15:34:39Z

So, is this a bug fix? If so IMO it should go to 3.0 too.

paulidale · 2023-05-10T00:32:15Z

I think it's a bug fix, if it isn't it can't go to 3.1.

t8m · 2023-05-10T11:54:43Z

@tmshort @levitte ok with merging to 3.0?

levitte · 2023-06-07T14:58:48Z

@tmshort @levitte ok with merging to 3.0?

I am

t8m · 2023-06-15T12:04:57Z

Merged to master, 3.1, and 3.0 branches. Thank you.

… "UNDEF" EVP_PKEY_get_default_digest_name() may return 1 with the returned digest name "UNDEF". This case hasn't been documented, and the meaning has been left undefined, until now. Reviewed-by: Todd Short <todd.short@me.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #20460) (cherry picked from commit af99d55)

… "UNDEF" EVP_PKEY_get_default_digest_name() may return 1 with the returned digest name "UNDEF". This case hasn't been documented, and the meaning has been left undefined, until now. Reviewed-by: Todd Short <todd.short@me.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #20460)

… "UNDEF" EVP_PKEY_get_default_digest_name() may return 1 with the returned digest name "UNDEF". This case hasn't been documented, and the meaning has been left undefined, until now. Reviewed-by: Todd Short <todd.short@me.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #20460) (cherry picked from commit af99d55)

apps/ca.c: Handle EVP_PKEY_get_default_digest_name() returning 1 with…

ed80d74

… "UNDEF" EVP_PKEY_get_default_digest_name() may return 1 with the returned digest name "UNDEF". This case hasn't been documented, and the meaning has been left undefined, until now.

levitte 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 branch: 3.1 Merge to openssl-3.1 labels Mar 8, 2023

levitte self-assigned this Mar 8, 2023

levitte requested review from a team March 8, 2023 11:04

levitte mentioned this pull request Mar 8, 2023

Default and mandatory digest for provider-based signature algorithms #20428

Closed

levitte commented Mar 8, 2023

View reviewed changes

t8m approved these changes Mar 8, 2023

View reviewed changes

t8m added the triaged: bug The issue/pr is/fixes a bug label Mar 8, 2023

t8m removed the approval: otc review pending This pull request needs review by an OTC member label Mar 8, 2023

baentsch reviewed Mar 8, 2023

View reviewed changes

apps/ca.c Outdated Show resolved Hide resolved

baentsch reviewed Mar 8, 2023

View reviewed changes

doc/man3/EVP_PKEY_get_default_digest_nid.pod Show resolved Hide resolved

fixup! apps/ca.c: Handle EVP_PKEY_get_default_digest_name() returning…

0e35917

… 1 with "UNDEF"

t8m added the tests: exempted The PR is exempt from requirements for testing label Mar 8, 2023

tmshort approved these changes May 4, 2023

View reviewed changes

tmshort 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 May 4, 2023

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 May 5, 2023

t8m added the branch: 3.0 Merge to openssl-3.0 branch label May 10, 2023

t8m closed this Jun 15, 2023

levitte deleted the fix-apps-ca-regarding-EVP_PKEY_get_default_digest_name branch June 15, 2023 12:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

apps/ca.c: Handle EVP_PKEY_get_default_digest_name() returning 1 with "UNDEF" #20460

apps/ca.c: Handle EVP_PKEY_get_default_digest_name() returning 1 with "UNDEF" #20460

levitte commented Mar 8, 2023

levitte commented Mar 8, 2023

levitte commented Mar 8, 2023

levitte Mar 8, 2023

t8m commented Mar 8, 2023

baentsch commented Mar 8, 2023 •

edited by levitte

levitte commented Mar 8, 2023

levitte commented Mar 8, 2023

baentsch commented Mar 8, 2023

baentsch commented Mar 8, 2023

t8m commented Mar 8, 2023

t8m commented Mar 8, 2023

levitte commented Mar 8, 2023

t8m commented Mar 8, 2023

openssl-machine commented Apr 8, 2023

levitte commented Apr 15, 2023 •

edited

openssl-machine commented May 5, 2023

t8m commented May 9, 2023

paulidale commented May 10, 2023

t8m commented May 10, 2023

levitte commented Jun 7, 2023

t8m commented Jun 15, 2023

apps/ca.c: Handle EVP_PKEY_get_default_digest_name() returning 1 with "UNDEF" #20460

apps/ca.c: Handle EVP_PKEY_get_default_digest_name() returning 1 with "UNDEF" #20460

Conversation

levitte commented Mar 8, 2023

levitte commented Mar 8, 2023

levitte commented Mar 8, 2023

levitte Mar 8, 2023

Choose a reason for hiding this comment

t8m commented Mar 8, 2023

baentsch commented Mar 8, 2023 • edited by levitte

levitte commented Mar 8, 2023

levitte commented Mar 8, 2023

baentsch commented Mar 8, 2023

baentsch commented Mar 8, 2023

t8m commented Mar 8, 2023

t8m commented Mar 8, 2023

levitte commented Mar 8, 2023

t8m commented Mar 8, 2023

openssl-machine commented Apr 8, 2023

levitte commented Apr 15, 2023 • edited

openssl-machine commented May 5, 2023

t8m commented May 9, 2023

paulidale commented May 10, 2023

t8m commented May 10, 2023

levitte commented Jun 7, 2023

t8m commented Jun 15, 2023

baentsch commented Mar 8, 2023 •

edited by levitte

levitte commented Apr 15, 2023 •

edited