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

ciphers/signatures: Reject update if module isn't operational #22506

Conversation

neverpanic
Copy link
Contributor

When a FIPS self-test such as for example a pair-wise consistency test fails, the module should go into an error state and refuse any further cryptographic operations.

This is not the case for the RSA or ECDSA pair-wise consistency test, after which EVP_DigestUpdate() and EVP_EncryptUpdate() can still be called.

Add invocations of ossl_prov_is_running() to provider functions called by EVP_DigestUpdate() and EVP_EncryptUpdate() to fix this.

I am aware that new contexts cannot be created, and existing contexts cannot be finalized because the init and final variants of those APIs have the ossl_is_prov_running() call, but our lab claims that is not sufficient, and the update API must also not work on PCT failure.

We added an abort() downstream for these cases, since it's really a corner case that essentially never happens outside of bit-flips, but that's clearly not appropriate upstream, and neither is it a clean solution. In order to reduce our downstream patches, I'm proposing this instead.

When a FIPS self-test such as for example a pair-wise consistency test
fails, the module should go into an error state and refuse any further
cryptographic operations.

This is not the case for the RSA or ECDSA pair-wise consistency test,
after which EVP_DigestUpdate() can still be called.

Add invocations of ossl_prov_is_running() to provider functions called
by EVP_DigestUpdate() to fix this.

Signed-off-by: Clemens Lang <cllang@redhat.com>
When a FIPS self-test such as for example a pair-wise consistency test
fails, the module should go into an error state and refuse any further
cryptographic operations.

This is not the case for the RSA or ECDSA pair-wise consistency test,
after which EVP_EncryptUpdate() can still be called.

Add invocations of ossl_prov_is_running() to provider functions called
by EVP_EncryptUpdate() to fix this.

Signed-off-by: Clemens Lang <cllang@redhat.com>
@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Oct 25, 2023
@t8m
Copy link
Member

t8m commented Oct 25, 2023

For DigestUpdate there is no consequence of this check missing as there is no output generated until DigestFinal() is called. So it is doubtful whether this is really mandated by the FIPS 140 standard. On the other hand CipherUpdate produces an output so this seems like a bug to me.

@t8m t8m 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 triaged: bug The issue/pr is/fixes a bug 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 labels Oct 25, 2023
@neverpanic
Copy link
Contributor Author

For DigestUpdate there is no consequence of this check missing as there is no output generated until DigestFinal() is called. So it is doubtful whether this is really mandated by the FIPS 140 standard.

That was also what we argued, but considering that a hashing operation may be triggered in DigestUpdate(), our lab did not accept this explanation.

@paulidale
Copy link
Contributor

paulidale commented Oct 25, 2023

Your lab is mistaken. Push back harder. Laugh at them if need be. If they are not willing to be reasonable, request a formal CMVP ruling about this and present your argument there. Be warned that such rulings can result in detrimental outcomes so you need to be prepared and very persistent.

Our 140-2 validation passed with things are they are. We've heard of no issues with this in KeyPair's 140-3 validation (upon which ours is based). It's simply not required. What's currently there meets the letter and intent of the requirement.

I strongly oppose this change, it introduces what could potentially be a performance regression. The update functions were deliberately excluded from the running checks because of this concern. I.e. the current behaviour is intentional and will not be changed.

@paulidale paulidale added resolved: not a bug The issue is not considered a bug resolved: wont fix The issue has been confirmed but won't be fixed and removed branch: 3.0 Merge to openssl-3.0 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 labels Oct 25, 2023
@neverpanic
Copy link
Contributor Author

Thanks for the review, I'll go back to them and see what we can do.

@t8m t8m added the branch: 3.2 Merge to openssl-3.2 label Oct 26, 2023
@mattcaswell
Copy link
Member

Closing since this is marked as resolved.

@neverpanic
Copy link
Contributor Author

We actually haven't heard back from NIST on this. I'll make sure to ask for a status update in our next meeting with our lab.

They wouldn't let us submit without a change to address this concern, so we're still very much interested in a resolution where we don't have to carry downstream patches.

I'll come back with more info when I have it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch: master Merge to master branch branch: 3.1 Merge to openssl-3.1 branch: 3.2 Merge to openssl-3.2 resolved: not a bug The issue is not considered a bug resolved: wont fix The issue has been confirmed but won't be fixed 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

4 participants