Typo/Logic Error in OQS_SIG_STFL_secret_key_new for LMS configurations

[elGoonie]

Description

Description:

There appears to be a typo or a copy-paste error in the OQS_SIG_STFL_secret_key_new function within liboqs/src/sig_stfl/sig_stfl.c. Specifically, the #ifdef checks do not align with the method_name being compared, leading to potentially incorrect or unexpected behavior where a module's availability is checked against a different, unrelated module.

Affected File: liboqs/src/sig_stfl/sig_stfl.c

Details: Line 1419: When method_name is OQS_SIG_STFL_alg_lms_sha256_h25_w8, the code checks for OQS_ENABLE_SIG_STFL_lms_sha256_h25_w4. This looks like a mismatch.

Expected behavior: If method_name is ...h25_w8, the corresponding #ifdef should check for OQS_ENABLE_SIG_STFL_lms_sha256_h25_w8.

Line 1469ff: Similarly, when method_name is OQS_SIG_STFL_alg_lms_sha256_h15_w8_h10_w8, the code checks for OQS_ENABLE_SIG_STFL_lms_sha256_h10_w8_h10_w8. This is also a mismatch.

Expected behavior: If method_name is ...h15_w8_h10_w8, the corresponding #ifdef should check for OQS_ENABLE_SIG_STFL_lms_sha256_h15_w8_h10_w8.

Proposed Fix: The #ifdef conditions should be updated to match the method_name being evaluated in the strcasecmp call.

Note: I've identified these specific issues but currently lack the time to implement the fix myself. Furthermore, due to the nature of these copy-paste-like errors, it cannot be ruled out that similar mismatches exist in other parts of the codebase. A thorough review of similar logic blocks might be beneficial. I'm providing this bug report in the hope that someone else from the community can address it.

Expected behaviour

No response

liboqs version

main

Environment

No response

Use of generative AI

No response

Additional information

No response

[xuganyu96]

Hi @elGoonie, thank you very much for pointing out the bug. I will take a look and update this issue with my progress.

[xuganyu96]

Double-checking sig_stfl.c's pre-processing macro mismatch:

import os
import re

sig_stfl_c_path = os.path.join(os.environ["LIBOQS_DIR"], "src", "sig_stfl", "sig_stfl.c")
with open(sig_stfl_c_path, "r", encoding="utf-8") as f:
    sig_stfl_c = f.read().splitlines()

# OQS_SIG_STFL_alg_is_enabled
funcstart, funcstop = 118, 559
loc = funcstart
while loc < funcstop:
    line = sig_stfl_c[loc]
    if "strcasecmp" in line:
        else_if_strcasecmp = line
        result = re.search(r"OQS_SIG_STFL_alg_(?P<algname>\w+)", else_if_strcasecmp)
        assert result, "Cannot match OQS_SIG_STFL_alg_xxx"
        param1 = result.group("algname").lower()

        ifdef_macro = sig_stfl_c[loc + 1]
        result = re.search(r"OQS_ENABLE_SIG_STFL_(?P<algname>\w+)", ifdef_macro)
        assert result, "Cannot match OQS_ENABLE_SIG_STFL_xxx"
        param2 = result.group("algname").lower()

        if not (param1 == param2):
            print(f"{loc+1:<4}: {else_if_strcasecmp}")
            print(f"{loc+2:<4}: {ifdef_macro}")
        loc += 2
    else:
        loc += 1

# OQS_SIG_STFL_new
funcstart, funcstop = 560, 995
loc = funcstart
while loc < funcstop:
    line = sig_stfl_c[loc]
    if "strcasecmp" in line:
        else_if_strcasecmp = line
        result = re.search(r"OQS_SIG_STFL_alg_(?P<algname>\w+)", else_if_strcasecmp)
        assert result, "Cannot match OQS_SIG_STFL_alg_xxx"
        param1 = result.group("algname").lower()

        ifdef_macro = sig_stfl_c[loc + 1]
        result = re.search(r"OQS_ENABLE_SIG_STFL_(?P<algname>\w+)", ifdef_macro)
        assert result, "Cannot match OQS_ENABLE_SIG_STFL_xxx"
        param2 = result.group("algname").lower()

        return_new = sig_stfl_c[loc + 2]
        result = re.search(r"OQS_SIG_STFL_alg_(?P<algname>\w+)_new", return_new)
        assert result, "Cannot match OQS_SIG_STFL_alg__xxx_new"
        param3 = result.group("algname").lower()

        if not (param1 == param2 == param3):
            print(f"{loc+1:<4}: {else_if_strcasecmp}")
            print(f"{loc+2:<4}: {ifdef_macro}")
            print(f"{loc+3:<4}: {return_new}")

        loc += 3
    else:
        loc += 1

# OQS_SIG_STFL_SECRET_KEY_new
funcstart, funcstop = 1075, 1510
loc = funcstart
while loc < funcstop:
    line = sig_stfl_c[loc]
    if "strcasecmp" in line:
        else_if_strcasecmp = line
        result = re.search(r"OQS_SIG_STFL_alg_(?P<algname>\w+)", else_if_strcasecmp)
        assert result, "Cannot match OQS_SIG_STFL_alg_xxx"
        param1 = result.group("algname").lower()

        ifdef_macro = sig_stfl_c[loc + 1]
        result = re.search(r"OQS_ENABLE_SIG_STFL_(?P<algname>\w+)", ifdef_macro)
        assert result, "Cannot match OQS_ENABLE_SIG_STFL_xxx"
        param2 = result.group("algname").lower()

        return_new = sig_stfl_c[loc + 2]
        result = re.search(r"OQS_SECRET_KEY_(?P<algname>\w+)_new", return_new)
        assert result, "Cannot match OQS_SECRET_KEY_xxx_new"
        param3 = result.group("algname").lower()

        if not (param1 == param2 == param3):
            print(f"{loc+1:<4}: {else_if_strcasecmp}")
            print(f"{loc+2:<4}: {ifdef_macro}")
            print(f"{loc+3:<4}: {return_new}")

        loc += 3
    else:
        loc += 1

Found the following output:

536 : 	} else if (0 == strcasecmp(method_name, OQS_SIG_STFL_alg_lms_sha256_h20_w8_h10_w8)) {
537 : #ifdef OQS_ENABLE_SIG_STFL_lms_sha256_h5_w1
542 : 	} else if (0 == strcasecmp(method_name, OQS_SIG_STFL_alg_lms_sha256_h20_w8_h15_w8)) {
543 : #ifdef OQS_ENABLE_SIG_STFL_lms_sha256_h5_w1
548 : 	} else if (0 == strcasecmp(method_name, OQS_SIG_STFL_alg_lms_sha256_h20_w8_h20_w8)) {
549 : #ifdef OQS_ENABLE_SIG_STFL_lms_sha256_h5_w1
1419: 	} else if (0 == strcasecmp(method_name, OQS_SIG_STFL_alg_lms_sha256_h25_w8)) {
1420: #ifdef OQS_ENABLE_SIG_STFL_lms_sha256_h25_w4
1421: 		return OQS_SECRET_KEY_LMS_SHA256_H25_W8_new();
1469: 	} else if (0 == strcasecmp(method_name, OQS_SIG_STFL_alg_lms_sha256_h15_w8_h10_w8)) {
1470: #ifdef OQS_ENABLE_SIG_STFL_lms_sha256_h10_w8_h10_w8
1471: 		return OQS_SECRET_KEY_LMS_SHA256_H15_W8_H10_W8_new();

[baentsch]

Seeing these many sig_stfl issues recently, can I ask whether it would make sense to add some interop tests with another implementation to avoid this from happening again? And thinking this through further, if there is another reliable implementation for interop testing (at least for LMS, openssl seems to be an alternative, no?) how much effort should OQS put into this algorithm family? What makes this code superior to other code bases? @elGoonie can I ask what made you select this implementation for your project? Anyone else (known to) using this alg family using liboqs? In which OQS sub projects is sig_stfl integrated and could those help with unearthing interop problems without having to write specific tests?

[ashman-p]

Seeing these many sig_stfl issues recently, can I ask whether it would make sense to add some interop tests with another implementation to avoid this from happening again? And thinking this through further, if there is another reliable implementation for interop testing (at least for LMS, openssl seems to be an alternative, no?) how much effort should OQS put into this algorithm family? What makes this code superior to other code bases? @elGoonie can I ask what made you select this implementation for your project? Anyone else (known to) using this alg family using liboqs? In which OQS sub projects is sig_stfl integrated and could those help with unearthing interop problems without having to write specific tests?

Definitely a typo in the definition of the macros. There are at least 2 reasons for the escape...

1. Each individual macro was not enabled each one by one, in isolation of the others, with a check for availability of the corresponding LMS variant.
2. Missed during code review. With LMS family enabled, all variants were enabled and available.

With the entire LMS family enabled, interop tests would not uncover the issue caused by the typo.

Lastly, openssl does not support LMS key generation. I believe Wolf and BC does.

[elGoonie]

Seeing these many sig_stfl issues recently, can I ask whether it would make sense to add some interop tests with another implementation to avoid this from happening again? And thinking this through further, if there is another reliable implementation for interop testing (at least for LMS, openssl seems to be an alternative, no?) how much effort should OQS put into this algorithm family? What makes this code superior to other code bases? @elGoonie can I ask what made you select this implementation for your project? Anyone else (known to) using this alg family using liboqs? In which OQS sub projects is sig_stfl integrated and could those help with unearthing interop problems without having to write specific tests?

Hello @baentsch , I have been using liboqs for a good 1.5 years to test various PQC methods on a wide range of hardware. Since I am currently interested in LMS/XMSS and have automated my benchmarks, including evaluation, via liboqs, I wanted to see if the benchmarks now work after fixing my first issue #2121 with LMS.

[ashman-p]

@elGoonie, @xuganyu96 , thanks for the reports finding and confirming the problem. @xuganyu96, if you like you can assign the issue to me unless you want to handle it. Thanks.

[xuganyu96]

@elGoonie, @xuganyu96 , thanks for the reports finding and confirming the problem. @xuganyu96, if you like you can assign the issue to me unless you want to handle it. Thanks.

Thanks for taking on the issue. I've assigned the issue to you @ashman-p.