Skip to content

Conversation

@L-series
Copy link
Contributor

@L-series L-series commented Nov 12, 2025

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.

@L-series L-series marked this pull request as ready for review November 12, 2025 16:02
@L-series L-series requested a review from a team as a code owner November 12, 2025 16:02
@hanno-becker
Copy link
Contributor

hanno-becker commented Nov 12, 2025

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
Copy link
Contributor Author

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.

Change randombytes() to return int (0 on success, non-zero on failure)
instead of void, allowing callers to detect and handle RNG failures.

Updated function signature, all call sites to check return
values and test files to use CHECK macro.

Signed-off-by: Andreas Hatziiliou <[email protected]>
Run the autogen script to reflect the changes made to the randombytes()
API.

Signed-off-by: Andreas Hatziiliou <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants