Add RSA-PSS signing support (TLS 1.3 client mutual authentication with RSA cert)

[EdouardMALOT]

Summary

This PR completes the RSA-PSS support introduced in #377 by adding the signing side: a TLS 1.3 client can now perform mutual authentication (client certificate) with an RSA certificate.

#377 added RSA-PSS verification, which lets a NetX Duo client verify the CertificateVerify sent by an RSA server. However, when the server requests a client certificate, the client must also sign its own CertificateVerify with RSA-PSS — RFC 8446 §4.4.3 forbids RSASSA-PKCS1-v1_5 in CertificateVerify. Before this PR, _nx_secure_tls_process_certificate_request() returned NX_SECURE_TLS_UNSUPPORTED_CERT_SIGN_TYPE for any RSA local certificate in TLS 1.3, making client-cert authentication impossible with RSA keys.

This implements rsa_pss_rsae_sha256 (0x0804), which is the mandatory-to-implement signature algorithm for CertificateVerify per RFC 8446 §9.1. Related: #161.

Changes

• crypto_libraries/inc/nx_crypto_rsa.h — declare _nx_crypto_rsa_pss_sign()
• crypto_libraries/src/nx_crypto_rsa.c — implement _nx_crypto_rsa_pss_sign(): EMSA-PSS-ENCODE (RFC 8017 §9.1.1), salt length == hash length as required by RFC 8446 §4.2.3, salt generated via NX_CRYPTO_RAND() (same entropy source as the ECDSA nonce path). The RSA private-key operation is then applied to the encoded message by the existing signing flow.
• nx_secure/src/nx_secure_tls_process_certificate_request.c — TLS 1.3: instead of rejecting RSA local certificates, select rsa_pss_rsae_sha256 and proceed with the client Certificate/CertificateVerify flow
• nx_secure/src/nx_secure_tls_send_certificate_verify.c — TLS 1.3: build the CertificateVerify message with the SignatureScheme wire code (rsa_pss_rsae_sha256/384/512 handled) and the PSS-encoded message, instead of PKCS#1 v1.5 padding (which remains used for TLS 1.2)

Notes / limitations

• When answering a CertificateRequest, the client currently selects rsa_pss_rsae_sha256 unconditionally (the RFC 8446 §9.1 MUST algorithm, supported by any compliant server). The CertificateVerify encoding path itself also supports rsa_pss_rsae_sha384/512 should the selection be extended later.
• rsa_pss_pss_* variants (RSASSA-PSS certificates, 0x0809–0x080b) are out of scope; this covers the common case of classic RSA certificates (rsae).

Test plan

• TLS 1.3 mutual authentication handshake (server requests a client certificate) completes with an RSA client certificate, rsa_pss_rsae_sha256 CertificateVerify accepted by the server (tested against a Mosquitto/OpenSSL broker requiring client certificates)
• TLS 1.3 server-only handshake (no CertificateRequest) unaffected
• TLS 1.2 client certificate path still uses RSASSA-PKCS1-v1_5 (unchanged behavior)

[EdouardMALOT]

@fdesbiens : the remaining Secure_Interoperability / run_tests failure is unrelated to this change — the job fails during the "Install softwares" step.

[fdesbiens]

Thank you for this PR, @EdouardMALOT, and for all the others you submitted.

I am currently on vacation; I will review them once I am back, the week of July 20.

[fdesbiens]

@fdesbiens : the remaining Secure_Interoperability / run_tests failure is unrelated to this change — the job fails during the "Install softwares" step.

This is expected. The script relies on ifconfig to tweak networking configuration, and this should be modernised. I will spend some time fixing this along with the reviews.

[fdesbiens]

Thank you for this — it is careful work, and the parts that are hardest to get right are right. Before the findings, what I verified as correct, because it is worth recording:

The PSS encoding is byte-exact. I extracted _nx_crypto_rsa_pss_sign() into a harness, made the salt deterministic by compiling with -DNX_CRYPTO_RAND=test_rand, and compared the resulting EM against an independent EMSA-PSS-ENCODE implementation:

SHA-256 / RSA-2048         modulus= 256  emLen= 256  MATCH
SHA-256 / RSA-3072         modulus= 384  emLen= 384  MATCH
SHA-256 / RSA-4096         modulus= 512  emLen= 512  MATCH
SHA-256 / 66-byte mod      modulus=  66  emLen=  66  MATCH   <- emLen == hLen + sLen + 2, empty PS
EMSA-PSS-ENCODE (RFC 8017 §9.1.1) byte-exact agreement: ALL MATCH

Every step is correct: emLen = ceil(emBits/8), the step 3 rejection, DB = PS || 0x01 || salt with the right PS length, MGF1 over H, the XOR, clearing the top 8*emLen - emBits bits, and the 0xBC trailer. The boundary case where PS is empty works, a 65-byte modulus is correctly refused, and an undersized scratch is correctly refused. Each EM round-trips through _nx_crypto_rsa_pss_verify(), and flipping one bit of mHash makes the verifier reject it. No ASan or UBSan findings on any of those runs.

The TLS 1.3 wiring is correct. Two things I specifically checked because they are the usual sources of interop failure:

• The value passed as mHash really is the right thing. By the time the PSS call is reached, handshake_hash holds the digest of 0x20 × 64 || context || 0x00 || transcript_hash (built at nx_secure_tls_send_certificate_verify.c:231-257, hashed immediately after, with handshake_hash_length reset to the digest length). That matches RFC 8446 §4.4.3.
• The 0x0804 selection is not blindly trusted. _nx_secure_tls_process_certificate_request() still matches expected_sign_alg against the server's advertised signature_algorithms list and returns NX_SECURE_TLS_UNSUPPORTED_CERT_SIGN_ALG if the server does not offer it, and nx_secure_tls_signature_algorithm is only assigned on a match. So a server that does not advertise rsa_pss_rsae_sha256 fails cleanly rather than receiving a signature it will reject. Good.

The tls_1_3_enable_build_coverage configuration builds clean under -Werror -Wall -Wextra, and all 150 nx_secure tests pass in that configuration, so nothing existing regresses.

Now the findings. One is blocking:

_nx_crypto_rsa_pss_sign() writes emLen bytes into a buffer whose size it is never told. The only caller passes the 600-byte _nx_secure_padded_signature with em_bits computed from the certificate's modulus length, which nx_secure bounds nowhere. With a 6144-bit RSA client certificate this walks off the end. Reproduced under ASan; details on nx_crypto_rsa.c:789.

The salt does not go through NX_CRYPTO_RBG, and the comment justifying that is incorrect — ECDSA does not use NX_CRYPTO_RAND() directly. Details on nx_crypto_rsa.c:837.

There is no test coverage. gcov on the coverage build reports 0 of 47 executable lines in _nx_crypto_rsa_pss_sign, and the same for the two functions #377 added. AGENTS.md asks for matching regression tests and 100% coverage. Details on nx_crypto_rsa.c:760, including a concrete way to make the function KAT-testable — I already have a harness that does it and am happy to hand it over.

Everything else is minor. Thank you also for basing this on dev.

[fdesbiens]

This function writes em_len = ceil(em_bits/8) bytes into em, but its parameter list has no output-buffer length. Compare scratch, which correctly gets a scratch_length and is checked at :823. There is no equivalent check for em, so the caller's buffer size is simply assumed.

The one caller does not satisfy that assumption in all configurations. In nx_secure_tls_send_certificate_verify.c, em is the 600-byte _nx_secure_padded_signature and em_bits is (data_size << 3) - 1, where data_size is nx_secure_rsa_public_modulus_length taken straight from the local certificate (:499). Nothing in nx_secure bounds that value — I grepped for a cap and there is none. Any RSA client certificate with a modulus above 600 bytes, i.e. above 4800 bits, overruns the buffer.

Reproduced with a 768-byte modulus (6144-bit key) and em allocated at exactly 600 bytes:

em buffer = 600 bytes, modulus = 768 bytes -> writing 768 bytes
==56476==ERROR: AddressSanitizer: heap-buffer-overflow on address 0xf4e03f5f
WRITE of size 1 at 0xf4e03f5f thread T0
    #0 _nx_crypto_sha256_digest_calculate  nx_crypto_sha2.c:317
    #1 _nx_crypto_method_sha256_operation  nx_crypto_sha2.c:786
    #2 _nx_crypto_rsa_pss_sign             nx_crypto_rsa.c:908
0xf4e03f5f is located 135 bytes after 600-byte region [0xf4e03c80,0xf4e03ed8)

The first write to land outside is H, written through h = em + db_len at :908, and the masked-DB loop then writes across the whole range.

This is not attacker-controlled — the modulus comes from the device's own certificate — so it is a robustness and configuration-safety problem rather than a remote issue. It is still memory corruption in a signing path, and a new public crypto API that cannot be called safely without out-of-band knowledge of its own output size.

Two changes would close it, and I would like both:

1. Add an em_length parameter and return NX_CRYPTO_INVALID_BUFFER_SIZE when em_len > em_length. It is a new function, so there is no compatibility cost to fixing the signature now.
2. Guard the call site on data_size > sizeof(_nx_secure_padded_signature) and return NX_SECURE_TLS_INVALID_CERTIFICATE.

Worth knowing: the PKCS#1 v1.5 path in the same function has the same missing guard already — working_ptr = &_nx_secure_padded_signature[data_size - signature_length] at :649 followed by a 36-byte copy. That one is pre-existing and not yours, but a single data_size check placed before the version branch fixes both, which seems like the right shape.

[EdouardMALOT]

Fixed in dc4716e, both parts. The function now takes an em_length, and the call site rejects data_size > sizeof(_nx_secure_padded_signature) before the version branch, so the PKCS#1 v1.5 path at :649 is covered too.

[fdesbiens]

The comment above this loop says NX_CRYPTO_RAND() is "the same source ECDSA uses for its nonce, so the entropy is at least as good as the existing signature paths on this device". That is not accurate, and the difference matters.

ECDSA and EC key generation go through NX_CRYPTO_RBG, not NX_CRYPTO_RAND — see nx_crypto_ec.c:2945 and :4671. NX_CRYPTO_RBG resolves to _nx_crypto_drbg in NX_CRYPTO_SELF_TEST (FIPS) builds and to _nx_crypto_huge_number_rbg otherwise (nx_crypto.h:92 and :100). So:

• In a FIPS build, ECDSA gets a real DRBG while this salt gets raw rand(). The two are not the same source at all.
• Even in a default build, _nx_crypto_huge_number_rbg consumes all 32 bits of each NX_CRYPTO_RAND() result (nx_crypto_huge_number.c:41-45), whereas this loop keeps only the low 8 bits and throws away 24. With a classic LCG rand() the low bits are the weakest part of the output, so this is the least favourable way to use it.

The fix is shorter than the current code and inherits the right behaviour in every build:

status = NX_CRYPTO_RBG(s_len << 3, salt);
if (status != NX_CRYPTO_SUCCESS)
{
    return(status);
}

On severity, to be fair to the change: PSS salt does not need to be secret, and unlike an ECDSA nonce a repeated or predictable salt does not leak the private key — it weakens the tightness of the security proof and loses the randomisation benefit, nothing more. So this is a robustness and FIPS-consistency fix, not a key-compromise bug. But there is no reason to bypass the RBG the rest of the library uses, and the comment should not claim parity that does not exist.

[EdouardMALOT]

Fixed in 39bc588 with the code you proposed, and the comment no longer claims parity with ECDSA. Builds clean with and without NX_CRYPTO_SELF_TEST.

Reading _nx_crypto_drbg I noticed it returns on a generate failure without releasing NX_CRYPTO_DRBG_MUTEX_PUT. Harmless by default since the macros are empty, but it leaks the mutex on ports that define them. It is unrelated to this PR so I kept it out — I will open a separate one.

[fdesbiens]

We need matching regression tests and 100% coverage for new features. There are none here, and gcov confirms nothing reaches the code. From tls_1_3_enable_build_coverage after the full 150-test run:

_nx_crypto_rsa_pss_mgf1      executable= 33  executed=  0  never= 33  (0%)
_nx_crypto_rsa_pss_verify    executable= 53  executed=  0  never= 53  (0%)
_nx_crypto_rsa_pss_sign      executable= 47  executed=  0  never= 47  (0%)

So the two functions from #377 are also untested — that is not this PR's doing, but it means the whole PSS feature currently has no coverage, and this PR is the natural point to fix that.

There is a structural obstacle worth solving deliberately: because the salt is generated inside the function, the output is not reproducible, so it cannot be compared against the RFC 8017 test vectors. Two ways out, and I would suggest the second:

1. Compile-time override. NX_CRYPTO_RAND is overridable, so a test can build with -DNX_CRYPTO_RAND=test_rand and get a deterministic salt. This is exactly how I produced the byte-exact comparison in the summary, so I know it works — happy to hand over the harness, it is about 120 lines and already covers RSA-2048/3072/4096, the empty-PS boundary, the step 3 rejection and the scratch-size rejection.
2. Accept an optional salt. Add a const UCHAR *salt, UINT salt_length pair where NX_CRYPTO_NULL means "generate internally". That makes the function directly KAT-testable without build tricks, matches what most PSS APIs do, and would let a test use the published RFC 8017 vectors as-is.

Either way, please also add a sign-then-verify round-trip test and a negative test (tampered EM must be rejected), and ideally one end-to-end TLS 1.3 client-certificate test with an RSA cert so the nx_secure side is covered too. Note that a test built on option 1 would also give #377's verify path its first coverage.

If you do this part, I could look into providing the rest of the test coverage for #377 myself.

[fdesbiens]

The comment above this line says setting signature_length == data_size "makes the PKCS#1 loop body below run zero iterations (i < data_size - data_size - 1 underflows)". That reasoning is inverted, and the conclusion is the opposite of the truth.

data_size, signature_length and i are all UINT. With the two equal, data_size - signature_length - 1 evaluates to 0xFFFFFFFF, so for (i = 2; i < 0xFFFFFFFF; ++i) would run about four billion iterations writing 0xFF across all of memory — not zero iterations. The only thing preventing that is the if (!tls_session -> nx_secure_tls_1_3) guard you added at :668.

The guard is correct and the code is safe today. The problem is the comment: it tells the next reader that the underflow is harmless, which is precisely the belief under which someone would later delete the guard as redundant. That is a nasty trap to leave in a signing path.

Please either drop the assignment entirely, since nothing downstream reads signature_length on the TLS 1.3 path, or set it to a meaningful value — and in both cases replace the comment with a plain statement that the PKCS#1 padding loop must not run for TLS 1.3, which is what the guard at :668 enforces.

[EdouardMALOT]

Fixed in 59faf34. You were right, and I took the first option: nothing reads signature_length on the TLS 1.3 path, so the assignment is gone.

One correction to the note: the guard is still needed, but signature_length is initialised to 0 at :95, so without it the loop bound is data_size - 1 and it overwrites the PSS EM with 0xFF rather than running four billion iterations. The comment says that now.

[fdesbiens]

0x0804, 0x0805 and 0x0806 appear as bare literals here and again as case labels at nx_secure_tls_send_certificate_verify.c:196, :199 and :202. Every other signature algorithm in this code has a name — NX_SECURE_TLS_SIGNATURE_ECDSA_SHA256 and friends.

There are currently no PSS constants anywhere in nx_secure/inc/, so please add them alongside the existing signature-algorithm defines, e.g. NX_SECURE_TLS_SIGNATURE_RSA_PSS_RSAE_SHA256 and the 384/512 variants, and use them in both files. It also makes the eventual rsa_pss_pss_* extension easier to slot in.

[fdesbiens]

This brings the file to roughly 1.4 KB of static RAM — handshake_hash[162], _nx_secure_padded_signature[600] and now this — which is a noticeable amount on the class of device ThreadX targets.

The sizing comment says "db[<=511 B] + salt[<=64 B] for RSA-4096+SHA-512", which checks out: RSA-4096 with SHA-512 needs 447 + 64 = 511 bytes, so 600 is comfortable. But the buffer is only ever needed for the duration of one call, and the function already fails cleanly with NX_CRYPTO_INVALID_BUFFER_SIZE when the scratch is too small, so there is room to be tighter — 512 would do for everything up to RSA-4096, and deriving it from a documented maximum modulus constant would be clearer than a bare 600.

Also worth stating in a comment: because this is static, two TLS sessions signing concurrently will corrupt each other. That is the existing pattern in this file rather than something you introduced, but this adds one more instance of it, and it is not obvious to a reader.

[fdesbiens]

Every other externally visible function in this file is declared NX_CRYPTO_KEEP — _nx_crypto_rsa_operation at :82, _nx_crypto_method_rsa_init at :188, _nx_crypto_method_rsa_cleanup at :263, _nx_crypto_method_rsa_operation at :327. Neither _nx_crypto_rsa_pss_sign nor _nx_crypto_rsa_pss_verify has it.

That macro controls linker section placement and keep-out on several ports, so the omission can change what a port emits. #377 has the same gap, so fixing both here would be tidy.

[fdesbiens]

The new else { ... } wraps the existing curve switch but leaves its body at the old indentation, so the block's closing braces end up stacked and the switch looks as though it is outside the else. Please re-indent the wrapped lines one level.

[fdesbiens]

The new function's header block has DESCRIPTION, INPUT and OUTPUT, but omits AUTHOR, CALLS, CALLED BY. This should be added.

Another detail to fix while you are in there. The INPUT text describes scratch as "must be >= db_len bytes", but the code requires db_len + s_len (:823) — the documented requirement is smaller than the real one, which is the wrong direction for a caller to be misled in.

[EdouardMALOT]

Done in 5507dda, and the scratch line was fixed in dc4716e. I did the same for _nx_crypto_rsa_pss_verify and _nx_crypto_rsa_pss_mgf1, since they had the same gaps.

[fdesbiens]

Thank you for your meaningful contribution, @EdouardMALOT, and for your patience. I just submitted my review.

[EdouardMALOT]

Thanks for the review. Pushed four commits: the blocking finding, the two other substantive ones, and the header blocks. The remaining minor items and the test coverage are still to come.