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

Added a fuzzer for SMIME #20332

Closed
wants to merge 1 commit into from
Closed

Added a fuzzer for SMIME #20332

wants to merge 1 commit into from

Conversation

alex
Copy link
Contributor

@alex alex commented Feb 19, 2023

No description provided.

@alex alex force-pushed the smime-fuzzer branch 2 times, most recently from 40fd1e3 to 93cd250 Compare February 19, 2023 05:03
@kroeckx
Copy link
Member

kroeckx commented Feb 19, 2023

Can you also make it run during make test, see for instance #20269 on how to do it.


if (p7) {
PKCS7_free(p7);
}
Copy link
Member

Choose a reason for hiding this comment

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

Can you do more things with the PKCS7? For instance call functions like PKCS7_get_signer_info(), PKCS7_cert_from_signer_info(), PKCS7_get_signed_attribute(), PKCS7_get_smimecap(), PKCS7_dataVerify(), ...

Copy link
Member

Choose a reason for hiding this comment

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

I guess the CMS fuzzer could use things like that too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some usage of the p7 API. I'm candidly not totally confident they're correct, since many of these functions aren't documented.

Copy link
Member

Choose a reason for hiding this comment

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

You might want to look at the pkcs7 and smime tool for other things that might be useful to do.

@alex alex force-pushed the smime-fuzzer branch 3 times, most recently from 0e0ec23 to d363237 Compare February 19, 2023 13:07
@alex
Copy link
Contributor Author

alex commented Feb 24, 2023

@kroeckx do you want to hold this open until its expanded, or would you consider merging as is (modulo any other reviews) and then iterating as we get coverage metrics from oss-fuzz?

@kroeckx
Copy link
Member

kroeckx commented Feb 24, 2023 via email

fuzz/smime.c Outdated Show resolved Hide resolved
@alex alex force-pushed the smime-fuzzer branch 2 times, most recently from 4d34b79 to 31c4abc Compare February 24, 2023 11:51
Copy link
Member

@mattcaswell mattcaswell left a comment

Choose a reason for hiding this comment

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

Looks good. Just some style nits below.

fuzz/smime.c Outdated Show resolved Hide resolved
fuzz/smime.c Outdated Show resolved Hide resolved
fuzz/smime.c Outdated
BIO *b = BIO_new_mem_buf(buf, len);
PKCS7 *p7 = SMIME_read_PKCS7(b, NULL);

if (p7) {
Copy link
Member

Choose a reason for hiding this comment

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

if (p7 != NULL) {

fuzz/smime.c Show resolved Hide resolved
fuzz/smime.c Outdated Show resolved Hide resolved
fuzz/smime.c Outdated Show resolved Hide resolved
test/recipes/99-test_fuzz_smime.t Outdated Show resolved Hide resolved
fuzz/smime.c Outdated Show resolved Hide resolved
Copy link
Member

@mattcaswell mattcaswell left a comment

Choose a reason for hiding this comment

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

LGTM. @kroeckx - can you reconfirm?

@mattcaswell mattcaswell added branch: master Merge to master branch approval: review pending This pull request needs review by a committer labels Feb 24, 2023
@kroeckx kroeckx added approval: done This pull request has the required number of approvals 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 and removed approval: review pending This pull request needs review by a committer labels Feb 24, 2023
@alex
Copy link
Contributor Author

alex commented Feb 25, 2023

is anything additionally required from me to merge this?

@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.

@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 Feb 25, 2023
@kroeckx
Copy link
Member

kroeckx commented Feb 25, 2023

No, it will get merged when someone has time for it.

@mattcaswell
Copy link
Member

Pushed. Thanks for your contribution.

openssl-machine pushed a commit that referenced this pull request Feb 27, 2023
Reviewed-by: Kurt Roeckx <kurt@roeckx.be>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #20332)
@alex alex deleted the smime-fuzzer branch February 27, 2023 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants