CT::Poison: Return input

[FAlbertDev]

This adds more convenience to the new poison/unpoison interface by forwarding the input to the output. Especially for PQC algorithms like Classic McEliece, we have rejection sampling and other aborts, where we would need to transform something like

if(value != 0){
   return std::nullopt;
}

into

bool abort = (value != 0);
CT::unpoison(abort);
if(abort){
   return std::nullopt;
}

With this PR we can write:

if(CT::unpoison(value != 0)){
   return std::nullopt;
}

---

However, while writing this message, I'm asking myself if this might be a foot gun... For example, one could also write:

if(CT::unpoison(value) != 0){
   return std::nullopt;
}

and expect that value is still poisoned after the if-block is skipped, which is wrong. Do we only want to allow this syntax for booleans (we return an unpoisoned copy of the input boolean)? Or do we want to drop it entirely and use CT::Choice instead (which unpoisons internally)? What do you think?

[randombit]

CT::Mask has a concept unpoisoned_value for precisely this kind of thing, maybe it's better to generalize this idea (return an unpoisoned copy of a poisoned value) rather than overloading unpoison in this way, it seems very easy to footgun. CT::Mask::unpoisoned_value is not used much/at all currently - I think code that previously used it now uses CT::Choice and CT::Option instead.

I don't see how poison returning its input is useful.

[reneme]

Good point with the footgun! That certainly needs some thought.

I do think that there's value here, though. Eg. the getter below could be much simpler (and there's no footgun there!).

      DilithiumSerializedPrivateKey raw_sk() const {
         auto scope = CT::scoped_poison(*this);
         auto result = Dilithium_Algos::encode_private_key(m_rho, m_tr, m_signing_seed, m_s1, m_s2, m_t0, m_mode);
         CT::unpoison(result);
         return result;
      }

vs.

      DilithiumSerializedPrivateKey raw_sk() const {
         auto scope = CT::scoped_poison(*this);
         return CT::unpoison(Dilithium_Algos::encode_private_key(m_rho, m_tr, m_signing_seed, m_s1, m_s2, m_t0, m_mode));
      }

As a suggestion: we could restrict the pass-through convenience to rvalue-parameters. Because then there's no way to refer to the object that was manipulated in any other way but the returned reference. Like so:

std::vector<uint8_t> thing;
auto poisoned_thing = poison(thing); // compile error -> lvalue-reference overload does return void
auto poisoned_thing = poison(make_thing()); // ok -> make_thing passed an rvalue of thing
auto poisoned_thing = poison(std::move(thing)); // also ok, because one must not use `thing` after move.

That also fixes your initial example:

if(CT::unpoison(value != 0)) { //ok, because unpoisoning the rvalue of the comparison result
   return std::nullopt;
}

if(CT::unpoison(value) != 0) { // compile error -> value is an lvalue and `unpoison` returns void
   return std::nullopt;
}

[coveralls]

coverage: 91.654% (-0.003%) from 91.657% when pulling ba3d075f1bdf9afe50ea3fdf1dc74cf993a3816b on Rohde-Schwarz:valgrind/inline_poison into f1a91e752e7de034423343e6544afd94da636e38 on randombit:master.

[reneme]

Godbolt of a basic implementation: https://godbolt.org/z/qh49Ys6e1

[FAlbertDev]

Godbolt of a basic implementation: https://godbolt.org/z/qh49Ys6e1

Wow, I like it. I also like the idea of allowing r-values only. What do you think, @randombit? Are you okay with introducing this pass-through for r-values?

[reneme]

For the record: I do admit that this adds complexity in a sensitive utility that is fairly hard to test, and that we shouldn't do that lightly. As a compromise: instead of adding this rvalue-passthrough to all poison/unpoison overloads, we could add an explicit function that provides this functionality: like poison_and_forward(some_rvalue()), driveby_poison() ( :wink: ), ...

[FAlbertDev]

I've implemented the driveby_poison functions as @reneme suggested. I think that's a suitable compromise that cannot be misused since it is explicit, and only r-values are allowed as input. I currently work on valgrind for HSS/LMS, and these functions are also convenient in this context.

[FAlbertDev]

Thanks for the approve! @reneme could you briefly check the template magic before I merge this PR?