Fix spurious error queue entries while loading private keys by DDvO · Pull Request #15283 · openssl/openssl

DDvO · 2021-05-14T13:28:53Z

This removes spurious error queue entries such as

ossl_store_handle_load_result:crypto/store/store_result.c:151:error: unsupported:
asn1_check_tlen:crypto/asn1/tasn_dec.c:1156:error: wrong tag:
asn1_item_embed_d2i:crypto/asn1/tasn_dec.c:322:error: nested asn1 error:Type=EC_PRIVATEKEY

when loading private keys via OSSL_STORE: .
The underlying issue was revealed by a recent improvement in #15253.

~~On this occasion, also~~

~~Make OSSL_CRMF_ENCRYPTEDVALUE_get1_encCert() conservative on the error queue~~
~~crypto/{http,cmp}: Remove obsolete comments w.r.t. ERR_clear_error()~~

crypto/encode_decode/decoder_lib.c

providers/implementations/encode_decode/decode_der2key.c

levitte · 2021-05-15T06:40:40Z

Do you have an example that resulted in that spurious error queue entry?

DDvO · 2021-05-15T07:04:42Z

Do you have an example that resulted in that spurious error queue entry?

Sure.

wget 'http://pki.certificate.fi:8081/install-ca-cert.html/ca-certificate.crt?ca-id=632&download-certificate=1' -O insta.ca.crt
apps/openssl ecparam -genkey -name prime256v1 -out insta.priv.pem
apps/openssl cmp -config apps/openssl.cnf -section insta

yields

cmp_main:apps/cmp.c:2581:CMP info: using section(s) 'insta' of OpenSSL configuration file 'apps/openssl.cnf'
setup_client_ctx:apps/cmp.c:1891:CMP info: will contact http://pki.certificate.fi:8700/pkix/
ossl_store_handle_load_result:crypto/store/store_result.c:151:CMP error: unsupported:
asn1_check_tlen:crypto/asn1/tasn_dec.c:1156:CMP error: wrong tag:
asn1_item_embed_d2i:crypto/asn1/tasn_dec.c:322:CMP error: nested asn1 error:Type=EC_PRIVATEKEY
send_receive_check:crypto/cmp/cmp_client.c:167:CMP info: sending IR
send_receive_check:crypto/cmp/cmp_client.c:187:CMP info: received IP
send_receive_check:crypto/cmp/cmp_client.c:167:CMP info: sending CERTCONF
send_receive_check:crypto/cmp/cmp_client.c:187:CMP info: received PKICONF
save_free_certs:apps/cmp.c:1940:CMP info: received 1 extra certificate(s), saving to file 'insta.extracerts.pem'
save_free_certs:apps/cmp.c:1940:CMP info: received 1 enrolled certificate(s), saving to file 'insta.cert.pem'

When loading RSA keys instead this does not happen.

Update: A simpler way to reproduce this is:

apps/openssl ecparam -genkey -name prime256v1 -out privkey-with-params.pem
apps/openssl pkey -in  privkey-with-params.pem

DDvO · 2021-05-15T07:09:38Z

With the proposed changes there is a test failure on

make run_tests TESTS=test_x509

which appears to indicate a different type of uncleanness regarding the error queue:

25-test_x509.t .. 1/18 
../../util/wrap.pl ../../apps/openssl x509 -in ../../test/certs/v3-certs-RC2.p12 -passin 'pass:v3-certs' 2> out.txt => 1
Could not read certificate from ../../test/certs/v3-certs-RC2.p12
Unable to load certificate
not ok 17 - load v3-certs-RC2 no asn1 errors
25-test_x509.t .. 17/18 --------------------------------------------------------
25-test_x509.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/18 subtests

levitte · 2021-05-15T07:39:34Z

Ah, the spurious error happens because insta.priv.pem holds two items, demonstrated like this:

$ ./util/wrap.pl apps/openssl storeutl insta.priv.pem 
0: Parameters
-----BEGIN EC PARAMETERS-----
BggqhkjOPQMBBw==
-----END EC PARAMETERS-----
1: Pkey
-----BEGIN PRIVATE KEY-----
MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQgB3KN9BGnVLlGtTRR
QJTqyn2fS1OH390SzH5NRSLpZB+hRANCAAT95NrJeMI0BP50A/Yx0G88ZY1EJiP7
wOi12pAaMM+nN5iyed17ro8Y4JgIEJcSAt0rNo24FPHb013p/PrEeLXb
-----END PRIVATE KEY-----
Total found: 2

The point where this file is loaded is here:

openssl/apps/cmp.c

Line 630 in 773f1c3

EVP_PKEY *pkey = load_key(uri, format, 0, pass_string, eng, desc);

That will, of course, only accept private keys, not key params.

This has me think about recent development, where we're taking a closer look at the extected STORE_INFO type in ossl_store_handle_load_result() and its sub-functions. It seems to me that the semantics of what this function returns has shifted slightly because we're not making a difference between an error when decoding the contents and that the data doesn't match the expected STORE_INFO type. ossl_store_handle_load_result() should return true, with a NULL *v for the latter, and since it currently doesn't, the NULL STORE_INFO is returned all the way here, and you get that spurious error printed:

openssl/apps/lib/apps.c

Lines 978 to 991 in 773f1c3

    
                   /* 
        
                    * This can happen (for example) if we attempt to load a file with 
        
                    * multiple different types of things in it - but the thing we just 
        
                    * tried to load wasn't one of the ones we wanted, e.g. if we're trying 
        
                    * to load a certificate but the file has both the private key and the 
        
                    * certificate in it. We just retry until eof. 
        
                    */ 
        
                   if (info == NULL) { 
        
                       if (OSSL_STORE_error(ctx)) { 
        
                           ERR_print_errors(bio_err); 
        
                           ERR_clear_error(); 
        
                       } 
        
                       continue; 
        
                   }

So, it seems to come down to store_result.c and what it does when the data doesn't match expectations.

DDvO · 2021-05-15T08:06:58Z

Indeed the errors occur only with the

 -----BEGIN EC PARAMETERS-----
BggqhkjOPQMBBw==
-----END EC PARAMETERS-----

portion of the private key file!
And their

ossl_store_handle_load_result:crypto/store/store_result.c:151:CMP error: unsupported:

part is unclear (if given at all, it should mention that it does not like the parameters found), while

asn1_check_tlen:crypto/asn1/tasn_dec.c:1156:CMP error: wrong tag:
asn1_item_embed_d2i:crypto/asn1/tasn_dec.c:322:CMP error: nested asn1 error:Type=EC_PRIVATEKEY

is misleading because the problem is not the EC PRIVATE KEY part but the EC PARAMETERS part.

DDvO · 2021-05-15T08:34:23Z

Actually also

ossl_store_handle_load_result:crypto/store/store_result.c:151:CMP error: unsupported:

is misleading in my view since EC PARAMETERS are not unsupported, just unexpected.

DDvO · 2021-05-15T08:52:06Z

So I suspect that in

    ERR_set_mark();
    if (*v == NULL && !try_name(&helper_data, v))
        goto err;
    ERR_pop_to_mark();
    ERR_set_mark();
    if (*v == NULL && !try_key(&helper_data, v, ctx, provider, libctx, propq))
        goto err;
    ERR_pop_to_mark();
    ERR_set_mark();
    if (*v == NULL && !try_cert(&helper_data, v, libctx, propq))
        goto err;
    ERR_pop_to_mark();
    ERR_set_mark();
    if (*v == NULL && !try_crl(&helper_data, v, libctx, propq))
        goto err;
    ERR_pop_to_mark();
    ERR_set_mark();
    if (*v == NULL && !try_pkcs12(&helper_data, v, ctx, libctx, propq))
        goto err;
    ERR_pop_to_mark();

a handling for the (EC) PARAMETERS case (OSSL_STORE_INFO_PARAMS) is missing?

DDvO · 2021-05-15T11:02:43Z

I've just found a very simple preliminary fix for the reported spurious update: 'unsupported' error problem,
but not for the deeper ones we discovered here during the analysis.

@levitte, will you take care of a deeper/proper solution (which is not urgent in my view)?

levitte · 2021-05-15T13:42:33Z

Please drop your changes to providers/implementations/encode_decode/decode_der2key.c

DDvO · 2021-05-15T14:14:21Z

Please drop your changes to providers/implementations/encode_decode/decode_der2key.c

Unfortunately these are still needed to prevent

asn1_check_tlen:crypto/asn1/tasn_dec.c:1156:CMP error: wrong tag:
asn1_item_embed_d2i:crypto/asn1/tasn_dec.c:322:CMP error: nested asn1 error:Type=EC_PRIVATEKEY

DDvO · 2021-05-15T14:15:28Z

Or what else do you concretely suggest to get rid of those?

DDvO · 2021-05-15T14:37:11Z

And is my latest quick-fix change to ossl_store_handle_load_result():

    if (*v == NULL && helper_data.object_type == OSSL_OBJECT_PKEY)
        return 0;

acceptable (for the time being) after all?

levitte · 2021-05-15T15:40:45Z

Or what else do you concretely suggest to get rid of those?

I would have a ERR_set_mark() / ERR_pop_to_mark() pair in here:

openssl/apps/lib/apps.c

Lines 978 to 991 in 773f1c3

    
                   /* 
        
                    * This can happen (for example) if we attempt to load a file with 
        
                    * multiple different types of things in it - but the thing we just 
        
                    * tried to load wasn't one of the ones we wanted, e.g. if we're trying 
        
                    * to load a certificate but the file has both the private key and the 
        
                    * certificate in it. We just retry until eof. 
        
                    */ 
        
                   if (info == NULL) { 
        
                       if (OSSL_STORE_error(ctx)) { 
        
                           ERR_print_errors(bio_err); 
        
                           ERR_clear_error(); 
        
                       } 
        
                       continue; 
        
                   }

and simply remove the error printing

levitte · 2021-05-15T15:43:28Z

@levitte, will you take care of a deeper/proper solution (which is not urgent in my view)?

Yes, I plan on doing so.

DDvO · 2021-05-15T15:55:57Z

@levitte, will you take care of a deeper/proper solution (which is not urgent in my view)?

Yes, I plan on doing so.

Pleased to hear.

DDvO · 2021-05-15T15:59:22Z

Or what else do you concretely suggest to get rid of those?

I would have a ERR_set_mark() / ERR_pop_to_mark() pair in here:

openssl/apps/lib/apps.c

Lines 978 to 991 in 773f1c3

/*

* This can happen (for example) if we attempt to load a file with

* multiple different types of things in it - but the thing we just

* tried to load wasn't one of the ones we wanted, e.g. if we're trying

* to load a certificate but the file has both the private key and the

* certificate in it. We just retry until eof.

*/

if (info == NULL) {

if (OSSL_STORE_error(ctx)) {

ERR_print_errors(bio_err);

ERR_clear_error();

}

continue;

}

and simply remove the error printing

This appears strange to me. For instance like this?

            ERR_set_mark();
            if (OSSL_STORE_error(ctx)) {
#if 0
                ERR_print_errors(bio_err);
                ERR_clear_error();
#endif
            }
            ERR_pop_to_mark();

As expected, this (or having the pair outside the block) does not prevent the error printed by

apps/openssl cmp -config apps/openssl.cnf -section insta

DDvO · 2021-05-15T16:00:38Z

As explained above, I am pretty sure that the trial-and-error construct in der2key_decode() needs to be adapted.

DDvO · 2021-05-15T16:23:15Z

How if we simply defer this (as WIP) to the beta phase?
Or if we commit my preliminary solution but mark it as, e.g. "TODO(3.0) improve" and open an issue for that?

levitte · 2021-05-15T16:27:47Z

I would like to ask you for a little patience. I intend to use the weekend for something else.

openssl-machine · 2021-06-15T00:06:48Z

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

openssl-machine · 2021-07-16T00:07:02Z

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

mattcaswell · 2021-07-16T09:50:14Z

Is this needed for 3.0? It does not have the 3.0 milestone at the moment

DDvO · 2021-12-20T10:30:59Z

I lost track of this PR on September,
but after yesterday's reminder by the openssl-machine,
I've just brushed it up, making it ready for further (hopefully final) reviewing.

t8m · 2021-12-20T11:33:16Z

crypto/crmf/crmf_lib.c

This is not a constant time operation.

I do not see why this could be problem here.
Using ERR_clear_error() was too aggressive since it deletes all errors, also unrelated ones.
And using ERR_set_mark(); ... ERR_pop_to_mark(); is the usual pattern to get rid of any - and only those - error queue entries potentially added by a function call.
If there is a more efficient way of achieving this please let me know.

Please note that the following code uses constant time operation. And ERR_clear_error() is constant time unless allocated err data is present.

Ah, now I see what you are after.

Hmm, looks like even ERR_clear_error() is not really constant-time due to OPENSSL_free(es->err_file[i]), where es->err_file[i] should be NULL if and only if no entry is present at index i.

And I doubt that constant-time behavior is relevant here because at this point an encrypted newly enrolled certificate (which BTW is extremely rare) has been received and is being decrypted. This only happens under the control of the client requesting a certficate and does not happen often.

The constant_time_* calls go back to #9774 (comment) and #9774 (comment).
To me, following the "best practice" mentioned there appears not really need.

If you prefer, I could move the ERR_pop_to_mark(); after those.

Please remove the set_mark/pop_to_mark as well.

Adding the set_mark/pop_to_mark was the actual point of that commit.
As explained, using ERR_clear_error() was too harsh.
We should remove only those entries that were (potentially) added by EVP_PKEY_decrypt().

I mean there is no point in clearing anything as there should be nothing security sensitive in error queue under the same assumptions that lead to removing the consttime calls.

Ah, right, so we can simply get rid also of the ERR_clear_error() - even better!
Done so, rebasing and squashing the changes to crmf_lib.c.

According to the ongoing exchange on #17319,
I've moved the changes to crmf_lib.c form this PR to an independent one: #17354.

DDvO · 2022-01-04T16:14:46Z

@t8m, can we please finalize this now,
since the changes still under discussion have been moved to #17354?

providers/implementations/encode_decode/decode_der2key.c

t8m

It would be nice to have some testcases to test for error reporting. But that is for different PR I think.

openssl-machine · 2022-01-05T20:00:18Z

This pull request is ready to merge

Reviewed-by: Tomas Mraz <[email protected]> (Merged from #15283)

Reviewed-by: Tomas Mraz <[email protected]> (Merged from #15283) (cherry picked from commit da198ad)

DDvO · 2022-01-06T08:16:13Z

Merged - thanks @levitte and @t8m

DDvO added approval: otc review pending triaged: bug The issue/pr is/fixes a bug labels May 14, 2021

DDvO force-pushed the fix_spurious_key_load_errors branch from 77811c0 to b97067a Compare May 14, 2021 13:58

levitte requested changes May 14, 2021

View reviewed changes

crypto/encode_decode/decoder_lib.c Outdated Show resolved Hide resolved

providers/implementations/encode_decode/decode_der2key.c Outdated Show resolved Hide resolved

DDvO closed this May 15, 2021

DDvO reopened this May 15, 2021

DDvO force-pushed the fix_spurious_key_load_errors branch from b97067a to 5f4caa7 Compare May 15, 2021 05:28

DDvO force-pushed the fix_spurious_key_load_errors branch from c41b255 to 8d5ceb6 Compare May 15, 2021 11:01

DDvO requested review from levitte and t8m December 20, 2021 10:28

t8m added approval: otc review pending and removed waiting-for: contributor response This pull request is awaiting a response by the contributor labels Dec 20, 2021

t8m reviewed Dec 20, 2021

View reviewed changes

DDvO mentioned this pull request Dec 20, 2021

Use of EVP_PKEY_decrypt() should be more secure #17319

Closed

DDvO requested a review from t8m December 21, 2021 16:09

t8m added the triaged: refactor The issue/pr requests/implements refactoring label Dec 21, 2021

DDvO force-pushed the fix_spurious_key_load_errors branch from 4416068 to 942e668 Compare December 21, 2021 17:38

OSSL_STORE: Prevent spurious error during loading private keys

1196fe5

DDvO force-pushed the fix_spurious_key_load_errors branch from 942e668 to 1196fe5 Compare December 25, 2021 12:43

DDvO mentioned this pull request Dec 25, 2021

CMS, PKCS7, and CRMF: simplify use of EVP_PKEY_decrypt() by helper function #17354

Closed

DDvO added the branch: 3.0 Applies to openssl-3.0 branch label Jan 4, 2022

t8m reviewed Jan 4, 2022

View reviewed changes

providers/implementations/encode_decode/decode_der2key.c Outdated Show resolved Hide resolved

t8m reviewed Jan 4, 2022

View reviewed changes

providers/implementations/encode_decode/decode_der2key.c Show resolved Hide resolved

fixup! OSSL_STORE: Prevent spurious error during loading private keys

79a0eab

DDvO requested a review from t8m January 4, 2022 17:24

t8m approved these changes Jan 4, 2022

View reviewed changes

t8m added approval: done This pull request has the required number of approvals and removed approval: otc review pending triaged: refactor The issue/pr requests/implements refactoring labels Jan 4, 2022

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 Jan 5, 2022

openssl-machine pushed a commit that referenced this pull request Jan 6, 2022

OSSL_STORE: Prevent spurious error during loading private keys

da198ad

Reviewed-by: Tomas Mraz <[email protected]> (Merged from #15283)

openssl-machine pushed a commit that referenced this pull request Jan 6, 2022

OSSL_STORE: Prevent spurious error during loading private keys

7a30610

Reviewed-by: Tomas Mraz <[email protected]> (Merged from #15283) (cherry picked from commit da198ad)

DDvO closed this Jan 6, 2022

Uh oh!

Conversation

DDvO commented May 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

levitte commented May 15, 2021

Uh oh!

DDvO commented May 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DDvO commented May 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

levitte commented May 15, 2021

Uh oh!

DDvO commented May 15, 2021

Uh oh!

DDvO commented May 15, 2021

Uh oh!

DDvO commented May 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DDvO commented May 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

levitte commented May 15, 2021

Uh oh!

DDvO commented May 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DDvO commented May 15, 2021

Uh oh!

DDvO commented May 15, 2021

Uh oh!

levitte commented May 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

levitte commented May 15, 2021

Uh oh!

DDvO commented May 15, 2021

Uh oh!

DDvO commented May 15, 2021

Uh oh!

DDvO commented May 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DDvO commented May 15, 2021

Uh oh!

levitte commented May 15, 2021

Uh oh!

openssl-machine commented Jun 15, 2021

Uh oh!

openssl-machine commented Jul 16, 2021

Uh oh!

mattcaswell commented Jul 16, 2021

Uh oh!

DDvO commented Dec 20, 2021

Uh oh!

t8m Dec 20, 2021

Choose a reason for hiding this comment

Uh oh!

DDvO Dec 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

t8m Dec 20, 2021

Choose a reason for hiding this comment

Uh oh!

DDvO Dec 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DDvO Dec 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

t8m Dec 21, 2021

Choose a reason for hiding this comment

DDvO commented May 14, 2021 •

edited

Loading

DDvO commented May 15, 2021 •

edited

Loading

DDvO commented May 15, 2021 •

edited

Loading

DDvO commented May 15, 2021 •

edited

Loading

DDvO commented May 15, 2021 •

edited

Loading

DDvO commented May 15, 2021 •

edited

Loading

levitte commented May 15, 2021 •

edited

Loading

DDvO commented May 15, 2021 •

edited

Loading

DDvO Dec 20, 2021 •

edited

Loading

DDvO Dec 20, 2021 •

edited

Loading

DDvO Dec 20, 2021 •

edited

Loading

DDvO Dec 21, 2021 •

edited

Loading