API: add failure mode support for randombytes()

[L-series]

This PR adds failure mode support for the randombytes() interface.

Marking as draft as there are a few points for which I need clarifications. These are the following:

1. What is to be done regarding the OQS_randombytes API integration? From what I understand this is defined as having return type void on the liboqs side. Do we make a PR to change this on their end also or do we simply modify the return type of our inline mld_randombytes functions defined in files integration/liboqs/config_*.h?
2. Is it appropriate that I am adding the CHECK(x) macro to bench/test_components_mldsa.c?
3. Should I add a test which verifies the behavior in case randombytes() fails?

As an aside. It could be a good idea to factor our the CHECK macro into its own test header file as the code is currently duplicated in many files across the project. @hanno-becker @mkannwischer please let me know your thoughts.

[hanno-becker]

Thank you @L-series!

What is to be done regarding the OQS_randombytes API integration? From what I understand this is defined as having return type void on the liboqs side. Do we make a PR to change this on their end also or do we simply modify the return type of our inline mld_randombytes functions defined in files integration/liboqs/config_*.h?

So long as OQS' randombytes has return type void, we would merely need to change the wrapper to

#define MLD_CONFIG_CUSTOM_RANDOMBYTES
#if !defined(__ASSEMBLER__)
#include <oqs/rand.h>
#include <stdint.h>
#include "../../mldsa/src/sys.h"
static MLD_INLINE int mld_randombytes(uint8_t *ptr, size_t len)
{
  OQS_randombytes(ptr, len);
  return 0;
}

or am I missing something?

Is it appropriate that I am adding the CHECK(x) macro to bench/test_components_mldsa.c?

Yes, I see no problem with that.

Should I add a test which verifies the behavior in case randombytes() fails?

The CBMC proofs will need to cover the new behavior, but yes, we also need have specific tests that trigger failure of randombytes() at various points. This will require a custom configuration and a dedicated test; it's not clear on first sight where this should live. It does not fit into the existing tests in tests/*, nor would it make sense to add a new test class (alongside func, KAT, ACVP, stack, bench) for just this. I tend towards a custom 'example', despite being more of a test than an example -- but that line is blurry anyway.

[L-series]

or am I missing something?

No, this is how I implemented it, it does seem like the best solution currently - however it feels slightly wrong.

The CBMC proofs will need to cover the new behavior

I'll look into modifying the proofs and adding a testing example.

[hanno-becker]

Hey @L-series! How are you getting on? Let us know if you need any input. Otherwise, can you mark the PR as draft until it's ready for review?

[L-series]

Hey @hanno-becker, sorry for the delay. Haven't had much time outside of work recently. Will mark as a draft till I fix the failing CI tests.

[L-series]

I've added a test case as well as fixed the failing CI tests. Please let me know what needs to be modified regarding the proofs as I'm not sure what to search for.

[mkannwischer]

Thanks @L-series.

Could you please rebase on top of main - there has been some restructuring to the config.h?

ALso could you please add the new failure cases to the documentation of the top level functions in sign.h and mldsa_native.h?

[hanno-becker]

Thank you @L-series for pushing this, I think this will be a valuable addition!

I don't think we should have a MLD_CONFIG_RANDOMBYTES_FAILURE_TEST option. I agree that there is some similarity to the PCT breakage test, but the difference here is that randombytes is externally controlled, so an injection of error can happen through a dedicated implementation of randombytes and need not happen from the mldsa-native source.

I think we should remove MLD_CONFIG_RANDOMBYTES_FAILURE_TEST and instead have a separate test file test/test_rng_fail.c or similar, which uses a custom randombytes implementation injecting failures at various points. One way this could be achieved is by first doing a 'dry-run' of ML-DSA API from test_rng_fail.c and having the instrumented randombytes implementation merely count (in a global variable) how many times it has been invoked. And then, say there have been N invocations of randombytes, test_rng_fail.c runs the standard tests again N times, and in the i-th invocation, randombytes is configured to fail at the i-th invocation, but working beforehand. This way, we can trigger the failure handling for all invocations of randombytes.

What do you think?

[mkannwischer]

Thanks @L-series. The test looks much better to me now.
Please also add it to scripts/tests - otherwise large parts of the CI don't actually run it.

Do you want some help with the CBMC proofs? I can take a look later.

[L-series]

Added the two missing CBMC proofs and added the test to scripts/tests. Please let me know if i should follow what is done in 34281c3 regarding the integration with CI. I saw that the alloc test was disabled in certain of the CI actions and was wondering if I should do the same here.

[hanno-becker]

Thank you very much @L-series, mostly looks good! Some smaller comments.

[mkannwischer]

Thanks @L-series. We are almost there. A few small nits, then I am happy with this PR.

[mkannwischer]

Please also clean-up the commit history. autogen should never be run in a separate commit (the goal is that each individual commit can pass CI - we don't always succeed with this, but running autogen separately guarantees that the first commit won't pass CI).

[L-series]

@mkannwischer Cleaned up the code in accordance with your comments and I fixed the commit history to not include an autogen commit. Willl run autogen on a per-commit basis from now onwards!

[mkannwischer]

Thanks @L-series for the persistence and changes. This looks excellent to me now

@hanno-becker, please take another look.

[hanno-becker]

Do we need a custom build here? Can we not use the default build but just link against the customized randombytes?

[hanno-becker]

Same comment here: I'm not sure we need a separate build.

[hanno-becker]

Looks great, thanks a lot @L-series

The only thing that should be improved is the build: We don't need a separate build for the fail-RNG test, but can just link a different randombytes. This will avoid the build time for make test creep up even further.

I leave it to you whether you want to do this now or as a follow-up.

[mkannwischer]

Looks great, thanks a lot @L-series

The only thing that should be improved is the build: We don't need a separate build for the fail-RNG test, but can just link a different randombytes. This will avoid the build time for make test creep up even further.

I leave it to you whether you want to do this now or as a follow-up.

@L-series could you do that as a follow-up, please? Then we can already get the restructuring PRs going without conflicting with this PR.