Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

PKCS7_verify() unnecessarily creates temp indata BIO which ignores caller's indata BIO callbacks #16429

Open
bobm21 opened this issue Aug 26, 2021 · 0 comments · May be fixed by #16590
Open

PKCS7_verify() unnecessarily creates temp indata BIO which ignores caller's indata BIO callbacks #16429

bobm21 opened this issue Aug 26, 2021 · 0 comments · May be fixed by #16590
Labels
branch: master good first issue help wanted triaged: refactor
Milestone
Post 3.0.0

Comments

@bobm21
Copy link

@bobm21 bobm21 commented Aug 26, 2021

This PKCS7_verify() performance optimization from 2005 creates a new RO mem BIO from the caller's indata mem BIO, even if it's already a RO mem BIO in this code block:

openssl/crypto/pkcs7/pk7_smime.c

Lines 301 to 318 in 69b920b

/*
* Performance optimization: if the content is a memory BIO then store
* its contents in a temporary read only memory BIO. This avoids
* potentially large numbers of slow copies of data which will occur when
* reading from a read write memory BIO when signatures are calculated.
*/
if (indata && (BIO_method_type(indata) == BIO_TYPE_MEM)) {
char *ptr;
long len;
len = BIO_get_mem_data(indata, &ptr);
tmpin = (len == 0) ? indata : BIO_new_mem_buf(ptr, len);
if (tmpin == NULL) {
ERR_raise(ERR_LIB_PKCS7, ERR_R_MALLOC_FAILURE);
goto err;
}
} else
tmpin = indata;

I have a use case for a callback on a RO mem indata BIO passed to PKCS7_verify() and simply added a test for the indata BIO already being RO to the if statement to avoid the creation of the new tmpin BIO in OpenSSL 1.1.1j so that my callback remained in effect:

if (indata && (BIO_method_type(indata) == BIO_TYPE_MEM) &&
    !BIO_test_flags(indata, BIO_FLAGS_MEM_RDONLY)) {

However, after taking a closer look, the performance issue with reading RW mem BIOs that existed at the time of this optimization no longer exists, so all the logic related to the tmpin RO mem BIO should be deleted and the caller's indata BIO used as-is.
@bobm21 bobm21 added the issue: bug report label Aug 26, 2021
@paulidale paulidale added triaged: feature and removed issue: bug report labels Aug 26, 2021
@paulidale paulidale added this to the Post 3.0.0 milestone Aug 26, 2021
@t8m t8m added branch: master triaged: cleanup triaged: refactor good first issue and removed triaged: feature triaged: cleanup labels Aug 26, 2021
@nyx191 nyx191 linked a pull request Sep 13, 2021 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
branch: master good first issue help wanted triaged: refactor
Projects
None yet
Milestone
Post 3.0.0
Development

Successfully merging a pull request may close this issue.

Revert unnecessary PKCS7_verify() performance optimization nyx191/openssl
3 participants
@t8m @paulidale @bobm21