Open matlimatli opened 5 months ago
I agree that the void
return is less than ideal. However, I believe this is a legacy of the DRBG that NIST provided for submissions to the PQC standardization process, which returns void
. As such, almost all of the code in liboqs
was written with this function signature in mind.
Modifying keypair
/ encaps
/ etc to error-check randombytes
would require quite a bit of maintenance on our part. We typically pull in this code from upstream sources (e.g., PQClean / pq-crystals) which would (presumably) still be using the NIST-provided randombytes
API. Hence, we would have to patch the code and re-patch every time we need to pull in an upstream update.
I suppose that it might not be too difficult to change the OQS_randombytes
signature to return a status code and just ignore it in keypair
/ encaps
/ etc. while allowing developers to pass an error-checked implementation to OQS_randombytes_custom_algorithm
. This would also enable us to add better error handling in the future.
It would be great to hear a little more about your use case and if that latter option would be helpful for you. (And if it would be helpful enough, please feel free to submit a PR proposing the change!)
Thanks for the explanation! It makes sense (but still is a bit unfortunate).
My use cases are mainly hypothetical at this time, but I am thinking of cases where a long-running system may for example temporarily run out of good entropy. In this case I would expect the cryptographic operations to fail, so that the calling code could remedy the situation (the obvious workaround here is to let the custom randombytes function handle it, which may or may not be a good idea) or shut down the system gracefully.
I'm not sure it would be a good idea to let OQS_randombytes
return a status code if it is not checked by the callers. It might be dangerous if it could return bad randomness without anyone noticing. In cases where we want to use a function with a different signature, it is probably more safe to use a wrapper, similar to OQS_randombytes_openssl
.
I'm not sure it would be a good idea to let OQS_randombytes return a status code if it is not checked by the callers. It might be dangerous if it could return bad randomness without anyone noticing.
Absolutely agreed: All functions based on this (in upstream-imported algorithms) must stop working if entropy is bad.
In cases where we want to use a function with a different signature, it is probably more safe to use a wrapper, similar to OQS_randombytes_openssl.
Also agreed, but maybe thinking in reverse: What about adding such (result-returning) call to the public liboqs
API and gradually introducing (or patching it) it to "LTS" code, e.g., the ML-* algorithms? The current (void) call could be a wrapper to this (still just exiting on bad retval)?
What about adding such (result-returning) call to the public liboqs API and gradually introducing (or patching it) it to "LTS" code, e.g., the ML-* algorithms? The current (void) call could be a wrapper to this (still just exiting on bad retval)?
Ah, yes, I see. That sounds like a good plan!
Ah, yes, I see. That sounds like a good plan!
PR welcome :-) Time permitting, of course.
The current signature of
OQS_Randombytes()
is to returnvoid
. This might be a limiting factor, which is also noted inOQS_randombytes_openssl()
with the following comment:and the failure handling of this function is therefore to terminate the execution via
exit()
.In cases where we supply our own randomness algorithm via
OQS_randombytes_custom_algorithm()
, the same problem applies. We have no other means of handling a failure than forcefully exiting. It is not unthinkable that the higher level code would want to take care of this situation in a custom way without having to resort to using an exit handler.Would it be feasible to change this signature to returning a status value, which can then be propagated through the higher level functions? For example, the keypair and encaps functions already return an
OQS_STATUS
value, which can be taken care of in the calling code. Wouldn't it make sense if OQS_ERROR was returned if an underlying randomness call failed?