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

Convenience overloads for CT::poison() #4197

Merged
merged 4 commits into from
Jul 12, 2024

Conversation

reneme
Copy link
Collaborator

@reneme reneme commented Jul 11, 2024

This introduces a few convenience helpers for CT::poison(). For instance

CT::poison(some_integer_type);
CT::poison(some_range);
CT::poison(some_type_that_has_method_const_time_poison);
CT::poison_range(a_list_of_big_ints); // iterates the list and poisons each range element individually
CT::poison_all(some_int, some_range, some_type_with_const_time_poison);

auto scope = CT::scoped_poison(m_private); // will be unpoisoned at the end of scope

... and of course the respective CT::unpoison() functions.

@reneme reneme requested a review from randombit July 11, 2024 14:55
@reneme reneme self-assigned this Jul 11, 2024
Comment on lines 108 to 152
/**
* Unpoison a class type that provides a public `const_time_unpoison()` method
* For instance: BigInt, CT::Mask<>, FrodoMatrix, ...
*/
template <typename T>
requires requires(const T& x) { x.const_time_unpoison(); }
constexpr void unpoison(const T& x) {
x.const_time_unpoison();
}
Copy link
Collaborator Author

@reneme reneme Jul 11, 2024

Choose a reason for hiding this comment

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

We saw that BigInt::const_time_poison() was a thing already and just extended the idea to general types. This will be convenient for the PQC stuff in particular, as it often has helper-structures that could encapsulate their poisoning.

Ideally, this method should be _const_time_poison(), I guess, but we didn't want to break the existing BigInt API.

Copy link
Owner

Choose a reason for hiding this comment

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

We could alternately name it _const_time_poison and on BigInt just define const_time_poison as a deprecated forward.

Or just define another instance and handle both cases --

template <typename T>
   requires requires(const T& x) { x._const_time_unpoison(); }
constexpr void unpoison(const T& x) {
   x._const_time_unpoison();
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll rename them to _.... which is anyway the new convention.

@coveralls
Copy link

coveralls commented Jul 11, 2024

Coverage Status

coverage: 91.708% (+0.002%) from 91.706%
when pulling 85b06f4 on Rohde-Schwarz:ct/poison_overloads
into 1af5545 on randombit:master.

src/lib/utils/ct_utils.h Outdated Show resolved Hide resolved
src/lib/utils/ct_utils.h Outdated Show resolved Hide resolved
src/lib/utils/ct_utils.h Show resolved Hide resolved
src/lib/utils/ct_utils.h Show resolved Hide resolved
Co-Authored-By: Fabian Albert <fabian.albert@rohde-schwarz.com>
@@ -416,6 +493,10 @@ class Mask final {
*/
constexpr T value() const { return value_barrier<T>(m_mask); }

constexpr void const_time_poison() const { CT::poison(m_mask); }

constexpr void const_time_unpoison() const { CT::unpoison(m_mask); }
Copy link
Owner

Choose a reason for hiding this comment

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

Wait, this doesn't seem right. Are we poisoning/unpoisoning the masks currently somewhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. Here:

CT::unpoison(bad_input);

CT::unpoison(ok_mask);

Copy link
Owner

Choose a reason for hiding this comment

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

Ah ok, fallout from not having CT::Choice for this kind of logic.

Fine to merge this then, it's just keeping status quo. These should be fixed though.

@reneme
Copy link
Collaborator Author

reneme commented Jul 12, 2024

I applied all review suggestions and tweaked the requirements for the overloads a little. Most notably: poison(Range&&) now requires that the range's members are trivially copyable. Otherwise a (hypothetical) range of complex objects would have unexpected outcomes. Say poison(std::vector<std::vector<uint8_t>>) would have worked and (probably) poisoned the inner vectors' meta-data, which is certainly not what we would have expected.

Additionally, I added poison_range(R&&) which iterates any range of poisonable objects and poisons them individually. I deliberately decided to not make this an overload of poison() to avoid inadvertently binding to the wrong overload for some unforeseen reason.

Also note that I replaced the existing calls to something.const_time_poison() with CT::poison(something) for convention's sake (to avoid calling the "internal" _const_time_poison()).

reneme and others added 3 commits July 12, 2024 11:20
Essentially just renames all `const_time_poison()` members to `_const_time_poison()`.
This communicates that it is not meant for general use by library users and/or by
other code locations internally. Likewise for `...unpoison()`.
Co-Authored-By: Fabian Albert <fabian.albert@rohde-schwarz.com>
@reneme reneme merged commit 6c92e4b into randombit:master Jul 12, 2024
39 checks passed
@reneme reneme deleted the ct/poison_overloads branch July 12, 2024 10:00
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