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?

[L-series]

@hanno-becker

Thanks for the feedback! I completely agree with your approach, using a custom randombytes implementation is cleaner than having a compile-time config option for what is essentially external dependency testing. The dry-run strategy to automatically discover and test all randombytes call sites is likely more maintainable.

I'll proceed to:

• Remove MLD_CONFIG_RANDOMBYTES_FAILURE_TEST from the config
  and create test/test_rng_fail.c with the custom randombytes implementation
• Implementing the N-runs approach you described

[hanno-becker]

@L-series Please see the recently merged allocation test pq-code-package/mlkem-native#1424 in mlkem-native (WIP PR for mldsa-native is #810) for how to approach the rng-failure test -- we should strive to have this uniform.

[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.