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

Fix error reporting in EVP_PKEY_{sign,verify,verify_recover} #23411

Closed
wants to merge 2 commits into from

Conversation

levitte
Copy link
Member

@levitte levitte commented Jan 29, 2024

For some reason, those functions (and the _init functions too) would
raise EVP_R_OPERATION_NOT_SUPPORTED_FOR_THIS_KEYTYPE when the passed
ctx is NULL, and then not check if the provider supplied the function
that would support these libcrypto functions.

This corrects the situation, and has all those libcrypto functions
raise ERR_R_PASS_NULL_PARAMETER if ctx is NULL, and then check for the
corresponding provider supplied, and only when that one is missing,
raise EVP_R_OPERATION_NOT_SUPPORTED_FOR_THIS_KEYTYPE.

For some reason, those functions (and the _init functions too) would
raise EVP_R_OPERATION_NOT_SUPPORTED_FOR_THIS_KEYTYPE when the passed
ctx is NULL, and then not check if the provider supplied the function
that would support these libcrypto functions.

This corrects the situation, and has all those libcrypto functions
raise ERR_R_PASS_NULL_PARAMETER if ctx is NULL, and then check for the
corresponding provider supplied, and only when that one is missing,
raise EVP_R_OPERATION_NOT_SUPPORTED_FOR_THIS_KEYTYPE.
@levitte levitte added branch: master Merge to master branch approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 tests: exempted The PR is exempt from requirements for testing branch: 3.2 Merge to openssl-3.2 labels Jan 29, 2024
@levitte
Copy link
Member Author

levitte commented Jan 29, 2024

As far as I know, we don't have much testing of error raised. That's
the reason I haven't added a test.

@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Jan 29, 2024
Because 0 doesn't mean error for EVP_PKEY_verify(), -1 is returned when
ERR_R_PASSED_NULL_PARAMETER is raised.  This is done consistently for all
affected functions.
@mattcaswell mattcaswell removed the approval: otc review pending This pull request needs review by an OTC member label Jan 29, 2024
Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

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

LGTM

@tom-cosgrove-arm tom-cosgrove-arm 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 Jan 29, 2024
@t8m t8m added the triaged: bug The issue/pr is/fixes a bug label Jan 29, 2024
@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 Jan 30, 2024
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@mattcaswell
Copy link
Member

Pushed to master/3.2/3.1/3.0

openssl-machine pushed a commit that referenced this pull request Jan 31, 2024
For some reason, those functions (and the _init functions too) would
raise EVP_R_OPERATION_NOT_SUPPORTED_FOR_THIS_KEYTYPE when the passed
ctx is NULL, and then not check if the provider supplied the function
that would support these libcrypto functions.

This corrects the situation, and has all those libcrypto functions
raise ERR_R_PASS_NULL_PARAMETER if ctx is NULL, and then check for the
corresponding provider supplied, and only when that one is missing,
raise EVP_R_OPERATION_NOT_SUPPORTED_FOR_THIS_KEYTYPE.

Because 0 doesn't mean error for EVP_PKEY_verify(), -1 is returned when
ERR_R_PASSED_NULL_PARAMETER is raised.  This is done consistently for all
affected functions.

Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #23411)

(cherry picked from commit 5a25177)
openssl-machine pushed a commit that referenced this pull request Jan 31, 2024
For some reason, those functions (and the _init functions too) would
raise EVP_R_OPERATION_NOT_SUPPORTED_FOR_THIS_KEYTYPE when the passed
ctx is NULL, and then not check if the provider supplied the function
that would support these libcrypto functions.

This corrects the situation, and has all those libcrypto functions
raise ERR_R_PASS_NULL_PARAMETER if ctx is NULL, and then check for the
corresponding provider supplied, and only when that one is missing,
raise EVP_R_OPERATION_NOT_SUPPORTED_FOR_THIS_KEYTYPE.

Because 0 doesn't mean error for EVP_PKEY_verify(), -1 is returned when
ERR_R_PASSED_NULL_PARAMETER is raised.  This is done consistently for all
affected functions.

Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #23411)

(cherry picked from commit 5a25177)
openssl-machine pushed a commit that referenced this pull request Jan 31, 2024
For some reason, those functions (and the _init functions too) would
raise EVP_R_OPERATION_NOT_SUPPORTED_FOR_THIS_KEYTYPE when the passed
ctx is NULL, and then not check if the provider supplied the function
that would support these libcrypto functions.

This corrects the situation, and has all those libcrypto functions
raise ERR_R_PASS_NULL_PARAMETER if ctx is NULL, and then check for the
corresponding provider supplied, and only when that one is missing,
raise EVP_R_OPERATION_NOT_SUPPORTED_FOR_THIS_KEYTYPE.

Because 0 doesn't mean error for EVP_PKEY_verify(), -1 is returned when
ERR_R_PASSED_NULL_PARAMETER is raised.  This is done consistently for all
affected functions.

Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #23411)
openssl-machine pushed a commit that referenced this pull request Jan 31, 2024
For some reason, those functions (and the _init functions too) would
raise EVP_R_OPERATION_NOT_SUPPORTED_FOR_THIS_KEYTYPE when the passed
ctx is NULL, and then not check if the provider supplied the function
that would support these libcrypto functions.

This corrects the situation, and has all those libcrypto functions
raise ERR_R_PASS_NULL_PARAMETER if ctx is NULL, and then check for the
corresponding provider supplied, and only when that one is missing,
raise EVP_R_OPERATION_NOT_SUPPORTED_FOR_THIS_KEYTYPE.

Because 0 doesn't mean error for EVP_PKEY_verify(), -1 is returned when
ERR_R_PASSED_NULL_PARAMETER is raised.  This is done consistently for all
affected functions.

Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #23411)

(cherry picked from commit 5a25177)
@levitte levitte deleted the fix-EVP_PKEY-operation-errors branch January 31, 2024 10:50
Sashan pushed a commit to Sashan/openssl that referenced this pull request Feb 12, 2024
For some reason, those functions (and the _init functions too) would
raise EVP_R_OPERATION_NOT_SUPPORTED_FOR_THIS_KEYTYPE when the passed
ctx is NULL, and then not check if the provider supplied the function
that would support these libcrypto functions.

This corrects the situation, and has all those libcrypto functions
raise ERR_R_PASS_NULL_PARAMETER if ctx is NULL, and then check for the
corresponding provider supplied, and only when that one is missing,
raise EVP_R_OPERATION_NOT_SUPPORTED_FOR_THIS_KEYTYPE.

Because 0 doesn't mean error for EVP_PKEY_verify(), -1 is returned when
ERR_R_PASSED_NULL_PARAMETER is raised.  This is done consistently for all
affected functions.

Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl#23411)
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: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 branch: 3.2 Merge to openssl-3.2 severity: fips change The pull request changes FIPS provider sources 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

5 participants