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

Document Falcon constant time errors #1552

Merged
merged 4 commits into from
Sep 18, 2023
Merged

Document Falcon constant time errors #1552

merged 4 commits into from
Sep 18, 2023

Conversation

praveksharma
Copy link
Member

@praveksharma praveksharma commented Sep 14, 2023

Documents Falcon constant time errors being caught by CI.
Adds updates to Classic McEliece docs that were missed in #1541.

[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 praveksharma marked this pull request as draft September 14, 2023 15:29
@SWilson4
Copy link
Member

Tacking on documentation for the recently added Falcon ARM implementation.

@praveksharma praveksharma added this to the 0.9.0 milestone Sep 14, 2023
@praveksharma praveksharma added the bug Something isn't working; high priority to fix label Sep 14, 2023
@praveksharma praveksharma self-assigned this Sep 14, 2023
@praveksharma praveksharma marked this pull request as ready for review September 14, 2023 19:20
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.

The contents of the PR look OK and logical. But it seems to document a regression in Falcon and the liboqs-built-in SHA3 code (not being CT any more), do I understand this right? Is Falcon indeed the only algorithm affected by this common code CT-failure?

@baentsch
Copy link
Member

The contents of the PR look OK and logical. But it seems to document a regression in Falcon and the liboqs-built-in SHA3 code (not being CT any more), do I understand this right? Is Falcon indeed the only algorithm affected by this common code CT-failure?

Correcting myself: I mis-read the YML indentation level documenting the CT error (falsely thinking common SHA3 is the problem). Assuming I now read correctly: Is this Falcon failure only in the AVX2 optimization? If so, what is the reason that all Falcon code is now considered to have secret dependent branching (assuming I read the YML right this time...)?

@praveksharma
Copy link
Member Author

Is this Falcon failure only in the AVX2 optimization? If so, what is the reason that all Falcon code is now considered to have secret dependent branching?

The errors are in fact only only in the AVX2 optimization. I didn't realise that default behaviour was to enable AVX2 optmisations even supplying the -DOQS_USE_AVX2_INSTRUCTIONS=OFF option when building on x86_64; I also missed the AVX2 tag in the suppression files, hence the incorrect documentation. I've made the relevant corrections and will merge the changes.

@praveksharma praveksharma merged commit e6c650c into main Sep 18, 2023
35 checks passed
@dstebila dstebila deleted the ps-falcon branch November 28, 2023 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working; high priority to fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants