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

PKITS 4.10.7 and 4.10.8 hang on Windows in OpenSSL 3.0 #19643

Closed
brucestephens opened this issue Nov 10, 2022 · 8 comments
Closed

PKITS 4.10.7 and 4.10.8 hang on Windows in OpenSSL 3.0 #19643

brucestephens opened this issue Nov 10, 2022 · 8 comments
Labels
branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 help wanted triaged: bug The issue/pr is/fixes a bug

Comments

@brucestephens
Copy link

Presuming TA.pem is the trust anchor from PKITS, the following works fine on GNU/Linux but hangs on Windows:

% openssl cms -verify -CAfile TA.pem -policy anyPolicy -in SignedInvalidMappingFromanyPolicyTest7.eml
CMS Verification failure
0046881801000000:error:17000064:CMS routines:cms_signerinfo_verify_cert:certificate verify error:../crypto/cms/cms_smime.c:289:Verify error: invalid or inconsistent certificate policy extension

Similarly for the next test.

git bisect suggests the problem is 9aa4be6 so I presume the difference is something to do with the way locking works on Windows compared to pthreads.

@brucestephens brucestephens added the issue: bug report The issue was opened to report a bug label Nov 10, 2022
@mattcaswell
Copy link
Member

The issue is that there is an attempt to obtain a lock which is already held. The function in question is ossl_policy_cache_set_mapping which is only ever called by policy_cache_new which is only ever called by ossl_policy_cache_set.

ossl_policy_cache_set obtains the X509 lock before policy_cache_new is called and releases it afterwards. ossl_policy_cache_set_mapping then additionally tries to get the lock again.

Using pthreads the second attempt to obtain the lock simply fails and we skip the line that sets the EXFLAG_INVALID_POLICY flag. But that doesn't matter because policy_cache_new sets it anyway when ossl_policy_cache_set_mapping returns -1.

Under Windows it seems that attempting to obtain the lock a second time hangs.

This is all just wrong and the change from 9aa4be6 is just wrong. In fact the whole setting of the EXFLAG_INVALID_POLICY flag can be removed from that function completely because policy_cache_new will do it anyway.

The fix itself is easy - but setting "help wanted" because a test should be constructed for this.

@mattcaswell mattcaswell added help wanted triaged: bug The issue/pr is/fixes a bug and removed issue: bug report The issue was opened to report a bug labels Nov 10, 2022
@t8m t8m added branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 labels Nov 10, 2022
paulidale added a commit to paulidale/openssl that referenced this issue Nov 10, 2022
@kroeckx
Copy link
Member

kroeckx commented Nov 10, 2022

It seems that in the case of USE_RWLOCK, we don't set PTHREAD_MUTEX_NORMAL or PTHREAD_MUTEX_ERRORCHECK. We probably should.

@t8m
Copy link
Member

t8m commented Nov 11, 2022

It seems that in the case of USE_RWLOCK, we don't set PTHREAD_MUTEX_NORMAL or PTHREAD_MUTEX_ERRORCHECK. We probably should.

Hmm, perhaps only in debug builds?

@kroeckx
Copy link
Member

kroeckx commented Nov 11, 2022 via email

@kroeckx
Copy link
Member

kroeckx commented Nov 12, 2022 via email

paulidale added a commit to paulidale/openssl that referenced this issue Nov 13, 2022
paulidale added a commit to paulidale/openssl that referenced this issue Nov 14, 2022
This reverts commit 9aa4be6 and removed the
redundant flag setting.

Fixes openssl#19643
paulidale added a commit to paulidale/openssl that referenced this issue Nov 14, 2022
@bernd-edlinger
Copy link
Member

Are you saying we lock the write-lock while already holding the read-lock ?

@paulidale
Copy link
Contributor

It's a wrtie lock while holding a write lock.

paulidale added a commit to paulidale/openssl that referenced this issue Nov 30, 2022
This reverts commit 9aa4be6 and removed the
redundant flag setting.

Fixes openssl#19643

Fixes LOW CVE-2022-3996
paulidale added a commit to paulidale/openssl that referenced this issue Nov 30, 2022
paulidale added a commit to paulidale/openssl that referenced this issue Dec 4, 2022
This reverts commit 9aa4be6 and removed the
redundant flag setting.

Fixes openssl#19643

Fixes LOW CVE-2022-3996
paulidale added a commit to paulidale/openssl that referenced this issue Dec 4, 2022
paulidale added a commit to paulidale/openssl that referenced this issue Dec 4, 2022
paulidale added a commit to paulidale/openssl that referenced this issue Dec 6, 2022
openssl-machine pushed a commit that referenced this issue Dec 8, 2022
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #19652)
openssl-machine pushed a commit that referenced this issue Dec 8, 2022
This reverts commit 9aa4be6 and removed the
redundant flag setting.

Fixes #19643

Fixes LOW CVE-2022-3996

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #19652)

(cherry picked from commit 4d0340a)
openssl-machine pushed a commit that referenced this issue Dec 8, 2022
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #19652)

(cherry picked from commit 61203c2)
openssl-machine pushed a commit that referenced this issue Dec 8, 2022
This reverts commit 9aa4be6 and removed the
redundant flag setting.

Fixes #19643

Fixes LOW CVE-2022-3996

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #19652)

(cherry picked from commit 4d0340a)
openssl-machine pushed a commit that referenced this issue Dec 8, 2022
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #19652)

(cherry picked from commit 61203c2)
beldmit pushed a commit to beldmit/openssl that referenced this issue Dec 26, 2022
This reverts commit 9aa4be6 and removed the
redundant flag setting.

Fixes openssl#19643

Fixes LOW CVE-2022-3996

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#19652)
beldmit pushed a commit to beldmit/openssl that referenced this issue Dec 26, 2022
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#19652)
@anton-kazlouski
Copy link

Is there any ETA for releasing this fix?

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.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 help wanted triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants