[REGRESSION] CMS_Final() fails when modifying CMS

[alonbl]

Root cause: #19919 "Fix SMIME_crlf_copy() to properly report an error"
master 9e5bd89
>=openssl-3.0.8 6259cf3

Previous behavior required CMS_final() to be called when CMS is modified (for example add recepient).
Currently, the CMS_final() fails with:

802BF10400000000:error:100C0102:BIO routines:bio_read_intern:passed a null parameter:crypto/bio/bio_lib.c:274:
802BF10400000000:error:1C80006B:Provider routines:ossl_cipher_generic_block_final:wrong final block length:providers/implementations/ciphers/ciphercommon.c:429:

Interestingly, if the CMS_final() is removed, the CMS is created correctly, however, it leaks memory.

Reproduction project is available here[1].

[1] https://github.com/alonbl/openssl-regression-cms

[t8m]

@DDvO might have an idea what is wrong here. If I remember correctly he was touching this code.

[alonbl]

Root cause: #19919

[alonbl]

@mattcaswell: can you please look into this issue?

[t8m]

I do not see how #19919 could cause this. Is this no longer reproducible if this PR is reverted?

[DDvO]

I tried out with master the reproduction project - great work @alonbl.

Indeed the issue disappears when going back one step in history from
commit 9e5bd89 (which is part of #19918) to commit e51dd6e.

Looks like commit 9e5bd89 just surfaced some earlier bug.

[DDvO]

Hmm, I went back in the commit history as far as commit 0195cdd of #16466
where ossl_rsa_to_EncryptedPrivateKeyInfo_der_encoder_functions needed by the reproduction project was introduced.

Even there the issue surfaces when cherry-picking commit 9e5bd89.
So either there is a pretty old bug or the reproduction project uses the API wrongly.

[DDvO]

BTW, a hint for others using the reproduction project:
I used git clone git@github.com:alonbl/openssl-regression-cms.git
in one of my OpenSSL development directories
and adapted the first two lines of openssl-regression-cms/Makefile to:

OPENSSL_CFLAGS = -I../include               # $(shell pkg-config --cflags libcrypto)
OPENSSL_LIBS = -lcrypto -L.. -Wl,-rpath=..  # $(shell pkg-config --libs libcrypto)

After doing any experimental changes to the local OpenSSL branch, call

make -C openssl-regression-cms check

[alonbl]

@DDvO : Thanks for checking this out, I will be glad to understand what's wrong with API usage, it works since openssl-1.1 up to now. The mission is adding recepient to existing CMS, in openssl-1.1 without the call to CMS_final() it fails, in openss-3 it seems like the CMS_final() is called implicitly but leaks, calling it explicitly resolves the issue.

BTW: You can just override the OPENSSL_CFLAGS and OPENSSL_LIBS from outside, this is why I added these vars :)

[DDvO]

@DDvO : Thanks for checking this out, I will be glad to understand what's wrong with API usage, it works since openssl-1.1 up to now.

I've just tested this with OpenSSL_1_1_1-stable and indeed it works there,
also after cherry-picking commit 9e5bd89 which pointed out the issue.
So I agree that the API use must be fine and this issue is really a regression.

BTW: You can just override the OPENSSL_CFLAGS and OPENSSL_LIBS from outside, this is why I added these vars :)

Good hint - so one can simply use

CFLAGS=-g OPENSSL_CFLAGS="-I../include" OPENSSL_LIBS="-lcrypto -lpthread -ldl -L.. -Wl,-rpath=.." make -C openssl-regression-cms check

[alonbl]

@DDvO : I would like to bring into your attention that when adding a new signer to existing CMS there is no need to call CMS_final(), this means that although in the past (1.1) the CMS_final() was required for adding recipient, it may been incorrect behavior and openssl-3 is behaving correctly only thing to solve is the memory leak of this use case.

[t8m]

Thanks @DDvO for looking into this so far.

@alonbl Can you please try to investigate what exactly is being leaked in that no CMS_final() call case? How does 1.1.1 behave when you do not call CMS_final()?

[DDvO]

I meanwhile found that CMS_final(CMS_ContentInfo *cms, BIO *data, BIO *dcont, unsigned int flags) should never have been called with data == NULL - this was wrong API use, which before commit 9e5bd89 went uncaught.

Actually independent of that, there is the following mem leak on the content encryption key in the cms structure:

==1056658== 512 bytes in 1 blocks are definitely lost in loss record 1 of 1
==1056658==    at 0x483877F: malloc (vg_replace_malloc.c:307)
==1056658==    by 0x4AADCFA: CRYPTO_malloc (mem.c:196)
==1056658==    by 0x499D0A4: cms_RecipientInfo_ktri_decrypt (cms_env.c:623)
==1056658==    by 0x499DFE7: CMS_RecipientInfo_decrypt (cms_env.c:1017)
==1056658==    by 0x49A9901: CMS_decrypt_set1_pkey_and_peer (cms_smime.c:747)
==1056658==    by 0x49A970E: CMS_decrypt_set1_pkey (cms_smime.c:700)
==1056658==    by 0x1095B3: main (cms-add.c:80)

where I found that the following would be a workaround that could be done just before CMS_ContentInfo_free(cms):

BIO_free_all(CMS_dataInit(cms, NULL));

[DDvO]

I found a fix, which is similar to what I did in #19981 #19222 - CMS_ContentInfo_free() is missing

CMS_EncryptedContentInfo *ec = ossl_cms_get0_env_enc_content(cms);
if (ec != NULL)
   OPENSSL_clear_free(ec->key, ec->keylen);

Will provide a PR shortly.

[alonbl]

Hi,

Thanks great!

I can confirm that removing the CMS_final() and replacing the i2d_CMS_bio() with i2d_CMS_bio_stream() and adding the BIO_free_all(CMS_dataInit(cms, NULL)) works in all branches.

Look at this branch: https://github.com/alonbl/openssl-regression-cms/tree/no-final

Alon

[alonbl]

Actually this works also with i2d_CMS_bio() now, I am sure that openssl-1.1 had an issue with writing a valid CMS without the CMS_final(), I will look into that.