ASN.1: Refuse to encode to DER if non-optional items are missing [3.0] by levitte · Pull Request #16036 · openssl/openssl

levitte · 2021-07-09T07:06:47Z

…tent The test case creates an RSA public key and tries to pass it through i2d_PrivateKey(). This SHOULD fail, since the private bits are missing.

Fixes openssl#16026

ASN1_FBOOLEAN is designed to use as a default for optional ASN1 items. This test program used it for non-optional items, which doesn't encode well.

Make it only report (and fail on) encoding/decoding failures when success is expected.

levitte · 2021-07-09T07:09:19Z

This is like #16027, but for 3.0

t8m · 2021-07-09T07:55:31Z

The CI failure is only CMP related - perhaps @DDvO can look at it? I suppose the test makes some incorrect expectations?

levitte · 2021-07-09T07:56:50Z

I suppose the test makes some incorrect expectations?

I have no idea... test/cmp_ctx_test.c is too macroised for me to follow, not to mention debug.

levitte · 2021-07-09T08:36:06Z

(irrelevant comment removed)

levitte · 2021-07-09T08:50:55Z

So it appears that this particular test tries to set an incomplete certificate (signature algorithm is unset):

openssl/test/cmp_ctx_test.c

Line 822 in 0007ff2

ADD_TEST(test_CTX_set1_get0_srvCert);

The result is that OSSL_CMP_CTX_set1_srvCert() tries to call ossl_x509v3_cache_extensions(), which fails because the certificate fingerprint couldn't be calculated (the calculation is done on the DER encoding of the certificate, but since it's incomplete, the DER encoding will fail with ASN1_R_ILLEGAL_ZERO_CONTENTS).

levitte · 2021-07-09T09:09:52Z

I suppose that for that test, the following lines:

openssl/test/cmp_ctx_test.c

Lines 341 to 342 in 0007ff2

    
           TYPE val1_to_free = NEW; \ 
        
           TYPE val1 = val1_to_free; \

... translate to:

    X509 *val1_to_free = X509_new();
    X509 *val1 = val1_to_free;

and with set_fn == OSSL_CMP_CTX_set1_srvCert, it's clear why the following test fails:

openssl/test/cmp_ctx_test.c

Lines 371 to 374 in 0007ff2

    
           if (!(*set_fn)(ctx, val1)) { \ 
        
               TEST_error("setting first value failed"); \ 
        
               res = 0; \ 
        
           } \

t8m · 2021-07-09T09:25:33Z

I suppose replacing all the X509_new() calls in the test with calls to X509_dup() of a real certificate should fix it?

DDvO · 2021-07-09T09:42:40Z

I suppose replacing all the X509_new() calls in the test with calls to X509_dup() of a real certificate should fix it?

The underlying issue is that X509_new() produces an invalid X.509 structure, not having the mandatory fields.
Indeed, copying a real cert would be a workaround.

DDvO · 2021-07-09T10:00:20Z

(irrelevant comment removed)

The stack trace you originally provided here was of interest.

#9  0x0000555555667485 in X509_digest (cert=0x5555559b99b0, 
    md=0x55555596a4e0 <sha1_md>, data=0x5555559b9ae8 "", len=0x0)
    at ../master/crypto/x509/x_all.c:432
#10 0x000055555564f210 in ossl_x509v3_cache_extensions (x=0x5555559b99b0)
    at ../master/crypto/x509/v3_purp.c:421
#11 0x00005555555eb9b8 in OSSL_CMP_CTX_set1_srvCert (ctx=0x55555599b860, 
    val=0x5555559b99b0) at ../master/crypto/cmp/cmp_ctx.c:649
#12 0x00005555555bc370 in execute_CTX_set1_get0_srvCert (
    fixture=0x5555559e4430) at ../master/test/cmp_ctx_test.c:736
#13 0x00005555555bc59a in test_CTX_set1_get0_srvCert ()
    at ../master/test/cmp_ctx_test.c:736

It shows why part of the certificate get/set test failed, namely only those calling OSSL_CMP_CTX_set1_*():
They call ossl_x509v3_cache_extensions() for checking the validity of the cert, which among others has the side effect of hashing the cert contents, which now fails for a incomplete ASN.1 structures, as you wrote above.

test/cmp_ctx_test.c

t8m

Approving the base change done by Richard.

DDvO

Approving the additional changes done by Tomáš.

levitte · 2021-07-09T15:41:12Z

@t8m, many thanks for fixing the test

openssl-machine · 2021-07-10T15:00:09Z

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

…tent The test case creates an RSA public key and tries to pass it through i2d_PrivateKey(). This SHOULD fail, since the private bits are missing. Reviewed-by: Tomas Mraz <[email protected]> (Merged from #16036)

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

ASN1_FBOOLEAN is designed to use as a default for optional ASN1 items. This test program used it for non-optional items, which doesn't encode well. Reviewed-by: Tomas Mraz <[email protected]> (Merged from #16036)

Make it only report (and fail on) encoding/decoding failures when success is expected. Reviewed-by: Tomas Mraz <[email protected]> (Merged from #16036)

Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: David von Oheimb <[email protected]> (Merged from #16036)

levitte · 2021-07-10T15:07:43Z

Merged

2296cc3 TEST: Check that i2d refuses to encode non-optional items with no content
4cd4735 ASN.1: Refuse to encode to DER if non-optional items are missing
f0f4de4 Fix test/asn1_encode_test.c to not use ASN1_FBOOLEAN
50d0a51 Fix test/asn1_encode_test.c to handle encoding/decoding failure
6bfd3e5 test_cmp_ctx: Avoid using empty X509 with i2d

DDvO · 2021-07-12T05:49:27Z

I suppose replacing all the X509_new() calls in the test with calls to X509_dup() of a real certificate should fix it?

The underlying issue is that X509_new() produces an invalid X.509 structure, not having the mandatory fields.
Indeed, copying a real cert would be a workaround.

As I wrote above, the quick solution taken here is just a workaround.
For the root cause and my proposal to solve it, see #16043.

davidben · 2021-07-16T16:44:39Z

crypto/asn1/tasn_enc.c

        i = ASN1_item_ex_i2d(pval, NULL, ASN1_ITEM_ptr(tt->item), -1, iclass);
        if (!i)
            return 0;
+        if (i == 0 && (tt->flags & ASN1_TFLG_OPTIONAL) == 0) {


I believe this block is unreachable because the line immediately after it would have returned zero first. It seems:

They should be swapped.

This call is missing a check for negative values.

levitte added branch: master Applies to master branch approval: review pending This pull request needs review by a committer labels Jul 9, 2021

levitte added this to the 3.0.0 milestone Jul 9, 2021

levitte added 4 commits July 9, 2021 09:08

TEST: Check that i2d refuses to encode non-optional items with no con…

06548e7

…tent The test case creates an RSA public key and tries to pass it through i2d_PrivateKey(). This SHOULD fail, since the private bits are missing.

ASN.1: Refuse to encode to DER if non-optional items are missing

d55443f

Fixes openssl#16026

Fix test/asn1_encode_test.c to not use ASN1_FBOOLEAN

d1020f7

ASN1_FBOOLEAN is designed to use as a default for optional ASN1 items. This test program used it for non-optional items, which doesn't encode well.

Fix test/asn1_encode_test.c to handle encoding/decoding failure

ae01f3b

Make it only report (and fail on) encoding/decoding failures when success is expected.

levitte force-pushed the fix-16026 branch from 0a5db33 to ae01f3b Compare July 9, 2021 07:08

levitte mentioned this pull request Jul 9, 2021

ASN.1: Refuse to encode to DER if non-optional items are missing [1.1.1] #16027

Closed

test_cmp_ctx: Avoid using empty X509 with i2d

d2e0e3f

DDvO reviewed Jul 9, 2021

View reviewed changes

test/cmp_ctx_test.c Show resolved Hide resolved

t8m approved these changes Jul 9, 2021

View reviewed changes

fixup! test_cmp_ctx: Avoid using empty X509 with i2d

5ea2ced

DDvO approved these changes Jul 9, 2021

View reviewed changes

DDvO added the approval: done This pull request has the required number of approvals label Jul 9, 2021

openssl-machine pushed a commit that referenced this pull request Jul 10, 2021

ASN.1: Refuse to encode to DER if non-optional items are missing

4cd4735

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

openssl-machine pushed a commit that referenced this pull request Jul 10, 2021

test_cmp_ctx: Avoid using empty X509 with i2d

6bfd3e5

Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: David von Oheimb <[email protected]> (Merged from #16036)

levitte closed this Jul 10, 2021

levitte deleted the fix-16026 branch July 10, 2021 15:07

DDvO mentioned this pull request Jul 11, 2021

Fix ossl_x509v3_cache_extensions(): EXFLAG_NO_FINGERPRINT is not an error #16043

Closed

1 task

davidben reviewed Jul 16, 2021

View reviewed changes

gotthardp mentioned this pull request Aug 14, 2021

generate_partial_X509 fails as i2d_X509_bio cannot produce invalid data in OSSL >=3.0 tpm2-software/tpm2-tools#2813

Closed

Uh oh!

Conversation

levitte commented Jul 9, 2021

Uh oh!

levitte commented Jul 9, 2021

Uh oh!

t8m commented Jul 9, 2021

Uh oh!

levitte commented Jul 9, 2021

Uh oh!

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

Uh oh!

Uh oh!

levitte commented Jul 9, 2021

Uh oh!

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

Uh oh!

Uh oh!

t8m commented Jul 9, 2021

Uh oh!

DDvO commented Jul 9, 2021

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

t8m left a comment

Choose a reason for hiding this comment

Uh oh!

DDvO left a comment • 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!

levitte commented Jul 9, 2021

Uh oh!

openssl-machine commented Jul 10, 2021

Uh oh!

levitte commented Jul 10, 2021

Uh oh!

DDvO commented Jul 12, 2021

Uh oh!

davidben Jul 16, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

levitte commented Jul 9, 2021 •

edited

Loading

levitte commented Jul 9, 2021 •

edited

Loading

DDvO commented Jul 9, 2021 •

edited

Loading

DDvO left a comment •

edited

Loading