ML-DSA/Dilithium: obtain security level from DER when decoding #8177

bigbrett · 2024-11-12T14:32:03Z

Add ability to obtain Dilithium security level (parameters) from a DER encoded key based on the algorithm type OID
Add test coverage for decoding keys without level specified

Description

Similar to #8129, wolfHSM needs to be able to "blindly" decode a DER key without knowing its exact contents. In this case it just knows that an item is an ML-DSA key, and nothing more. This change allows the security level to be set dynamically in the output MlDsaKey struct based on the algorithm OID in the DER file.

Testing

Added new wolfCrypt test coverage for decoding keys without specifying a level

Checklist

added tests
updated/added doxygen
updated appropriate READMEs
Updated manual and documentation

encoded key based on the algorithm type OID - Add test coverage for decoding DER keys without level specified

SparkiDev · 2024-11-12T22:29:52Z

Consider changing call to DecodeAsymKey_Assign() to call DecodeAsymKey_ex(), a new function, which returns the OID.
Need to rework DecodeASymKey_Assign() to call DecodeAsymKey_ex() with an OID.

bigbrett · 2024-11-12T22:49:15Z

Consider changing call to DecodeAsymKey_Assign() to call DecodeAsymKey_ex(), a new function, which returns the OID.
Need to rework DecodeASymKey_Assign() to call DecodeAsymKey_ex() with an OID.

@SparkiDev I had originally thought about modifying DecodeAsymKey_Assign() since it was already parsing these values out, but decided against it since it would make this change extremely intrusive (e.g. touching code called by basically every public key function). Instead I decided to do it this way to localize the addition to just the Dilithium code.

If you would rather see this rolled out globally then I can do that, but perhaps we could add that in a subsequent PR? I'd prefer to just get this in for Dilithium now, so some wolfHSM features depending on this functionality can be completed. I agree that this would be great generic functionality to have though, and would be happy to take a stab at it.

Let me know your thoughts.

SparkiDev · 2024-11-12T23:08:57Z

All the PQC algorithms go through here and they pass in the OID.
Having the OID discovered would be useful to all these algorithms.

bigbrett · 2024-11-13T22:35:35Z

@SparkiDev Okay, spent some time today refactoring the code to pull OID detection into DecodeAsymKey_Assign_ex and DecodeAsymKeyPublic_Assign_ex. Added additional test coverage too.

bigbrett · 2024-11-14T03:57:48Z

Jenkins retest this please

dgarske · 2024-11-15T17:07:14Z

wolfssl/wolfcrypt/asn.h

@@ -2542,6 +2542,10 @@ WOLFSSL_LOCAL int SetAsymKeyDerPublic(const byte* pubKey, word32 pubKeyLen,
 WOLFSSL_LOCAL int DecodeAsymKeyPublic_Assign(const byte* input,
    word32* inOutIdx, word32 inSz, const byte** pubKey, word32* pubKeyLen,
    int keyType);
+WOLFSSL_LOCAL int DecodeAsymKeyPublic_Assign_ex(const byte* input,


Since DecodeAsymKeyPublic_Assign is a local function it is fine with me to change all uses of it... Vs asking another _ex.

Ok, made the change in 2207791. If @SparkiDev insists on the _ex function as he originally suggested we can just revert that commit.

…ed in original _Assign function

dgarske

[quantum-safe-wolfssl-all-clang-tidy] [1 of 1] [2207791aab]
    autogen.sh 2207791aab...   real 0m10.667s  user 0m8.739s  sys 0m1.113s
    configure...   real 0m27.840s  user 0m14.860s  sys 0m15.123s
    build...   real 3m16.297s  user 19m42.864s  sys 0m28.443s
    check...FAIL: scripts/unit.test
   real 0m26.240s  user 0m24.799s  sys 0m9.948s
ERROR - tests/api.c line 34847 failed with:
    expected: wc_Dilithium_PublicKeyDecode(der, &idx, key, pubDerLen) == (BAD_FUNC_ARG)
    result:   -140 != -173
ERROR - tests/api.c line 98894 failed with:
    expected: Test failed
    result:   ret 0
Testing TLSv1_2
Testing DTLSv1_2
 passed (  0.00096)
   1001: test_wolfSSL_Cleanup                                : passed (  0.00000)

FAILURES:
   285: test_wc_dilithium_der

 End API Tests
FAIL scripts/unit.test (exit status: 1)

scripts/unit.log tail:
Testing TLSv1_2
Testing DTLSv1_2
 passed (  0.00096)
   1001: test_wolfSSL_Cleanup                                : passed (  0.00000)

FAILURES:
   285: test_wc_dilithium_der

 End API Tests
FAIL scripts/unit.test (exit status: 1)

check exited with status 2
retry?  [y] n
    quantum-safe-wolfssl-all-clang-tidy fail_check
    failed config: '--srcdir' '.' '--disable-jobserver' '--enable-option-checking=fatal' '--enable-all' '--enable-testcert' '--enable-intelasm' '--enable-sp-asm' '--enable-experimental' '--enable-kyber' '--enable-lms' '--enable-xmss' '--enable-dilithium' '--disable-qt' 'CPPFLAGS=-DWOLFSSL_SP_INT_NEGATIVE -DNO_WOLFSSL_CIPHER_SUITE_TEST -DWOLFSSL_OLD_PRIME_CHECK -pedantic -DSP_ALLOC -DWOLFSSL_CLANG_TIDY' 'CC=/home/davidgarske/GitHub/testing/git-hooks/clang-tidy-builder.sh' 'CLANG=/usr/lib/llvm-18/bin/clang-18' 'CFLAGS=-Wunreachable-code-aggressive -Wthread-safety -Wloop-analysis -Wenum-compare-conditional -fcolor-diagnostics -fcomplete-member-pointers -Wheader-hygiene -Wstring-conversion -Wtautological-overlap-compare -Wno-language-extension-token -Wunreachable-code-break -Wunreachable-code-return -Wimplicit-fallthrough -Wno-language-extension-token'
    BUILD_ENV: 'CLANG_TIDY=/usr/lib/llvm-18/bin/clang-tidy' 'CLANG=/usr/lib/llvm-18/bin/clang-18' 'CLANG_TIDY_EXTRA_ARGS=--use-color=1 --quiet -line-filter=[{"name":"asn1.h","lines":[[1,166]]},{"name":".c"},{"name":".h"},{"name":".s"},{"name":".S"},{"name":".i"}]' 'CLANG_OVERRIDE_CFLAGS=' 'FAIL_BUILD_CODENAME=fail_analytic_build' 'MAX_FIPS_CODE_SZ=10000000'
    EXE: 'run_untrusted_task' 'make' 'EXTRA_CFLAGS=-fdiagnostics-color=always -fmax-errors=20' '--quiet' 'WOLFSSL_OPENSSL_TEST=1' '-j' '42' 'check'
exiting with status 2
results for 2207791aab:
failed: quantum-safe-wolfssl-all-clang-tidy
final tally for 2207791aab with build env de3c7da84a: 0 of 1 check succeeded, 0 skipped, 0 forced, and 1 failed.
stopped at config: 'quantum-safe-wolfssl-all-clang-tidy' '--srcdir' '.' '--disable-jobserver' '--enable-option-checking=fatal' '--enable-all' '--enable-testcert' '--enable-intelasm' '--enable-sp-asm' '--enable-experimental' '--enable-kyber' '--enable-lms' '--enable-xmss' '--enable-dilithium' '--disable-qt' 'CPPFLAGS=-DWOLFSSL_SP_INT_NEGATIVE -DNO_WOLFSSL_CIPHER_SUITE_TEST -DWOLFSSL_OLD_PRIME_CHECK -pedantic -DSP_ALLOC -DWOLFSSL_CLANG_TIDY' 'CC=/home/davidgarske/GitHub/testing/git-hooks/clang-tidy-builder.sh' 'CLANG=/usr/lib/llvm-18/bin/clang-18' 'CFLAGS=-Wunreachable-code-aggressive -Wthread-safety -Wloop-analysis -Wenum-compare-conditional -fcolor-diagnostics -fcomplete-member-pointers -Wheader-hygiene -Wstring-conversion -Wtautological-overlap-compare -Wno-language-extension-token -Wunreachable-code-break -Wunreachable-code-return -Wimplicit-fallthrough -Wno-language-extension-token'

$ cd /tmp/wolfssl_test_workdir.3444126/wolfssl
davidgarske@P700-XEON:/tmp/wolfssl_test_workdir.3444126/wolfssl$ ./tests/unit.test -285
starting unit tests...
 Begin API Tests
   285: test_wc_dilithium_der                               :
ERROR - tests/api.c line 34847 failed with:
    expected: wc_Dilithium_PublicKeyDecode(der, &idx, key, pubDerLen) == (BAD_FUNC_ARG)
    result:   -140 != -173

 failed (  0.00012)

ERROR - tests/api.c line 98894 failed with:
    expected: Test failed

    result:   ret 0


FAILURES:
   285: test_wc_dilithium_der

 End API Tests

wolfcrypt/src/asn.c

wolfcrypt/test/test.c

billphipps

No additional changes. Agree with Sean that the API should be extended rather than changed. A couple of questions but I think this is close.

wolfcrypt/src/dilithium.c

wolfcrypt/test/test.c

bigbrett · 2024-11-19T00:24:45Z

Jenkins retest this please

bigbrett · 2024-11-19T03:39:37Z

Jenkins retest this please

dgarske · 2024-11-20T20:41:38Z

Hi @bigbrett ,

This PR seems to have broken some of the tests. Can you please review the issue and put up a PR?

[quantum-safe-wolfssl-all-g++-latest-debug] [6 of 39] [ef67b1c06a]
    configure...   real 0m24.978s  user 0m14.014s  sys 0m13.168s
    build...   real 0m28.446s  user 1m40.272s  sys 0m10.845s
    check...FAIL: scripts/unit.test
   real 0m19.072s  user 0m33.427s  sys 0m11.238s

scripts/unit.log tail:
 passed (  0.00001)
wolfSSL Entering wolfSSL_ERR_clear_error
   286: test_wc_dilithium_der                               :RNG_HEALTH_TEST_CHECK_SIZE = 128
sizeof(seedB_data)         = 128
opened /dev/urandom.
rnd read...
ERR TRACE: wolfcrypt/src/dilithium.c L 10026 BAD_FUNC_ARG (-173)
ERR TRACE: wolfcrypt/src/dilithium.c L 10026 BAD_FUNC_ARG (-173)
ERR TRACE: wolfcrypt/src/dilithium.c L 9179 BAD_FUNC_ARG (-173)
FAIL scripts/unit.test (exit status: 139)

stopped at config: 'quantum-safe-wolfssl-all-g++-latest-debug' '--srcdir' '.' '--disable-jobserver' '--enable-option-checking=fatal' '--enable-all' '--enable-testcert' '--enable-acert' '--enable-dtls13' '--enable-dtls-mtu' '--enable-dtls-frag-ch' '--enable-quic' '--enable-debug' '--enable-debug-trace-errcodes' '--enable-sp-math-all' '--enable-experimental' '--enable-kyber=all,original' '--enable-lms' '--enable-xmss' '--enable-dilithium' '--enable-dual-alg-certs' '--disable-qt' 'CPPFLAGS=-DNO_WOLFSSL_CIPHER_SUITE_TEST -DWOLFSSL_OLD_PRIME_CHECK' 'CC=g++-13'

bigbrett force-pushed the dilithium-get-algo-from-der branch from c6347bd to dd1ddb7 Compare November 12, 2024 16:06

- Add ability to obtain Dilithium security level (parameters) from a DER

0e6ac11

encoded key based on the algorithm type OID - Add test coverage for decoding DER keys without level specified

bigbrett force-pushed the dilithium-get-algo-from-der branch from dd1ddb7 to 0e6ac11 Compare November 12, 2024 21:19

guard use of dilithium_key->params on WC implementation

6b1b6ec

bigbrett force-pushed the dilithium-get-algo-from-der branch from 26a59b0 to 6b1b6ec Compare November 12, 2024 23:08

- Move automatic OID detection from Dilithium code into ASN code

f672105

bigbrett force-pushed the dilithium-get-algo-from-der branch from 0ad6d4a to f672105 Compare November 13, 2024 22:32

bigbrett assigned SparkiDev Nov 13, 2024

bigbrett assigned wolfSSL-Bot Nov 14, 2024

bigbrett mentioned this pull request Nov 14, 2024

(initial) ML-DSA support wolfSSL/wolfHSM#84

Merged

dgarske requested changes Nov 15, 2024

View reviewed changes

dgarske assigned bigbrett and unassigned SparkiDev Nov 15, 2024

dgarske requested a review from SparkiDev November 15, 2024 17:08

removed DecodeAsymKeyXXX_Assign_ex function, functionality now includ…

2207791

…ed in original _Assign function

dgarske requested changes Nov 15, 2024

View reviewed changes

dgarske assigned bigbrett and unassigned bigbrett and wolfSSL-Bot Nov 15, 2024

update test in api.c to handle new dilithium security level DER parsing

07e2715

SparkiDev requested changes Nov 18, 2024

View reviewed changes

wolfcrypt/src/asn.c Show resolved Hide resolved

wolfcrypt/src/asn.c Show resolved Hide resolved

wolfcrypt/test/test.c Show resolved Hide resolved

billphipps previously approved these changes Nov 18, 2024

View reviewed changes

wolfcrypt/src/dilithium.c Show resolved Hide resolved

wolfcrypt/src/dilithium.c Show resolved Hide resolved

wolfcrypt/test/test.c Show resolved Hide resolved

bigbrett added 3 commits November 18, 2024 15:47

properly protect test.c calls to dilithium public/private API

63deea5

add macros for dilithium DER export buffer sizes

d50fb63

make inOutKeyType parameter mandatory for DecodeAsymKey_Assign

9815fcd

bigbrett dismissed billphipps’s stale review via 9815fcd November 18, 2024 23:46

fix typo in comment

48dcbe9

SparkiDev previously approved these changes Nov 19, 2024

View reviewed changes

added DER size macros to libOQS build

26d3b00

bigbrett dismissed SparkiDev’s stale review via 26d3b00 November 19, 2024 00:53

bigbrett requested a review from dgarske November 19, 2024 15:03

dgarske approved these changes Nov 19, 2024

View reviewed changes

dgarske merged commit 18f52b2 into wolfSSL:master Nov 19, 2024
143 checks passed

bigbrett mentioned this pull request Nov 21, 2024

dilithium fixes for FIPS 204 draft mode #8207

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ML-DSA/Dilithium: obtain security level from DER when decoding #8177

ML-DSA/Dilithium: obtain security level from DER when decoding #8177

bigbrett commented Nov 12, 2024

SparkiDev commented Nov 12, 2024

bigbrett commented Nov 12, 2024 •

edited

Loading

SparkiDev commented Nov 12, 2024

bigbrett commented Nov 13, 2024

bigbrett commented Nov 14, 2024

dgarske Nov 15, 2024

bigbrett Nov 15, 2024

dgarske left a comment

billphipps left a comment

bigbrett commented Nov 19, 2024

bigbrett commented Nov 19, 2024

dgarske commented Nov 20, 2024

ML-DSA/Dilithium: obtain security level from DER when decoding #8177

ML-DSA/Dilithium: obtain security level from DER when decoding #8177

Conversation

bigbrett commented Nov 12, 2024

Description

Testing

Checklist

SparkiDev commented Nov 12, 2024

bigbrett commented Nov 12, 2024 • edited Loading

SparkiDev commented Nov 12, 2024

bigbrett commented Nov 13, 2024

bigbrett commented Nov 14, 2024

dgarske Nov 15, 2024

Choose a reason for hiding this comment

bigbrett Nov 15, 2024

Choose a reason for hiding this comment

dgarske left a comment

Choose a reason for hiding this comment

billphipps left a comment

Choose a reason for hiding this comment

bigbrett commented Nov 19, 2024

bigbrett commented Nov 19, 2024

dgarske commented Nov 20, 2024

bigbrett commented Nov 12, 2024 •

edited

Loading