From 5769664497cb7aaa43e2e6699aca0d290d38b9f9 Mon Sep 17 00:00:00 2001 From: Shaohua Wang Date: Thu, 15 Oct 2026 10:45:39 +0000 Subject: [PATCH] pki: Fail sshsig_sign when no signature was produced `sshsig_sign` documents that it returns `SSH_OK` on success, and that on success the `signature` out parameter holds the allocated armored signature. Two paths break that contract. Both are reached after `rc` has been set to `SSH_OK` by `sshsig_prepare_data`, and both jump to the shared `cleanup` label without resetting it, so the single `return rc` reports success while `signature` is still the `NULL` it was initialised to: - the `#else` of `#ifdef WITH_FIDO2`, taken for an SK key type on a build without FIDO2 - `sig == NULL`, taken when the signing backend refused the key A caller that follows the documented contract and only checks the return code then dereferences `NULL`. This was found as a segfault in a consumer signing with a 1024-bit RSA key on an OpenSSL FIPS build, where the modulus is below the approved minimum, so `pki_sign_data` returns `NULL` and the second path reports success. Set `rc = SSH_ERROR` on both before `goto cleanup`, which is how the rest of the file already behaves: `ssh_pki_generate_key` returns `SSH_ERROR` directly from its equivalent `#else /* WITH_FIDO2 */` branch, and `sshsig_verify` assigns `rc = SSH_ERROR` before its own `goto cleanup`. The test signs with the public part of the fixture RSA key. `pki_sign_data` rejects a key without private material at its `!ssh_key_is_private(privkey)` guard and returns `NULL`, reaching the second path, so the defect reproduces without a FIPS build or FIDO2 hardware. Reverting only the `src/pki.c` change makes it fail with `0 != 0xffffffffffffffff`. The SK branch is not covered: that needs a build with FIDO2 where `pki_sk_do_sign` is unavailable, and the suite has no fixture for it. --- src/pki.c | 2 ++ tests/unittests/torture_pki_sshsig.c | 34 ++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/src/pki.c b/src/pki.c index ef90df23..aede1d35 100644 --- a/src/pki.c +++ b/src/pki.c @@ -3527,6 +3527,7 @@ int sshsig_sign(const void *data, ssh_buffer_get_len(tosign)); #else SSH_LOG(SSH_LOG_WARN, SK_NOT_SUPPORTED_MSG); + rc = SSH_ERROR; goto cleanup; #endif } else { @@ -3538,6 +3539,7 @@ int sshsig_sign(const void *data, } if (sig == NULL) { SSH_LOG(SSH_LOG_TRACE, "Failed to sign data with private key"); + rc = SSH_ERROR; goto cleanup; } diff --git a/tests/unittests/torture_pki_sshsig.c b/tests/unittests/torture_pki_sshsig.c index 85b07a22..bf33dc72 100644 --- a/tests/unittests/torture_pki_sshsig.c +++ b/tests/unittests/torture_pki_sshsig.c @@ -796,6 +796,36 @@ static void torture_sshsig_error_cases_all_combinations(void **state) assert_int_equal(rc, SSH_ERROR); } +/* `pki_sign_data` refuses a key without private material, and `sshsig_sign` used to reach `cleanup` + * from its `sig == NULL` branch without setting `rc`. It then returned the `SSH_OK` that + * `sshsig_prepare_data` had left there, while the out parameter stayed NULL, so a caller following + * the documented contract - `SSH_OK` means the signature was allocated - dereferences NULL. The same + * applies to a key that the signing backend rejects, for example an RSA key below the FIPS minimum + * modulus on a FIPS build, which is how this was found. */ +static void torture_sshsig_sign_error_leaves_no_signature(void **state) +{ + struct sshsig_st *test_state = *state; + ssh_key pubkey = NULL; + char *signature = NULL; + int rc; + + rc = ssh_pki_export_privkey_to_pubkey(test_state->rsa_key, &pubkey); + assert_int_equal(rc, SSH_OK); + assert_non_null(pubkey); + + rc = sshsig_sign(input, + input_len, + pubkey, + NULL, /* pki_context */ + test_namespace, + SSHSIG_DIGEST_SHA2_256, + &signature); + assert_int_equal(rc, SSH_ERROR); + assert_null(signature); + + ssh_key_free(pubkey); +} + int torture_run_tests(void) { int rc; @@ -820,6 +850,10 @@ int torture_run_tests(void) torture_sshsig_error_cases_all_combinations, setup_sshsig_compat, teardown_sshsig_compat), + cmocka_unit_test_setup_teardown( + torture_sshsig_sign_error_leaves_no_signature, + setup_sshsig_compat, + teardown_sshsig_compat), }; ssh_init();