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.

Sign up for GitHub

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

Update X509 fuzzer to verify a chain #20243

Closed
wants to merge 1 commit into from
Closed

Update X509 fuzzer to verify a chain #20243

wants to merge 1 commit into from

Conversation

kroeckx
Copy link
Member

@kroeckx kroeckx commented Feb 8, 2023

It add supports for verifying that it's been signed by a CA, and checks the CRL and OCSP status

Can find CVE-2022-4203 and CVE-2023-0286

@kroeckx kroeckx added branch: master Merge to master branch branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 labels Feb 8, 2023
@t8m t8m added triaged: bug The issue/pr is/fixes a bug tests: present The PR has suitable tests present labels Feb 8, 2023
@t8m
Copy link
Member

t8m commented Feb 8, 2023

The CI is relevant.

@kroeckx
Copy link
Member Author

kroeckx commented Feb 8, 2023

So it still fails with:

../../util/../util/shlib_wrap.sh: 113: exec: ../../fuzz/x509-test: not found
../../util/wrap.pl ../../fuzz/x509-test ../../fuzz/corpora/x509 => 127

Not sure what's wrong.

t8m
t8m reviewed Feb 8, 2023
View reviewed changes
test/recipes/99-test_fuzz_x509.t Outdated
@@ -15,6 +15,9 @@ use OpenSSL::Test::Utils;
my $fuzzer = "x509";
setup("test_fuzz_${fuzzer}");

plan skip_all => "This test requires $fuzzer support"
if disabled($fuzzer);
Copy link
Member

Choose a reason for hiding this comment

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

should you check disabled("ocsp") instead?

@kroeckx
Copy link
Member Author

kroeckx commented Feb 8, 2023

Why aren't all the CI jobs running?

bernd-edlinger
bernd-edlinger reviewed Feb 9, 2023
View reviewed changes
test/recipes/99-test_fuzz_x509.t
@@ -15,6 +15,9 @@ use OpenSSL::Test::Utils;
my $fuzzer = "x509";
setup("test_fuzz_${fuzzer}");

plan skip_all => "This test requires $fuzzer support"
if disabled("ocsp");
Copy link
Member

Choose a reason for hiding this comment

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

is there no way how the test could be compiled with #ifndef OPENSSL_NO_OCSP?

Copy link
Member Author

Choose a reason for hiding this comment

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

In theory, yes. Since it's the last part of the file, it will currently then ignore part of the file it reads, and just not cover as much. If at some later point, the test is extended to read more data, the files will not have proper coverage.

@kroeckx kroeckx mentioned this pull request Feb 12, 2023
Closed
@kroeckx kroeckx added the approval: review pending This pull request needs review by a committer label Mar 21, 2023
@openssl-machine
Copy link
Collaborator

This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago

@t8m
Copy link
Member

t8m commented May 15, 2023

@kroeckx could you please rebase this?

@openssl-machine
Copy link
Collaborator

This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago

@kroeckx
Update X509 fuzzer to verify a chain
0c36791 
It add supports for verifying that it's been signed by a CA, and
checks the CRL and OCSP status

Can find CVE-2022-4203 and CVE-2023-0286
@openssl-machine
Copy link
Collaborator

This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago

@kroeckx
Copy link
Member Author

kroeckx commented Aug 27, 2023

Can someone please review this? This has been waiting for review for 8 months now has, significantly increases our fuzz coverage and can detect 2 more CVEs.

beldmit
beldmit approved these changes Aug 27, 2023
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 Aug 27, 2023
@beldmit
Copy link
Member

beldmit commented Aug 27, 2023

Not sure about the 1.1.1 branch but LGTM

@kroeckx kroeckx mentioned this pull request Aug 28, 2023
Closed
@openssl-machine
Copy link
Collaborator

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@DDvO
Copy link
Contributor

DDvO commented Aug 29, 2023

Looks like this is ready to merge.

@kroeckx kroeckx 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 Aug 31, 2023
openssl-machine pushed a commit that referenced this pull request Sep 1, 2023
@kroeckx @hlandau
Update X509 fuzzer to verify a chain
399c2da 
It add supports for verifying that it's been signed by a CA, and
checks the CRL and OCSP status

Can find CVE-2022-4203 and CVE-2023-0286

Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from #20243)
openssl-machine pushed a commit that referenced this pull request Sep 1, 2023
@kroeckx @hlandau
Update X509 fuzzer to verify a chain
9e4418f 
It add supports for verifying that it's been signed by a CA, and
checks the CRL and OCSP status

Can find CVE-2022-4203 and CVE-2023-0286

Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from #20243)

(cherry picked from commit 399c2da)
(cherry picked from commit 869d95b)
openssl-machine pushed a commit that referenced this pull request Sep 1, 2023
@kroeckx @hlandau
Update X509 fuzzer to verify a chain
869d95b 
It add supports for verifying that it's been signed by a CA, and
checks the CRL and OCSP status

Can find CVE-2022-4203 and CVE-2023-0286

Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from #20243)

(cherry picked from commit 399c2da)
@hlandau
Copy link
Member

hlandau commented Sep 1, 2023

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

A trivial build.info file conflict was fixed during merge to 3.1 and 3.0.

The changes needed to rebase this on 1.1.1 look slightly more substantial; would you like to open another PR?

@hlandau hlandau closed this Sep 1, 2023
@hlandau hlandau reopened this Sep 1, 2023
@hlandau hlandau removed the approval: ready to merge The 24 hour grace period has passed, ready to merge label Sep 1, 2023
@bernd-edlinger
Copy link
Member

no please leave 1.1.1 as is it is EOL next week anyway.

@bernd-edlinger bernd-edlinger mentioned this pull request Sep 1, 2023
Closed
@t8m t8m removed the branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch label Sep 1, 2023
@t8m t8m closed this Sep 1, 2023
xl32 pushed a commit to xl32/openssl that referenced this pull request Sep 29, 2023
@kroeckx @xl32
Update X509 fuzzer to verify a chain
d6c77a3 
It add supports for verifying that it's been signed by a CA, and
checks the CRL and OCSP status

Can find CVE-2022-4203 and CVE-2023-0286

Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from openssl#20243)

(cherry picked from commit 399c2da)
(cherry picked from commit 869d95b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bernd-edlinger bernd-edlinger bernd-edlinger left review comments

@t8m t8m t8m approved these changes

@beldmit beldmit beldmit approved these changes

Assignees
No one assigned
Labels
branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 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.

None yet
7 participants
@kroeckx @t8m @openssl-machine @beldmit @DDvO @hlandau @bernd-edlinger