On Thu, 2021-01-21 at 10:52 +0100, Ard Biesheuvel wrote:

On Wed, 20 Jan 2021 at 20:00, Alessandrelli, Daniele
[off-list ref] wrote:

quoted

Hi Ard,

Thank you very much for your valuable feedback.

On Mon, 2021-01-18 at 13:09 +0100, Ard Biesheuvel wrote:

quoted

This is rather unusual compared with how the crypto API is
typically
used, but if this is really what you want to implement, you can
do so
by:
- having a "ecdh" implementation that implements the entire
range, and
uses a fallback for curves that it does not implement
- export the same implementation again as "ecdh" and with a known
driver name "ecdh-keembay-ocs", but with a slightly lower
priority,
and in this case, return an error when the unimplemented curve is
requested.

That way, you fully adhere to the API, by providing
implementations of
all curves by default. And if a user requests "ecdh-keembay-ocs"
explicitly, it will not be able to use the P192 curve
inadvertently.

I tried to implement this, but it looks like the driver name is
mandatory, so I specified one also for the first implementation.

Basically I defined two 'struct kpp_alg' variables; both with
cra_name
= "ecdh", but with different 'cra_driver_name' (one with
cra_driver_name = "ecdh-keembay-ocs-fallback" and the other one
with
cra_driver_name = "ecdh-keembay-ocs").

Is this what you were suggesting?

Anyway, that works (i.e., 'ecdh-keembay-ocs' returns an error when
the
unimplemented curve is requested; while 'ecdh-keembay-ocs' and
'ecdh'
work fine with any curve), but I have to set the priority of 'ecdh-
keembay-ocs' to something lower than the 'ecdh_generic' priority.
Otherwise the implementation with fallback ends up using my "ecdh-
keembay-ocs" as fallback (so it ends up using a fallback that still
does not support the P-192 curve).

Also, the implementation without fallback is still failing crypto
self-
tests (as expected I guess).

Therefore, I tried with a slightly different solution. Still two
implementations, but with different cra_names (one with cra_name =
"ecdh" and the other one with cra_name = "ecdh-keembay"). This
solution
seems to be working, since, the "ecdh-keembay" is not tested by the
self tests and is not picked up as fallback for "ecdh" (since, if I
understand it correctly, it's like if I'm defining a new kind of
kpp
algorithm), but it's still picked when calling
crypto_alloc_kpp("ecdh-
keembay").

Does this second solution looks okay to you? Or does it have some
pitfall that I'm missing?

You should set the CRYPTO_ALG_NEED_FALLBACK flag on both
implementations, to ensure that neither of them are considered as
fallbacks themselves.

Thanks again!

I was setting that flag only for the first implementation (the one with
fallback), but I see now how it's needed for the second one as well.

With that, the second implementation (i.e., the one without fallback)
is not used anymore as a fallback for the first one.

As expected, the second implementation does not pass self-tests and
crypto_alloc_kpp() returns -ELIBBAD when trying to allocate it, but
I've seen that I can avoid the error (and have it allocated properly)
by passing the CRYPTO_ALG_TESTED flag in the 'type' argument, like
below:

   crypto_alloc_kpp("ecdh-keembay-ocs", CRYPTO_ALG_TESTED, 0);

Is that the right way to tell crypto_alloc_kpp() that we are fine using
an implementation that fails self-tests?