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

Regression from refactor of X509_load_cert_file_ex in 3.2.0 #22895

Closed
AlexanderOMara opened this issue Nov 30, 2023 · 0 comments
Closed

Regression from refactor of X509_load_cert_file_ex in 3.2.0 #22895

AlexanderOMara opened this issue Nov 30, 2023 · 0 comments
Labels
branch: master Merge to master branch branch: 3.2 Merge to openssl-3.2 severity: regression The issue/pr is a regression from previous released version triaged: bug The issue/pr is/fixes a bug

Comments

@AlexanderOMara
Copy link

AlexanderOMara commented Nov 30, 2023

I've found what I believe to be a regression caused by the refactoring of X509_load_cert_file_ex in commit ae29622 (from pull request #21545). Specifically, the removal of these 2 lines:

            X509_free(x);
            x = NULL;

With an upgrade to 3.2.0 (which I first noticed when Homebrew's updated to the latest version) osslsigncode verify some-signed-pe-file.exe started to fail for what should be valid signed binaries (like Firefox Setup 115.5.0esr.exe).

Downstream bug reports:

Error message seen with openssl 3.2.0, but not 3.1.4:

CMS_verify error
C0CFAEB0827F0000:error:17000064:CMS routines:cms_signerinfo_verify_cert:certificate verify error:crypto/cms/cms_smime.c:289:Verify error: self-signed certificate in certificate chain
Timestamp Server Signature verification: failed

PKCS7_verify error
C0CFAEB0827F0000:error:10800075:PKCS7 routines:PKCS7_verify:certificate verify error:crypto/pkcs7/pk7_smime.c:296:Verify error: self-signed certificate in certificate chain
Signature verification: failed

The source of the regression was not at-all obvious, but through a binary search testing 3.2.0's commit history I found the commit where it broke and the minimal amount of changes from that commit needed to reproduce the issue. Adding those lines back into 3.2.0 also restored the prior behavior.

I'm not an expert on openssl's internals and can't say exactly why this subtle logic change makes openssl behave differently than it used to, but I'm guessing that the X509 pointer is not reusable between loop iterations.

P.S. I don't know what the full impact radius of this logic change is, but worry it may be substantial.

@AlexanderOMara AlexanderOMara added the issue: bug report The issue was opened to report a bug label Nov 30, 2023
@t8m t8m added branch: master Merge to master branch triaged: bug The issue/pr is/fixes a bug severity: regression The issue/pr is a regression from previous released version branch: 3.2 Merge to openssl-3.2 and removed issue: bug report The issue was opened to report a bug labels Dec 1, 2023
openssl-machine pushed a commit that referenced this issue Dec 4, 2023
…_file_ex()

Fixes #22895

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #22885)

(cherry picked from commit 20c680d)
wbeck10 pushed a commit to wbeck10/openssl that referenced this issue Jan 8, 2024
…_file_ex()

Fixes openssl#22895

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#22885)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch: master Merge to master branch branch: 3.2 Merge to openssl-3.2 severity: regression The issue/pr is a regression from previous released version triaged: bug The issue/pr is/fixes a bug
Projects
None yet
2 participants