See https://github.com/HACS-workshop/spectre-mitigations/blob/master/crypto_guidelines.md#1-do-not-conditionally-choose-between-constant-and-non-constant-time. I'm biased, but I think BoringSSL's solution, which was to use separate APIs for constant-time and non-constant-time implementations, is better, in general.

Agreed, though I suppose that too is unsurprising. :-)

Echoing what I wrote on openssl-project:

In a crypto library, something like "modular exponentiation" is not a complete API contract. Rather, you need contracts like "modular exponentiation with secret fully-reduced base and public exponent" or "modular exponentiation with secret fully-reduced base and secret exponent publicly bounded by such-and-such". Cast in that light, I think it becomes much clearer that they should be separate functions. You could even imagine embedding secrecy into the type (either for safety or to take advantage of some future compiler annotations), in which case separate functions is necessary.

This PR instead makes things even further dependent on bit propagation, which was a large part of why the old scheme was error-prone and difficult to analyze. While the flipped default probably helps a bit, most BIGNUM functions do not allocate their results, so things are still ultimately dependent on bit propagation correctly overwriting whatever was in the result BIGNUM originally.

Separate functions, in contrast, place the algorithm decision right at the call site, where one typically thinks about it. It's much easier to reason about what code runs where, and whether the right code runs.

Separate functions feels better to me as well. Trying to track public vs private automatically is a noble ideal but it relies on the programmer setting things correctly every time. This PR will be easier/safer than having to remember to set and clear the constant time flag but the risk seems high still. Separate functions has this risk too but it is more in the face -- someone looking at the documentation will see both versions immediately rather than having to remember to look at this other page of documentation, which they mightn't even know exists, for the public/private differentiator. I'd advocate adding a suffix _public (for want of a better name) to the non-constant time flavours and making the shorter/existing names constant time. This means a performance hit for people who call BN directly, at least until they amend their code, but it shouldn't break anything.

A separate item along similar lines would be making the small memory model builds only have the constant time flavour of these routines.

Thanks for the link and other feedback. A separate API does seem like a good idea. I'm going to leave this PR open for a while longer to see if there is any other feedback.

I agree with above commentators that it is better to have distinct API endpoints for what are in fact distinct operations.

Bikeshedding here, but I'd say rather than _public - which may be easily confusing to an end user (ie, does _public mean it's the public API function and the non-_public is internal?) - use _vartime as is done in the Ed25519/X25519 code. Already public appears in many contexts within OpenSSL source, where vartime does not, so it would more easily catch the eye of a reviewer.

I opened #6078 to track this issue, so I'm going to close this PR.