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

Allow overriding CRL chain errors early so CRL chain processing will continue. #7501

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

kareem-wolfssl
Copy link
Contributor

Description

Fixes zd#17844

Testing

Customer confirmed fix

Checklist

  • added tests
  • updated/added doxygen
  • updated appropriate READMEs
  • Updated manual and documentation

@kareem-wolfssl kareem-wolfssl self-assigned this May 3, 2024
@kareem-wolfssl kareem-wolfssl removed their assignment May 13, 2024
src/crl.c Outdated
@@ -512,9 +512,11 @@ int CheckCertCRL_ex(WOLFSSL_CRL* crl, byte* issuerHash, byte* serial,
#endif
if (foundEntry == 0) {
WOLFSSL_MSG("Couldn't find CRL for status check");
#ifndef WOLFSSL_ALLOW_MISSING_CRL
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this only be an option for root/intermediates but not the leaf cert? What will ret be in the missing case? Please add inline code comment for purpose of this new macro.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the opposite, the customer wants it on the leaf and intermediates, not the root. The intention of the workaround is to continue parsing and verifying certs and CRLs up the chain even if a CRL is missing.
In the missing case, ret will be 0 as CheckCertCRLList would not have found any CRLs, so it wouldn't modify ret from the default value of 0. But depending on the build flags to wolfSSL, ret may be overwritten by crlIoCb.
I've added an inline comment, please let me know what you think.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to limit this to just the root? I am concerned there is not differentiation between the type of cert. I want to make sure the leaf cert still has the CRL missing check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again the customer wants to skip the check on the leaf and intermediates. They want to continue parsing even after a CRL_MISSING error so they can catch invalid certs or CRLs further up the chain. So if anything we should limit it to the leaf and intermediates.
This function does not have context on what point in the chain we are at, the workaround would need to be moved to a higher level function which is calling this ie. ProcessPeerCertsChainCRLCheck. I can move this, just let me know.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes please review and see if it's possible to move it up the stack and do it where the type of cert is known.... I.E. root ca

Copy link
Member

Choose a reason for hiding this comment

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

In the test_wolfSSL_X509_STORE_set_get_crl test at #7445 I ignore CRL missing errors with the verification callback. It continues parsing the cert chain so the customer should be able to use the verify callback. Have they tried that?

src/crl.c Outdated
@@ -512,9 +512,11 @@ int CheckCertCRL_ex(WOLFSSL_CRL* crl, byte* issuerHash, byte* serial,
#endif
if (foundEntry == 0) {
WOLFSSL_MSG("Couldn't find CRL for status check");
#ifndef WOLFSSL_ALLOW_MISSING_CRL
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to limit this to just the root? I am concerned there is not differentiation between the type of cert. I want to make sure the leaf cert still has the CRL missing check.

@dgarske dgarske removed their assignment May 15, 2024
@dgarske dgarske removed the request for review from wolfSSL-Bot May 16, 2024 16:51
@kareem-wolfssl kareem-wolfssl changed the title Add a flag to allow bypassing a CRL missing error. Allow overriding CRL chain errors early so CRL chain processing will continue. May 23, 2024
@dgarske
Copy link
Contributor

dgarske commented May 24, 2024

Retest this please

Copy link
Member

@julek-wolfssl julek-wolfssl left a comment

Choose a reason for hiding this comment

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

You need to find a way to not call the verify callback twice for the same failure. If ProcessPeerCertsChainCRLCheck fails and DoVerifyCallback doesn't override the error the verify callback will be called twice. The easiest way would be to perform the ProcessPeerCertsChainCRLCheck call after DoVerifyCallback in ProcessPeerCerts.

@kareem-wolfssl
Copy link
Contributor Author

I tried moving the check to the end of the function, but this lead to test failures, and doesn't seem scalable if we decide to call the verify callback in more spots in the future.
Instead, I've added a flag to indicate the verify callback did not override an error which prevents it from being called again. This should cover the case you laid out.

@dgarske dgarske removed their request for review September 12, 2024 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants