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

Add appropriate NULL checks in EVP_CIPHER api #22995

Closed
wants to merge 10 commits into from
Closed

Add appropriate NULL checks in EVP_CIPHER api #22995

wants to merge 10 commits into from

Conversation

nhorman
Copy link
Contributor

@nhorman nhorman commented Dec 9, 2023

The EVP_CIPHER api currently assumes that calls made into several APIs have already initalized the cipher in a given context via a call to EVP_CipherInit[_ex[2]]. If that hasnt been done, instead of an error, the result is typically a SIGSEGV.

Correct that by adding missing NULL checks in the apropriate apis prior to using ctx->cipher

Checklist
  • documentation is added or updated

@nhorman
Add appropriate NULL checks in EVP_CIPHER api
608b80f 
The EVP_CIPHER api currently assumes that calls made into several APIs
have already initalized the cipher in a given context via a call to
EVP_CipherInit[_ex[2]].  If that hasnt been done, instead of an error,
the result is typically a SIGSEGV.

Correct that by adding missing NULL checks in the apropriate apis prior
to using ctx->cipher
@nhorman nhorman self-assigned this Dec 9, 2023
@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Dec 9, 2023
t-j-h
t-j-h reviewed Dec 9, 2023
View reviewed changes
crypto/evp/evp_lib.c Outdated Show resolved Hide resolved
t-j-h
t-j-h reviewed Dec 9, 2023
View reviewed changes
doc/man3/EVP_EncryptInit.pod Outdated Show resolved Hide resolved
@t-j-h t-j-h added approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member labels Dec 9, 2023
@nhorman nhorman requested a review from t-j-h December 9, 2023 22:11
slontis
slontis reviewed Dec 10, 2023
View reviewed changes
Copy link
Member

@slontis slontis left a comment

Choose a reason for hiding this comment

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

Someone pointed out the getters crashing just recently...
EVP_CIPHER_CTX_get_key_length() has the same issue.

I would expect to see coverage tests..

@slontis slontis added the hold: needs tests The PR needs tests to be added to it label Dec 10, 2023
@slontis slontis mentioned this pull request Dec 10, 2023
Open
2 tasks
mattcaswell
mattcaswell requested changes Dec 11, 2023
View reviewed changes
crypto/evp/evp_lib.c Outdated Show resolved Hide resolved
t8m
t8m requested changes Dec 11, 2023
View reviewed changes
crypto/evp/evp_lib.c Outdated Show resolved Hide resolved
@nhorman
Copy link
Contributor Author

nhorman commented Dec 11, 2023

test added, removing hold on tests

@nhorman nhorman removed the hold: needs tests The PR needs tests to be added to it label Dec 11, 2023
t8m
t8m requested changes Dec 11, 2023
View reviewed changes
crypto/evp/evp_lib.c Outdated Show resolved Hide resolved
crypto/pem/pem_lib.c Outdated Show resolved Hide resolved
crypto/pem/pem_lib.c Outdated Show resolved Hide resolved
doc/man3/EVP_EncryptInit.pod Outdated
Comment on lines 526 to 527
B<EVP_CIPHER_CTX>. It will return zero if the cipher does not use an IV, or -1
in the event an error occurs (such as the cipher not yet being initalized).
Copy link
Member

Choose a reason for hiding this comment

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

Please no. I'm putting OTC hold on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Above issues fixed

@t8m
Copy link
Member

t8m commented Dec 11, 2023

OTC: Is it ok to introduce negative result of a function that could have never returned it before?

Concretely we are talking about EVP_CIPHER_get_iv_length() here.

@t8m t8m added hold: need otc decision The OTC needs to make a decision triaged: refactor The issue/pr requests/implements refactoring branch: master Merge to master branch tests: present The PR has suitable tests present labels Dec 11, 2023
@t8m
Copy link
Member

t8m commented Jan 9, 2024

OTC: Because of backwards compatibility reasons we should return 0 in case of error from EVP_CIPHER_get_iv_length().

We should add history note for EVP_CIPHER_get_iv_length() and EVP_CIPHER_get_block_size() about the error values being now returned.

@t8m t8m removed approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member labels Jan 9, 2024
slontis
slontis reviewed Jan 11, 2024
View reviewed changes
test/evp_extra_test.c Outdated Show resolved Hide resolved
slontis
slontis reviewed Jan 11, 2024
View reviewed changes
test/evp_libctx_test.c Outdated Show resolved Hide resolved
slontis
slontis reviewed Jan 11, 2024
View reviewed changes
test/evp_libctx_test.c Outdated Show resolved Hide resolved
t8m
t8m reviewed Jan 11, 2024
View reviewed changes
doc/man3/EVP_EncryptInit.pod Outdated Show resolved Hide resolved
t8m
t8m reviewed Jan 11, 2024
View reviewed changes
crypto/pem/pem_lib.c Outdated Show resolved Hide resolved
@jamuir jamuir mentioned this pull request Jan 11, 2024
Closed
2 tasks
@t8m t8m added the approval: review pending This pull request needs review by a committer label Jan 12, 2024
mattcaswell
mattcaswell requested changes Jan 15, 2024
View reviewed changes
crypto/evp/evp_lib.c Outdated Show resolved Hide resolved
crypto/evp/evp_lib.c Outdated Show resolved Hide resolved
crypto/pkcs12/p12_decr.c Show resolved Hide resolved
@t8m t8m requested a review from mattcaswell January 19, 2024 13:48
@mattcaswell mattcaswell removed their assignment Jan 24, 2024
@mattcaswell mattcaswell 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 24, 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 25, 2024
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@nhorman
Copy link
Contributor Author

nhorman commented Jan 25, 2024

merged.

@nhorman nhorman closed this Jan 25, 2024
openssl-machine pushed a commit that referenced this pull request Jan 25, 2024
@nhorman
Add appropriate NULL checks in EVP_CIPHER api
6f22bcd 
The EVP_CIPHER api currently assumes that calls made into several APIs
have already initalized the cipher in a given context via a call to
EVP_CipherInit[_ex[2]].  If that hasnt been done, instead of an error,
the result is typically a SIGSEGV.

Correct that by adding missing NULL checks in the apropriate apis prior
to using ctx->cipher

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #22995)
openssl-machine pushed a commit that referenced this pull request Jan 25, 2024
@nhorman
Add tests for return codes for EVP_CIPHER_CTX_get[block_size|iv_length]
72062fc 
make sure that we get the expected error codes when we do bad things,
rather than a crash

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #22995)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slontis slontis slontis left review comments

@t8m t8m t8m approved these changes

@mattcaswell mattcaswell mattcaswell approved these changes

@levitte levitte Awaiting requested review from levitte

@jamuir jamuir Awaiting requested review from jamuir

@t-j-h t-j-h Awaiting requested review from t-j-h

@hlandau hlandau Awaiting requested review from hlandau

Assignees

@nhorman nhorman

Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch severity: fips change The pull request changes FIPS provider sources tests: present The PR has suitable tests present triaged: refactor The issue/pr requests/implements refactoring
Projects
Project Board
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@nhorman @t8m @openssl-machine @hlandau @t-j-h @mattcaswell @slontis