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

Update Classic McEliece suppression files #1541

Merged
merged 6 commits into from
Sep 13, 2023
Merged

Update Classic McEliece suppression files #1541

merged 6 commits into from
Sep 13, 2023

Conversation

praveksharma
Copy link
Member

Updates suppression files to include potential leaks document in #1540.
Updates Classic McEliece advisories to include information about environment-specific constant time leaks.
Removes Classic-McEliece-6XX from weekly constant time test skip list.

  • [No] Does this PR change the input/output behaviour of a cryptographic algorithm (i.e., does it change known answer test values)? (If so, a version bump will be required from x.y.z to x.(y+1).0.)
  • [No] Does this PR change the list of algorithms available -- either adding, removing, or renaming? Does this PR otherwise change an API? (If so, PRs in oqs-provider, OQS-OpenSSL, OQS-BoringSSL, and OQS-OpenSSH will also need to be ready for review and merge by the time this is merged.)

@praveksharma
Copy link
Member Author

@bhess I was hoping you could help me work on this issue, since I can't reproduce these errors locally. Could you check if Valgrind still complains about potential constant time leaks?

It might be a good idea to add --error-limit=no to valgrind's options list to ensure valgrind reports all errors.

Copy link
Member

@baentsch baentsch left a comment

Choose a reason for hiding this comment

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

Thanks for keeping updating the suppression files for this algorithm family.

As this PR adds this (IMO very correct) advisory:

Current implementation of the algorithm may not be constant-time

and looking at the length/extent of the suppression files and the above advisory I wonder whether it wouldn't be simpler to completely exclude McEliece from constant time testing until someone states interest in giving McEliece the CT property (?) We have something similar for BIKE -- although I wonder whether this is (still) correct given there's no corresponding advisory for BIKE (???).

@bhess
Copy link
Member

bhess commented Sep 8, 2023

@bhess I was hoping you could help me work on this issue, since I can't reproduce these errors locally. Could you check if Valgrind still complains about potential constant time leaks?

I ran the tests and there is one error remaining. Seems to be all fine when adding one more rule.

{
   This implementation of Classic McEliece may not be constant time.
   Memcheck:Value8
   src:pk_gen.c:322
   # fun:PQCLEAN_MCELIECE348864_AVX2_pk_gen
   fun:PQCLEAN_MCELIECE348864_AVX2_crypto_kem_keypair
}

@praveksharma
Copy link
Member Author

praveksharma commented Sep 11, 2023

looking at the length/extent of the suppression files and the above advisory I wonder whether it wouldn't be simpler to completely exclude McEliece from constant time testing until someone states interest in giving McEliece the CT property (?)

I think this is a good decision and I've added Classic McEliece to the skip list in .github/weekly.yml.

We have something similar for BIKE -- although I wonder whether this is (still) correct given there's no corresponding advisory for BIKE (???).

Looking through the file history for the BIKE suppression file, it looks like the advisories weren't updated when the suppression file was created. I've added a similar advisory for BIKE.

I've updated the suppression file with the remaining error, we should be good to go.

Copy link
Member

@bhess bhess left a comment

Choose a reason for hiding this comment

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

The McEliece ct-run looks ok on my test machine. Thanks for the update!

However, I didn't notice any BIKE errors. There also seem to be no issues registered in issues.json. I think this file could be removed since ct-fixes to BIKE were made in #1400.

Copy link
Member

@baentsch baentsch left a comment

Choose a reason for hiding this comment

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

Beyond the single comment made I concur with @bhess that

this file could be removed since ct-fixes to BIKE were made in #1400.

@praveksharma praveksharma merged commit 7ef422a into main Sep 13, 2023
26 checks passed
@dstebila dstebila deleted the ps-mceliece branch November 28, 2023 15:13
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.

3 participants