Skip to content

Fix hang on Windows in x509 policy code#19652

Closed
paulidale wants to merge 4 commits intoopenssl:masterfrom
paulidale:fix-19643
Closed

Fix hang on Windows in x509 policy code#19652
paulidale wants to merge 4 commits intoopenssl:masterfrom
paulidale:fix-19643

Conversation

@paulidale
Copy link
Contributor

@paulidale paulidale commented Nov 10, 2022

This reverts commit 9aa4be6.

Fixes #19643

@paulidale paulidale added branch: master Applies to master branch approval: review pending This pull request needs review by a committer approval: otc review pending branch: 3.0 Applies to openssl-3.0 branch branch: 3.1 Applies to openssl-3.1 (EOL) labels Nov 10, 2022
@paulidale paulidale self-assigned this Nov 10, 2022
@kroeckx kroeckx added the hold: needs tests The PR needs tests to be added to it label Nov 10, 2022
@t8m
Copy link
Member

t8m commented Nov 11, 2022

A test will probably work only in tsan or on Windows. But yeah, it would be useful.

@t8m t8m added the triaged: bug The issue/pr is/fixes a bug label Nov 11, 2022
mattcaswell
mattcaswell reviewed Nov 11, 2022
View reviewed changes
@paulidale
Copy link
Contributor Author

paulidale commented Nov 13, 2022

Testcase added.

I suspect getting the test/pkits-test.pl script running as a unit test would be better. I've raised this as #19663.

@paulidale paulidale added tests: present The PR has suitable tests present and removed hold: needs tests The PR needs tests to be added to it labels Nov 14, 2022
t8m
t8m previously approved these changes Nov 14, 2022
View reviewed changes
Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please amend the first commit message because the commit is no longer just revert but it completely drops the unnecessary ex_flags setting.

@mattcaswell
Copy link
Member

mattcaswell commented Nov 14, 2022

Are there licence and/or CLA issues with just including the PKITS test data in our codebase?

@paulidale
Copy link
Contributor Author

paulidale commented Nov 14, 2022

They come from NIST which generally mean fair game to use. We've a lot of other NIST test cases already.

@mattcaswell
Copy link
Member

mattcaswell commented Nov 14, 2022

They come from NIST which generally mean fair game to use.

We should check this with OMC.

t8m
t8m reviewed Nov 28, 2022
View reviewed changes
@mattcaswell
Copy link
Member

mattcaswell commented Nov 30, 2022

OMC: Inclusion of this test data is ok to proceed.

@t-j-h
Copy link
Member

t-j-h commented Nov 30, 2022

FYI https://csrc.nist.gov/projects/pki-testing is the web page from which the test data is located.

@t8m t8m dismissed their stale review November 30, 2022 08:38

Proposing moving the test to test_cms recipe

@t8m t8m removed the approval: review pending This pull request needs review by a committer label Nov 30, 2022
@t8m
Copy link
Member

t8m commented Nov 30, 2022

@paulidale could you please move the testcase to test_cms recipe? It does not look to me it should be in test_threads as it does not really spawn any threads.

@paulidale
Copy link
Contributor Author

paulidale commented Dec 4, 2022

Relocated.

t8m
t8m reviewed Dec 5, 2022
View reviewed changes
@t8m t8m added the approval: review pending This pull request needs review by a committer label Dec 7, 2022
@t8m
Copy link
Member

t8m commented Dec 7, 2022

ping for second review

beldmit
beldmit approved these changes Dec 7, 2022
View reviewed changes
Copy link
Member

@beldmit beldmit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@beldmit beldmit added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Dec 7, 2022
FdaSilvaYY
FdaSilvaYY reviewed Dec 7, 2022
View reviewed changes
crypto/x509/pcy_map.c
ret = 1;
bad_mapping:
if (ret == -1 && CRYPTO_THREAD_write_lock(x->lock)) {
x->ex_flags |= EXFLAG_INVALID_POLICY;
Copy link
Contributor

@FdaSilvaYY FdaSilvaYY Dec 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line of code was presented in original code version, no ?
The version before the reverted commit ?

Copy link
Member

@t8m t8m Dec 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but it is useless. The caller will set the flag instead.

@openssl-machine openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Dec 8, 2022
@openssl-machine
Copy link
Collaborator

openssl-machine commented Dec 8, 2022

This pull request is ready to merge

openssl-machine pushed a commit that referenced this pull request Dec 8, 2022
@paulidale @t8m
x509: fix double locking problem
4d0340a 
This reverts commit 9aa4be6 and removed the
redundant flag setting.

Fixes #19643

Fixes LOW CVE-2022-3996

Reviewed-by: Dmitry Belyavskiy <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #19652)
openssl-machine pushed a commit that referenced this pull request Dec 8, 2022
@paulidale @t8m
test: add test case for deadlock reported in #19643
61203c2 
Reviewed-by: Dmitry Belyavskiy <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #19652)
openssl-machine pushed a commit that referenced this pull request Dec 8, 2022
@paulidale @t8m
x509: fix double locking problem
1adc6db 
This reverts commit 9aa4be6 and removed the
redundant flag setting.

Fixes #19643

Fixes LOW CVE-2022-3996

Reviewed-by: Dmitry Belyavskiy <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #19652)

(cherry picked from commit 4d0340a)
openssl-machine pushed a commit that referenced this pull request Dec 8, 2022
@paulidale @t8m
test: add test case for deadlock reported in #19643
d2cdcb6 
Reviewed-by: Dmitry Belyavskiy <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #19652)

(cherry picked from commit 61203c2)
@t8m
Copy link
Member

t8m commented Dec 8, 2022

Merged to master, 3.1, and 3.0 branches. Thank you.

@t8m t8m closed this Dec 8, 2022
openssl-machine pushed a commit that referenced this pull request Dec 8, 2022
@paulidale @t8m
x509: fix double locking problem
7725e7b 
This reverts commit 9aa4be6 and removed the
redundant flag setting.

Fixes #19643

Fixes LOW CVE-2022-3996

Reviewed-by: Dmitry Belyavskiy <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #19652)

(cherry picked from commit 4d0340a)
openssl-machine pushed a commit that referenced this pull request Dec 8, 2022
@paulidale @t8m
test: add test case for deadlock reported in #19643
6fbbb53 
Reviewed-by: Dmitry Belyavskiy <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #19652)

(cherry picked from commit 61203c2)
@paulidale paulidale deleted the fix-19643 branch December 8, 2022 22:25
@mattcaswell mattcaswell mentioned this pull request Dec 15, 2022
Closed
beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022
@paulidale @beldmit
x509: fix double locking problem
0c90252 
This reverts commit 9aa4be6 and removed the
redundant flag setting.

Fixes openssl#19643

Fixes LOW CVE-2022-3996

Reviewed-by: Dmitry Belyavskiy <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#19652)
beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022
@paulidale @beldmit
test: add test case for deadlock reported in openssl#19643
2f2cca9 
Reviewed-by: Dmitry Belyavskiy <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#19652)
raspbian-autopush pushed a commit to raspbian-packages/openssl that referenced this pull request Jan 22, 2023
@paulidale @sebastianas
x509: fix double locking problem
a3f1d60 
This reverts commit 9aa4be691f5c73eb3c68606d824c104550c053f7 and removed the
redundant flag setting.

Fixes #19643

Fixes LOW CVE-2022-3996

Reviewed-by: Dmitry Belyavskiy <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl/openssl#19652)

(cherry picked from commit 4d0340a6d2f327700a059f0b8f954d6160f8eef5)

Gbp-Pq: Name x509-fix-double-locking-problem.patch
laiyoufafa pushed a commit to laiyoufafa/third_party_openssl that referenced this pull request Apr 28, 2023
@paulidale
x509: fix double locking problem
5170a5b 
This reverts commit 9aa4be691f5c73eb3c68606d824c104550c053f7 and removed the
redundant flag setting.

Fixes #19643

Fixes LOW CVE-2022-3996

Reviewed-by: Dmitry Belyavskiy <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl/openssl#19652)

(cherry picked from commit 4d0340a6d2f327700a059f0b8f954d6160f8eef5)
Signed-off-by: code4lala <[email protected]>
Change-Id: I1013e00e8fe7c7975cf8e6dd4db380f37179660d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@t8m t8m t8m approved these changes

@mattcaswell mattcaswell mattcaswell left review comments

@beldmit beldmit beldmit approved these changes

+1 more reviewer

@FdaSilvaYY FdaSilvaYY FdaSilvaYY left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@paulidale paulidale

Labels

approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Applies to master branch branch: 3.0 Applies to openssl-3.0 branch branch: 3.1 Applies to openssl-3.1 (EOL) tests: present The PR has suitable tests present triaged: bug The issue/pr is/fixes a bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

PKITS 4.10.7 and 4.10.8 hang on Windows in OpenSSL 3.0

8 participants

@paulidale @t8m @mattcaswell @t-j-h @openssl-machine @beldmit @FdaSilvaYY @kroeckx