Memory leak in i2d_ASN1_bio_stream when using SMIME_write_CMS#14844
Memory leak in i2d_ASN1_bio_stream when using SMIME_write_CMS#14844Basskrapfen wants to merge 5 commits intoopenssl:masterfrom
Conversation
|
This PR needs to raised against master not 1_1_1.. |
crypto/asn1/bio_asn1.c
Outdated
There was a problem hiding this comment.
This looks like a double free here for the normal case?
ctx->prefix_free is not set to NULL after the cleanup().
There was a problem hiding this comment.
The implementation ndef_prefix_free() seems to be safe to be called multiple times. It is freeing "ndef_aux->derbuf" and setting the pointer to NULL afterwards. Calling OPENSSL_free() with NULL should be safe as far as I know.
Alternative would be to zero the free function ptr after it was called. It looks like the state machine cannot be reset to the beginning.
What do you suggest?
crypto/asn1/bio_asn1.c
Outdated
There was a problem hiding this comment.
Coding style (same for suffix)
if (ctx->prefix_free != NULL)
ctx->prefix_free(b, &ctx->ex_buf, &ctx->ex_len, &ctx->ex_arg);
There was a problem hiding this comment.
I would really like to see a test for this..
Especially if ctx->ex_buf, ctx->ex_len, ctx->ex_arg are being shared between the prefix and suffix.
There was a problem hiding this comment.
@slontis about the test, can I just extend the cms app with a flag to fail reading from the input?
There was a problem hiding this comment.
A low level test added to bio_memleak_test .c would be sufficient..
There was a problem hiding this comment.
If you already had some data you should be able to debug into it to see what needs to be set up for a simpler test.
There was a problem hiding this comment.
Ok, I've added a small unit test which checks for the case. If you comment the "ctx->prefix_free" calls which the patch added, you can verify the memory leak.
|
IMO this does not qualify for CLA: trivial. |
|
Is the approach to fix the issue OK in general? If yes I can continue to improve the patch. |
|
It looks ok to me. |
Basskrapfen
force-pushed
the
mem_leak_i2d_ASN1_bio_stream
branch
from
e945bd0 to
397a65a
Compare
|
Please remove the |
Thanks for the hint, will do so. |
|
There is still the CLA issue. Could you please sign a CLA and remove the CLA: trivial annotation from the first commit? |
…ack in asn1_bio_free().
Basskrapfen
force-pushed
the
mem_leak_i2d_ASN1_bio_stream
branch
from
e496374 to
81500bf
Compare
t8m
added
branch: 1.1.1
t8m
added
approval: review pending
github-actions
bot
added
the
severity: fips change
github-actions
bot
removed
the
severity: fips change
paulidale
added
approval: done
openssl-machine
added
approval: ready to merge
|
This pull request is ready to merge |
When creating a signed S/MIME message using SMIME_write_CMS() if the reading from the bio fails, the state is therefore still ASN1_STATE_START when BIO_flush() is called by i2d_ASN1_bio_stream(). This results in calling asn1_bio_flush_ex cleanup but will only reset retry flags as the state is not ASN1_STATE_POST_COPY. Therefore 48 bytes (Linux x86_64) leaked since the ndef_prefix_free / ndef_suffix_free callbacks are not executed and the ndef_aux structure is not freed. By always calling free function callback in asn1_bio_free() the memory leak is fixed. Reviewed-by: Paul Dale <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #14844)
|
I've squashed the commits and used the description from this PR to improve the commit message. Merged. Thank you for the contribution. |
|
As this does not cherry-pick cleanly, would you like to create a PR cherry picking the commit for 1.1.1, or should we do that? |
I can try cherry picking it for the 1.1.1 branch. Thanks a lot again for the support! |
When creating a signed S/MIME message using SMIME_write_CMS() if the reading from the bio fails, the state is therefore still ASN1_STATE_START when BIO_flush() is called by i2d_ASN1_bio_stream(). This results in calling asn1_bio_flush_ex cleanup but will only reset retry flags as the state is not ASN1_STATE_POST_COPY. Therefore 48 bytes (Linux x86_64) leaked since the ndef_prefix_free / ndef_suffix_free callbacks are not executed and the ndef_aux structure is not freed. By always calling free function callback in asn1_bio_free() the memory leak is fixed. Reviewed-by: Paul Dale <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#14844)
6 participants
When creating a signed S/MIME message using SMIME_write_CMS(), the state machine in the background does not handle error cases completely. This results in a small memory leak in i2d_ASN1_bio_stream / asn_mime.c
In BIO_new_NDEF, ndef_aux is initialized and set to the bio stream via
ndef_aux = OPENSSL_zalloc(sizeof(*ndef_aux));to bio->ex_arg
This memory is normally freed in asn1_bio_flush_ex (bio_asn1.c, cleanup parameter set to true).
This call is triggered via BIO_flush() which ends up in the asn1_bio_ctrl (bio_asn1.c) in the case the cmd is BIO_CTRL_FLUSH and state is ASN1_STATE_POST_COPY.
The state is also updated in the same function and asn1_bio_write.
In our use case the reading from the bio fails, the state is therefore still ASN1_STATE_START when BIO_flush() is called by i2d_ASN1_bio_stream. This results in calling asn1_bio_flush_ex cleanup but will only reset retry flags as the state is not ASN1_STATE_POST_COPY. Therefore 48 bytes (Linux x86_64) leaked since the ndef_prefix_free / ndef_suffix_free callbacks are not executed and the ndef_aux structure is not freed.
Patch is attached which makes sure the free callbacks are called. Another option would be to improve the state machine but this looks harder to get it right.
CLA: trivial