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 for mutual authentication to prevent mismatch of certificate and sigalgo #4813

Merged
merged 2 commits into from Feb 7, 2022

Conversation

dgarske
Copy link
Contributor

@dgarske dgarske commented Feb 1, 2022

Description

Fix for mutual authentication to prevent mismatch of certificate and sig algo. Fixes from Sean P.

Fixes ZD13571

Testing

Tested by customer.

Checklist

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

@dgarske dgarske self-assigned this Feb 1, 2022
src/tls13.c Outdated Show resolved Hide resolved
WOLFSSL_MSG("Peer sent RSA sig but not RSA cert");
if (args->sigAlgo == rsa_pss_sa_algo) {
WOLFSSL_MSG("Peer sent RSA sig");
validSigAlgo = (ssl->peerRsaKey != NULL) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does only RSA need this addition key != NULL check when each algo has a key pointer in the ssl struct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, I will 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.

Looks like that is existing logic that is carried over perhaps from static RSA. I will see if it makes sense to add the key pointer check to any other algos.

WOLFSSL_MSG("Peer sent ED448 sig but not ED448 cert");
ret = SIG_VERIFY_E;
goto exit_dcv;
if (args->sigAlgo == ed448_sa_algo) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we change all these if's to else if's ......

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, makes the cases with #if complicated. Not changing.

WOLFSSL_MSG("Peer sent RSA sig");
validSigAlgo = (ssl->peerRsaKey != NULL) &&
ssl->peerRsaKeyPresent;
}
Copy link
Member

@anhu anhu Feb 3, 2022

Choose a reason for hiding this comment

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

and then here put a big comment

} else {
    /* args->sigAlgo is unrecognized ! */
    validSigAlgo = 0;
}

Just to re-iterate that its not one of the values we expected?
Makes the logic much clearer and would probably be more efficient as once one else if clause evaluates to true, it finishes. Also the compiler would optimize out that final else statement so it wouldn't impact the code size much.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not planning to, since the macros around the if() make it problematic. I see no issue with the code running all the logic paths.

@@ -6564,35 +6564,41 @@ static int DoTls13CertificateVerify(WOLFSSL* ssl, byte* input,
}

/* Check for public key of required type. */
/* Assume invalid unless signature algo matches the key provided */
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, ok, this does the trick.

src/tls13.c Outdated
goto exit_dcv;
if (args->sigAlgo == falcon_level5_sa_algo) {
WOLFSSL_MSG("Peer sent Falcon Level 5 sig");
validSigAlgo = validSigAlgo = (ssl->peerFalconKey != NULL) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Double validSigAlgo =

… `validSigAlgo` to zero with comment to clarify the default assumption.
@SparkiDev SparkiDev requested review from julek-wolfssl and removed request for julek-wolfssl February 7, 2022 00:57
@SparkiDev SparkiDev assigned SparkiDev and unassigned julek-wolfssl Feb 7, 2022
@SparkiDev SparkiDev dismissed julek-wolfssl’s stale review February 7, 2022 00:59

Merging now. Comment responded to.

@SparkiDev SparkiDev merged commit f6d79ff into wolfSSL:master Feb 7, 2022
28 checks passed
@dgarske dgarske deleted the zd13571 branch February 7, 2022 19:34
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.

None yet

5 participants