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

asn1: add ASN1_STRING_set() check result #21587

Closed
wants to merge 2 commits into from

Conversation

nv-dmd
Copy link

@nv-dmd nv-dmd commented Jul 28, 2023

Function asn1_str2type() (crypto\asn1\asn1_gen.c) uses function ASN1_STRING_set() to set the ASN1 string (line 702), but the result of ASN1_STRING_set() execution is not checked, although it may fail.
In the same function asn1_str2type() on line 648 the result of ASN1_STRING_set() is checked.

Added ASN1_STRING_set() check result.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

crypto/asn1/asn1_gen.c Outdated Show resolved Hide resolved
@mattcaswell mattcaswell added branch: master Merge to master branch approval: review pending This pull request needs review by a committer branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 labels Jul 28, 2023
@kroeckx kroeckx 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 Jul 29, 2023
@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 Jul 30, 2023
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

Copy link
Member

@bernd-edlinger bernd-edlinger left a comment

Choose a reason for hiding this comment

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

other places do a ERR_raise(ERR_LIB_ASN1, ERR_R_MALLOC_FAILURE);
suggest to do the same here

@nv-dmd
Copy link
Author

nv-dmd commented Aug 4, 2023

other places do a ERR_raise(ERR_LIB_ASN1, ERR_R_MALLOC_FAILURE); suggest to do the same here

In the master branch is always done a ERR_raise(..., ERR_R_ASN1_LIB), but in the branches 3.0 and 3.1 done a ERR_raise(..., ERR_R_MALLOC_FAILURE)

What is better to done in such a case?

@bernd-edlinger
Copy link
Member

oh, thanks for pointing that out,
I've only looked at 3.0 3.1 and did not expect master to be different.
but in that case it would be better to have a different PR for 3.0/3.1
where ERR_R_MALLOC_FAILURE is used instead.

@tmshort
Copy link
Contributor

tmshort commented Aug 4, 2023

I can remove the (openssl-3.0) and (openssl-3.1) tags, and merge this just to master, @bernd-edlinger?

@bernd-edlinger
Copy link
Member

yes this is certainly okay for master

@tmshort tmshort removed branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 labels Aug 4, 2023
@tmshort tmshort dismissed bernd-edlinger’s stale review August 4, 2023 17:24

This will be merged to master only; @nv-dmd will make a new PR for 3.1/3.0

@tmshort tmshort self-assigned this Aug 4, 2023
@tmshort tmshort added tests: exempted The PR is exempt from requirements for testing triaged: bug The issue/pr is/fixes a bug labels Aug 4, 2023
@tmshort
Copy link
Contributor

tmshort commented Aug 4, 2023

Adding (tests: exempted) because the external-tests are failing for master.

@tmshort
Copy link
Contributor

tmshort commented Aug 4, 2023

Merged to master. Thank you for your contribution!

@tmshort tmshort closed this Aug 4, 2023
openssl-machine pushed a commit that referenced this pull request Aug 4, 2023
Reviewed-by: Kurt Roeckx <kurt@roeckx.be>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Todd Short <todd.short@me.com>
(Merged from #21587)
@nv-dmd nv-dmd deleted the asn1_check_ASN1_STRING_set branch August 4, 2023 18:57
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 tests: exempted The PR is exempt from requirements for testing triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants