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 bad & statement that was setting ocspSendNonce #6627

Merged
merged 1 commit into from
Jul 27, 2023

Conversation

jpbland1
Copy link
Contributor

to 1 when WOLFSSL_OCSP_NO_NONCE was selected
related to but doesn't solve zd 16377

Description

OCSP sends an optional nonce which is supposed to be disabled by WOLFSSL_OCSP_NO_NONCE but the logical statement was doing the opposite and sending the nonce when WOLFSSL_OCSP_NO_NONCE was set

Testing

Tested in gdb to confirm the nonce was being skipped

Checklist

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

@jpbland1 jpbland1 requested a review from dgarske July 17, 2023 16:55
@jpbland1 jpbland1 marked this pull request as ready for review July 17, 2023 16:56
Copy link
Contributor

@dgarske dgarske left a comment

Choose a reason for hiding this comment

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

Please share steps to reproduce and make sure we have test coverage.

@dgarske dgarske removed their assignment Jul 17, 2023
@jpbland1 jpbland1 requested a review from SparkiDev July 24, 2023 16:51
@jpbland1
Copy link
Contributor Author

jpbland1 commented Jul 24, 2023

Originally, wolfSSL_CertManagerEnableOCSP set the variable ocspSendNonce by checking if the WOLFSSL_OCSP_NO_NONCE option was set:

        if (options & WOLFSSL_OCSP_NO_NONCE)
            cm->ocspSendNonce = 0;
        else
            cm->ocspSendNonce = 1;

When it was moved and refactored in , it looks like a logical error was made and ocspSendNonce is now set like:

        /* Set nonce option for creating OCSP requests. */
        cm->ocspSendNonce = (options & WOLFSSL_OCSP_NO_NONCE) != 0;

which sets ocspSendNonce to the opposite of what it should be, since (options & WOLFSSL_OCSP_NO_NONCE) != 0 will be 1 when WOLFSSL_OCSP_NO_NONCE is set and 0 when it is not set. It's hard to test this since we don't have a responder that we can check for the nonce with. You can see it's wrong manually by looking in gdb to see that the nonce is being set when it shouldn't be.

Anyways, all we need to do is change it to (options & WOLFSSL_OCSP_NO_NONCE) != WOLFSSL_OCSP_NO_NONCE which will be 0 when WOLFSSL_OCSP_NO_NONCE is set and 1 when not set.

@dgarske
Copy link
Contributor

dgarske commented Jul 24, 2023

The current code works for me:

#define WOLFSSL_OCSP_NO_NONCE 2
uint32_t options, testnonce, ocspSendNonce;

options = WOLFSSL_OCSP_NO_NONCE;
testnonce = (options & WOLFSSL_OCSP_NO_NONCE);
ocspSendNonce = (options & WOLFSSL_OCSP_NO_NONCE) != 0;
printf("options 0x%x, test %u, sendNonce %u\n", options, testnonce, ocspSendNonce);

options = 0;
testnonce = (options & WOLFSSL_OCSP_NO_NONCE);
ocspSendNonce = (options & WOLFSSL_OCSP_NO_NONCE) != 0;
printf("options 0x%x, test %u, sendNonce %u\n", options, testnonce, ocspSendNonce);
options 0x2, test 2, sendNonce 1
options 0x0, test 0, sendNonce 0

@dgarske
Copy link
Contributor

dgarske commented Jul 24, 2023

The current code works for me:

#define WOLFSSL_OCSP_NO_NONCE 2
uint32_t options, testnonce, ocspSendNonce;

options = WOLFSSL_OCSP_NO_NONCE;
testnonce = (options & WOLFSSL_OCSP_NO_NONCE);
ocspSendNonce = (options & WOLFSSL_OCSP_NO_NONCE) != 0;
printf("options 0x%x, test %u, sendNonce %u\n", options, testnonce, ocspSendNonce);

options = 0;
testnonce = (options & WOLFSSL_OCSP_NO_NONCE);
ocspSendNonce = (options & WOLFSSL_OCSP_NO_NONCE) != 0;
printf("options 0x%x, test %u, sendNonce %u\n", options, testnonce, ocspSendNonce);
options 0x2, test 2, sendNonce 1
options 0x0, test 0, sendNonce 0

Test code: This is backwards, right?

@jpbland1
Copy link
Contributor Author

yes it's backwards, when it's set the result should be zero, not 1

Copy link
Contributor

@dgarske dgarske left a comment

Choose a reason for hiding this comment

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

Confirmed backwards logic is correctly fixed with this. Thanks @jpbland1

@bandi13
Copy link
Contributor

bandi13 commented Jul 24, 2023

retest this please

to 1 when WOLFSSL_OCSP_NO_NONCE was selected
related to but doesn't solve zd 16377
@bandi13
Copy link
Contributor

bandi13 commented Jul 25, 2023

retest this please

@dgarske dgarske merged commit c529b2f into wolfSSL:master Jul 27, 2023
70 checks passed
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