[owaldron]

This PR integrates SDitH's round 3 code into liboqs (#2453). The source upstream repo is at sdith/sdith, which was forked at owaldron:feat/liboqs-integration to house upstream changes required for integration (git diff).

The changes to the upstream repo are:

• bbc96a6: Portability fixes for liboqs integration
• b711691: liboqs metadata and wrapper API (no glue code)
• a0e7be2: FIPS202 shim onto liboqs' SHA3
• e47fad2: AES shim onto liboqs' AES, using the new rekey API
• 7e68c20: Symbol namespacing (gen_namespace.sh + generated header)

And the changes to liboqs are:

• acc8cdfb: Add OQS_AES128_ECB_rekey to the AES API, with KAT
• db18c554: copy_from_upstream: nested common dependencies
• ca71f231: (Generated) SDitH3 import via copy_from_upstream
• ddaa6dd0: Platform gating and -Werror exemption (Address -DOQS_STRICT_WARNINGS=ON warnings for SDitH code #2588)
• fc8461e5: Constant-time suppressions (Verify SDitH3 test_constant_time suppressions #2609)
• 49e261c6: Zephyr configuration
• c6b3bc4e: Algorithm documentation

---

• Does this PR change the input/output behaviour of a cryptographic algorithm (i.e., does it change known answer test values)? (If so, a version bump will be required from x.y.z to x.(y+1).0.)
• Does this PR change the list of algorithms available -- either adding, removing, or renaming? Does this PR otherwise change an API? (If so, PRs in fully supported downstream projects dependent on these, i.e., oqs-provider will also need to be ready for review and merge by the time this is merged. Also, make sure to update the list of algorithms in the continuous benchmarking files: .github/workflows/kem-bench.yml and sig-bench.yml)

---

Generative AI Disclosure: generative AI was used throughout the development process to aid with understanding, drafting, debugging, and reviewing code. Any and all code generated by AI was reviewed and possibly modified before inclusion. Non-trivial contributions by generative AI are accompanied by an Assisted-by tag in the commit.

[coveralls]

coverage: 80.542% (-2.1%) from 82.638% — owaldron:feat/sdith-integration into open-quantum-safe:main

[RodriM11]

hi @owaldron ! If the OR is ready for review, should we mark it as so? so people know and can start to go through it, even though they are certain build failures that we have to solve.

[owaldron]

Hi @RodriM11, yup! I just marked it as ready for review

[RodriM11]

@Frauschi 4 out of the remaining 5 build errors are related to Zephyr, would you mind taking a quick look to see if anything is amiss?

[owaldron]

For the timed-out noble-memopt test, I opened a PR #2595 that should speedup CI enough to avoid needing to arise the timeout cap.

[Frauschi]

I dug into the failing Zephyr jobs. There are two problems stacked on top of each other. Only cortex-a53 and riscv64 fail because SDitH is 64-bit only, and the KEMs sample breaks too because the Zephyr module has no switch for SDitH, so it gets built into every sample.

1. typeof doesn't build under Zephyr

Zephyr builds with -std=c17 instead of gnu11, so the GNU typeof keyword in the MAP_FIELD macro isn't available:

sdith_signature.c:191:17: error: implicit declaration of function 'typeof'

Changing it to __typeof__ fixes the build on both targets. Since the file comes in through copy_from_upstream, the fix probably has to be handled there somehow.

2. Signing needs way more heap than the sample has

With that fixed, the Signatures sample fails at runtime with OQS_SIG_sign failed on the first SDitH variant. crypto_sign_sign() mallocs its scratch space in one go, and the sample only has a 256 KB heap. On a host build I measured roughly 0.34 MB for L1-fast, 1 MB for L1-short and up to 4 MB for L5-short, so none of them fit.

Suggestion

I'd add a Kconfig switch in zephyr/Kconfig that's off by default and limited to 64-bit, like we already do for UOV:

config LIBOQS_ENABLE_SIG_SDITH
	bool "Enable the SDitH signature algorithm"
	default n
	depends on LIBOQS
	depends on 64BIT

plus the usual block in zephyr/CMakeLists.txt:

        if(CONFIG_LIBOQS_ENABLE_SIG_SDITH)
                set(OQS_ENABLE_SIG_SDITH ON)
        else()
                set(OQS_ENABLE_SIG_SDITH OFF)
        endif()

That should get the Zephyr jobs green.

If you want SDitH covered in CI too, add boards/qemu_cortex_a53.conf and boards/qemu_riscv64.conf overlays to the Signatures sample that turn on CONFIG_LIBOQS_ENABLE_SIG_SDITH and raise CONFIG_COMMON_LIBC_MALLOC_ARENA_SIZE to at least 4 MB, which keeps the 32-bit boards on their current 256 KB heap.

[xuganyu96]

20260928 Code review with Owen

Transparent AES key schedule memory layout

SDitH treats AES key schedule struct as transprent
(example),
which makes shimming AES key expansion difficult.

There are different level of shimming we could do:

• Do not shim at all
• Implement in liboqs a 4x key expand API and a 4x encrypt API
• Implement AES/Rijndael-256 PRNG

Explore the difficulty of doing each option.

OSSL shimming

With respect to providing an alternative API for writing AES key schedule to
user-provided buffer, the OpenSSL backend presents a problem where
OSSL_FUNC(EVP_CIPHER_CTX_new())
will allocate space on its own, and we as users of OpenSSL has not control over
it. This means that liboqs, instead of liboqs user, is responsible for
releasing this memory. This can be partially remediated with a global state
that keeps track of memory allocated by OpenSSL calls, then release them at
OQS_destroy, but maintaining global state feels uncomfortable.

[owaldron]

Hi @RodriM11, I've finished made the necessary changes to fix the CI errors and shim AES. It looks like CI is passing now! As per @dstebila's recommendation, I've also restructured the commits so it should be easier to review too

[baentsch]

Thanks for this work @owaldron ! Some caveats/questions/improvement proposals (see separate comments) but I'm approving as this is a much better integration machinery than what we have for other algorithms (and CI passes :).

[baentsch]

On this I've got a question: Does this file define liboqs as the entity responsible for this crypto? There are companies using S/CBOMs as a way to document their products have a "maintained" software stack and I'd rather not have their products containing SDitH point at us but rather at the upstream. That concern may not be SDitH specific, so tagging @bhess for advice as the original adder of this bookkeeping facility (hoping I remember this right?).

[baentsch]

This reads very weird: a) is this true? b) This at most is a moment-in-time statement changing whenever OpenSSL source code changes.

[owaldron]

I admit that this is weird, and is one of my biggest points of uncertainty with this PR.

a)
This option was suggested to me by Claude when brainstorming, but I've looked into it enough to convince myself that it is true. EVP_EncryptInit_ex wraps EVP_CipherInit_ex which itself wraps evp_cipher_init_internal. Within this function, the reset calls are all guarded with cipher != NULL (source):

    /* Ensure a context left lying around from last time is cleared */
    if (cipher != NULL && ctx->cipher != NULL) {
        unsigned long flags = ctx->flags;


        EVP_CIPHER_CTX_reset(ctx);
        /* Restore encrypt and flags */
        ctx->encrypt = enc;
        ctx->flags = flags;
    }

The best evidence that this is safe behaviour is the unit test test_evp_reset which shows that passing NULL to all but the key slot re-keys the ctx (source):

    if (!TEST_true(EVP_CipherInit_ex(ctx, type, NULL, NULL, NULL, t->initenc))) {
        errmsg = "EMPTY_ENC_INIT";
        goto err;
    }
    if (!TEST_true(EVP_CIPHER_CTX_set_padding(ctx, 0))) {
        errmsg = "PADDING";
        goto err;
    }
    if (t->keyfirst && !TEST_true(EVP_CipherInit_ex(ctx, NULL, NULL, t->key, NULL, -1))) {
        errmsg = "KEY_INIT (before iv)";
        goto err;
    }
    if (!evp_init_seq_set_iv(ctx, t)) {
        errmsg = "IV_INIT";
        goto err;
    }
    if (t->keyfirst == 0 && !TEST_true(EVP_CipherInit_ex(ctx, NULL, NULL, t->key, NULL, -1))) {
        errmsg = "KEY_INIT (after iv)";
        goto err;
    }

although this test unfortunately doesn't replace the existing key with a new key, nor does it assert that the context isn't free'd & re-allocated.

The lack of documentation on this use case does leave me a bit uneasy, however OpenSSL doesn't seem to document these functions much at all to begin with. The closest thing I could find documenting this behaviour was a (very old) comment regarding similar (but separate) logic in the legacy path of the evp_cipher_init_internal function (source):

    /*
     * Whether it's nice or not, "Inits" can be used on "Final"'d contexts so
     * this context may already have an ENGINE! Try to avoid releasing the
     * previous handle, re-querying for an ENGINE, and having a
     * reinitialisation, when it may all be unnecessary.
     */

b)
If we want to avoid using this feature, or if openssl moves away from supporting this, we can always switch to a fresh ctx init for the OpenSSL backend only. In this case, there is no getting around the free/alloc cost for each re-key for OpenSSL, but we can still reap the performance rewards for the other backends.

[baentsch]

Thanks for the explanation @owaldron ! I had a hunch that it was @claude finding out about this (current state of things). And no contradiction as to OpenSSL's state of documentation :-/

The extremely cautious person in my would go for option b) to avoid "surprises down the road". The more enterprising person in me would use this (e.g., sub-optimal performance numbers) to touch base with the OpenSSL community to either commit on the a) behaviour (and document it) or confirm b) to be the safe choice. Your call as to how many folks you'd like to interact with. I can recommend it: The OpenSSL community is surely interesting, particularly its breadth.

[owaldron]

Good call, I left a question in OpenSSL's GitHub discussions Q&A (number 33169) asking if this is idiomatic and safe behaviour. Hopefully the community can help give some guidance on whether this use of EVP_CipherInit is supported and if it should be documented or tested anywhere.

[owaldron]

A helpful commenter answered the Q&A post (link) and claims this behaviour is intended, citing the man page as evidence.

Two additional points they brought up, and why I think they're already addressed:

check the return of every init call (a failed init leaves the ctx unusable until you init it again)

We currently wrap our ossl calls with OQS_OPENSSL_GUARD, which raises an error on a bad return code. Although, a comment does call this macro "a temporary workaround"... I assume this refers to #1456's aim to return a status code instead of raising an error.

on 3.x you might as well switch to EVP_EncryptInit_ex2 since the non-2 ones are marked legacy now

I think we continue to support OpenSSL 1.1.1 (?), so it doesn't yet make sense to switch to _ex2. This migration would follow a complete 1.1.1 deprication.

[baentsch]

This migration would follow a complete 1.1.1 deprication.

That actually is an issue worth while raising (imo executing on) given OpenSSL 1.1.1 has reached end of life 3 years ago.

[baentsch]

I assume all of this is lifted out of SDitH original code? Otherwise, this would warrant a serious review (that I'd not be competent enough for). General question: What will happen with this common code as/if SDitH does some logic/security changes upstream in this space? Will they be transferred here or will this code drift from the upstream?

[owaldron]

Yup, (other than the added #include) this is copied right from the sdith source (here).

In the ideal case, we should just have to update the commit hash in copy_from_upstream.yml and run copy_from_upstream to keep all of the sdith code up-to-date. Right now, since this points to a fork, there's the extra step of syncing the changes too. I think that the biggest issue with this is that upstream maintainers would have to notify us when changes occur, or come here to run the script themselves.

If PRs like #2594 continue to close the performance gap, perhaps we can consider shimming this optimized AES backend as well. For now though (even with this PR), I think that the upstream's optimized AES implementation is better suited for their use case than the liboqs API.

[baentsch]

come here to run the script themselves.

That would be the perfect end state: I we (you!) make it simple for the upstream to keep their code in sync with liboqs the project achieves what I like to call PMM (perfect mutual motivation): Users get an up-to-date implementation (maybe also fielding security or other usage concerns that way); the upstream gets user feedback to improve their creation; OQS facilitates this without picking up undue maintenance load.

I think that the upstream's optimized AES implementation is better suited for their use case than the liboqs API.

Sounds sensible, too. OQS shouldn't prescribe things to algorithms that weaken them. To the contrary, if an upstream can make a good case for a better "common code" base, OQS should consider adopting it, maybe also for other algs needing that code.

[owaldron]

That would be the perfect end state: I we (you!) make it simple for the upstream to keep their code in sync with liboqs the project achieves what I like to call PMM

Sound good! I'll look into opening a PR upstream soon to eliminate the one level of indirection for the maintainers.

OQS should consider adopting it, maybe also for other algs needing that code.

Given that MQOM, SDitH, and FAEST all use AES for similar purposes within the VOLE-in-the-Head framework, I've started to explore whether some enhancement to liboqs' common code could be leveraged by all three algorithms. I have yet to try anything yet, though

[owaldron]

Hi @baentsch. Thank you for reviewing this so quickly! I'll work now to address your comments

[xuganyu96]

liboqs's AES module provides a set of AES-related APIs. There are two primary
sets of backends: a native implementation and an OpenSSL implementation. The
two backends are respectively defined as callback structs in src/common/aes/aes_c.c
and src/common/aes/aes_ossl.c. At compile-time, depending on whether
OQS_USE_AES_OPENSSL is turned on, CMake will choose to compile aes_c.c or
aes_ossl.c.

The addition of a rekey API to liboqs's AES module is easy to implement in with
aes_c.c, but the OpenSSL backend is trickier to work with. OpenSSL 3.3's
documentation
lists the following example:

ctx = EVP_CIPHER_CTX_new();
if (!EVP_EncryptInit_ex2(ctx, EVP_idea_cbc(), key, iv, NULL)) {
    /* Error */
    EVP_CIPHER_CTX_free(ctx);
    return 0;
}

Where EVP_EncryptInit_ex/EVP_EncryptInit_ex2 takes the input encryption key
and updates the context, so subsequent calls to something like EVP_EncryptUpdate
will use the updated encryption key and its corresponding round keys. We cannot
find official documentation promising that "if certain parameters are passed in
as NULL, then EVP_EncryptInit_ex will refresh round keys without freeing the
existing context and allocating for a new context". There are only evidence in
the source code, in the unit tests, and in some discussion comments.

It's probably a better idea to stick with more officially documented OpenSSL
behaviors.

With untested behavior:

static void AES128_ECB_rekey(const uint8_t *key, void *schedule) {
	struct key_schedule *ks = (struct key_schedule *) schedule;
	OQS_OPENSSL_GUARD(OSSL_FUNC(EVP_EncryptInit_ex)(ks->ctx, NULL, NULL, key, NULL));
}

With safer behavior:

static void AES128_ECB_rekey(const uint8_t *key, void *schedule) {
	struct key_schedule *ks = (struct key_schedule *) schedule;
  OSSL_FUNC(EVP_CIPHER_CTX_free)(ks->ctx);
  ks->ctx = OSSL_FUNC(EVP_CIPHER_CTX_new)();
  OQS_EXIT_IF_NULLPTR(ks->ctx, "OpenSSL");
  OQS_OPENSSL_GUARD(OSSL_FUNC(EVP_EncryptInit_ex)(ks->ctx, oqs_aes_128_ecb(), NULL, key, NULL));
}

[xuganyu96]

if oqs_aes128_rekey_ni is exactly oqs_aes128_load_schedule_into_ni, then there is no need to declare oqs_aes128_load_schedule_into_ni separately.

[xuganyu96]

Does OQS_MEM_malloc check for null pointer? If not, there should be a null pointer check right afterwards.

[baentsch]

Does OQS_MEM_malloc check for null pointer? If not, there should be a null pointer check right afterwards.

Good question. And there's even a macro for that (OQS_EXIT_IF_NULLPTR). Additional note to committers with too much time (or a willing AI :) at their hands: It may be worth while asking this question for the whole code base. And at least my AI tells me there's "room for improvement" (separate issue? quick PR?)

[owaldron]

Had some spare credits, so I let Claude whirl away at this quickly: #2633

[rben-dev]

Hi,

From my experience as a developer of MQOM and integrating it into liboqs:

• The AES rekeying API (avoiding spurious mallocs/frees) is interesting, but in MQOM from my benchmarks on an AVX2 with AES-NI machine this has almost no impact in (MQOM's) GGM trees implementation (0 to 1% performance gain for the whole signature).
• The AES x2 / x4 / x8 APIs (discussion SDitH3 Integration #2586 (comment)) that encrypt blocks under different keys are useful in the GGM context, and would indeed be beneficial for the three MPCitH based candidates. During MQOM's integration, we have accepted to "sacrifice" some performance while conforming to the OQS AES API. On the same AVX2 with AES-NI AMD machine, the performance loss on the whole signature for MQOM variants with heavy usage of GGMs is between 10% to 15%.

Having said that, beware that reusing / integrating https://github.com/sdith/sdith/blob/ce7adf06476674d4ff990f378f83f9981cd6745b/lib/aes/aes128_ctrle_special_avx2.c APIs "as is" is somehow specific to SDitH:

• The key schedule APIs are generic.
• But the encryption APIs suppose a counter mode, which is not the case in MQOM.
• For FAEST, the counter mode is used but with a carryless incrementation performed over different counter sizes, so adaptations must be performed.

I guess a useful common ground generic API would be:

• A x2/x4/x8 key schedule API
• A x2/x4/x8 ECB encryption and decryption API that deals with 2, 4 or 8 different key contexts

For CTR based schemes such as FAEST and SDitH, the "counters" would be prepared by the scheme before calling the OQS AES API.

Regards,

[owaldron]

Hi @rben-dev, thank you so much for your advice!

in MQOM from my benchmarks on an AVX2 with AES-NI machine this has almost no impact in (MQOM's) GGM trees implementation (0 to 1% performance gain for the whole signature)

You're right, I just ran my own benchmarks for sdith on ARMv8 and AES-NI (without AVX2) and the speedup from rekeying is 0 to 2%. The exception is a 28% speedup when liboqs uses the OpenSSL backend, but as discussed above (here and here) we aren't certain this implementation is safe anyways. I played a bit with @claude and it showed that (if several complimentary code changes are made) rekeying could lead to a 5% speedup across all backends, but for now it seems like a premature optimization that perhaps might be best explored after the integration.

During MQOM's integration, we have accepted to "sacrifice" some performance while conforming to the OQS AES API.

That's fair, but since I'm not on the SDitH team I did not want to make this decision on their behalf. Is MQOM's 10% to 15% performance loss between its standalone performance and it's liboqs integration? As you pointed out, SDitH interweaves the logic for their custom litte-endian CTR into their x2/x4/... AES in aes128_ctrle_special_avx2.c, so I feared that trying to work the liboqs API into this would be too invasive and may impact standalone performance. However, if we had the x4,x8,... api you proposed, having SDitH's CTR implemented as a wrapper on top of ECB mode would be pretty straightforward and we could shim this file entirely.

I guess a useful common ground generic API would be:

• A x2/x4/x8 key schedule API
• A x2/x4/x8 ECB encryption and decryption API that deals with 2, 4 or 8 different key contexts

I agree that this is probably the most useful API. It seems like a natural follow up to #2594. I'll look into adding this as a standalone change in a following PR, with part of the goal being to regaining as much of the MQOM performance gap as possible.

[rben-dev]

@owaldron

Thanks for the feedback.

I played a bit with @claude and it showed that (if several complimentary code changes are made) rekeying could lead to a 5% speedup across all backends, but for now it seems like a premature optimization that perhaps might be best explored after the integration.

Seems interesting, and yes if 5% can be achieved that would be great and benefit to all the algorithms using AES.

That's fair, but since I'm not on the SDitH team I did not want to make this decision on their behalf.

This makes sense.

Is MQOM's 10% to 15% performance loss between its standalone performance and it's liboqs integration?

Yes, and this has been measured all things being equal except for the AES backend: MQOM's internal AES-NI backend versus MQOM's API delegating to liboqs backend. Please note that MQOM achieves slightly better performance on the same AVX2 platform thanks to its VAES backend (as far as I know, liboqs does not supprt vaes yet), but for this test I explicitly forced the AES-NI backend to have a fair comparison.

I agree that this is probably the most useful API. It seems like a natural follow up to #2594. I'll look into adding this as a standalone change in a following PR, with part of the goal being to regaining as much of the MQOM performance gap as possible.

Not saying that this is the best way to design this, but on the MQOM side we have tried to split all the low-level stuff in abstraction layers enc.h that multiplexes enc_local.h, enc_liboqs.h and so on. All the x2/x4/x8 handling is delegated to this low-level layer (each low-level backend exposes such APIs, and for those unaccelerated x2/x4/x8 is simply a serialization of the unit operation). This leaves all the high-level operations on GGM trees free to use such APIs without knowing if the backend is accelerated or not, and fallback to "serialized" performance when parallelization is not available.

Of course I understand that liboqs integration is seeking the least possible modifications to the upstream code, but maybe discussing such abstractions (at least for AES and Rijndael) with the SDitH developers could be of interest?

Anyways, thanks for your efforts on integrating SDitH, and trying to smooth other MPCitH candidates performance!

Regards,