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

Classic McEliece Side Channel Report #204

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Classic McEliece Side Channel Report #204

wants to merge 4 commits into from

Conversation

aewag
Copy link
Collaborator

@aewag aewag commented Mar 27, 2024

Not yet ready for review

@falsecurity
Copy link
Collaborator

Ready for integration and review

@reneme
Copy link
Collaborator

reneme commented Mar 28, 2024

Thanks a lot

Failing CI is due to the missing #203. No problem for now, but please rebase to latest main at one point. :)

@reneme reneme changed the base branch from release/3.3.0 to main March 28, 2024 12:18
@reneme
Copy link
Collaborator

reneme commented Mar 28, 2024

Never mind! It interfered with some other changes while integrating, so I just went ahead and rebased your commits onto main. Please be aware when you decide to do further edits and commits.

@reneme reneme added this to the Botan 3.4.0 milestone Mar 28, 2024
@reneme reneme changed the title docs/sca: Add CMCE Classic McEliece Side Channel Report Mar 28, 2024
Copy link
Collaborator

@FAlbertDev FAlbertDev 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 the report. Looks very good to me, both technically and semantically. I left some minor suggestions.

docs/audit_report/src/side_channels/01_XX_cmce.rst Outdated Show resolved Hide resolved
docs/audit_report/src/side_channels/01_XX_cmce.rst Outdated Show resolved Hide resolved
docs/audit_report/src/side_channels/01_XX_cmce.rst Outdated Show resolved Hide resolved
Comment on lines +154 to +160
private:
constexpr bitref& assign(bool bit) noexcept {
const block_type assign_mask = 0 - static_cast<block_type>(bit);
this->m_block \|= (this->m_mask & assign_mask);
this->m_block &= ~(this->m_mask & ~assign_mask);
return \*this;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

We fixed the side channel using Botan's constant time helper class CT::Mask, which is more readable, in my opinion and does similar things as this code block. I guess your code translates better to assembly, though. So I'm fine with you version. (So nothing to do here, just a comment)

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's probably worth mentioning that the later (and final) versions of the code fix this side channel

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note added in 5df0aa0

@FAlbertDev FAlbertDev marked this pull request as ready for review March 28, 2024 12:52
@FAlbertDev
Copy link
Collaborator

Just so you know, you can take your time to embed our review comments. We currently collect all submission documents and will send them soon. In the past, reacting to review comments after the deadline was always totally fine.
So, thanks again for finishing this report over the last few days :)

@reneme reneme modified the milestones: Botan 3.4.0, Botan 3.6.0 Jun 13, 2024
@reneme reneme modified the milestones: Botan 3.6.0, Botan 3.7.0 Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants