From 13e1042b99221087c4c384be3d150df44c3d93cb Mon Sep 17 00:00:00 2001 From: Viktor Szakats Date: Mon, 7 Sep 2026 13:42:21 +0200 Subject: [PATCH] crypto: make `ssh2_bn_from_bin()` init the bignum, drop `ssh2_bn_init_from_bin()` To better hide backend-specific differences and t bring the init sub-steps (1. allocation, 2. conversion) close to each other. Also: - HACKING_CRYPTO.md: fix docs for `ssh2_bn_from_bin()` and `ssh2_bn_set_word()`. Ref: https://github.com/libssh2/libssh2/pull/2583#discussion_r3948922721 Closes #2594 --- src/HACKING-CRYPTO.md | 17 ++++----------- src/crypto.h | 5 ++--- src/kex.c | 43 ++++++++++++++++--------------------- src/libgcrypt.h | 4 +--- src/mbedtls.c | 14 ++++++++++++ src/mbedtls.h | 1 - src/openssl.c | 14 ++++++++++++ src/openssl.h | 1 - src/os400qc3.c | 50 ++++++++++++++++++++++++------------------- src/wincng.c | 26 +++++++++++++--------- 10 files changed, 97 insertions(+), 78 deletions(-) diff --git a/src/HACKING-CRYPTO.md b/src/HACKING-CRYPTO.md index a0ed905c..c4259653 100644 --- a/src/HACKING-CRYPTO.md +++ b/src/HACKING-CRYPTO.md @@ -329,15 +329,6 @@ ssh2_bn *ssh2_bn_init(void); ``` Creates a multiple precision number (preset to zero). -```c -ssh2_bn *ssh2_bn_init_from_bin(void); -``` - -Create a multiple precision number intended to be set by the -`ssh2_bn_from_bin()` function (see below). Unlike `ssh2_bn_init()`, this code -may be a dummy initializer if the `ssh2_bn_from_bin()` actually allocates the -number. Returns a value of type `ssh2_bn *`. - ```c void ssh2_bn_free(ssh2_bn *bn); ``` @@ -358,17 +349,17 @@ Returns the number of bits of multiple precision number at `bn`. int ssh2_bn_set_word(ssh2_bn *bn, uint32_t word); ``` Sets the value of `bn` to `word`. -Returns 1 on success, 0 otherwise. +Returns 0 on success, non-zero on failure. ```c -ssh2_bn *ssh2_bn_from_bin(ssh2_bn *bn, const unsigned char *bin, size_t len); +int ssh2_bn_from_bin(ssh2_bn **bn, const unsigned char *bin, size_t len); ``` Converts the positive integer in big-endian form of length `len` at `bin` into -an `ssh2_bn` and place it in `bn`. If `bn` is NULL, a new `ssh2_bn` is +an `ssh2_bn` and place it in `*bn`. If `*bn` is NULL, a new `ssh2_bn` is created. -Returns a pointer to target `ssh2_bn` or NULL if error. +Returns 0 on success, non-zero on failure. ```c int ssh2_bn_to_bin(ssh2_bn *bn, unsigned char *bin); diff --git a/src/crypto.h b/src/crypto.h index a006d471..c5b6dee8 100644 --- a/src/crypto.h +++ b/src/crypto.h @@ -305,11 +305,10 @@ void ssh2_bn_free(ssh2_bn *bn); #ifndef ssh2_bn_set_word int ssh2_bn_set_word(ssh2_bn *bn, uint32_t word); size_t ssh2_bn_bits(const ssh2_bn *bn); -int ssh2_bn_from_bin(ssh2_bn *bn, const unsigned char *bin, size_t len); int ssh2_bn_to_bin(const ssh2_bn *bn, unsigned char *bin); #endif -#ifndef ssh2_bn_init_from_bin -#define ssh2_bn_init_from_bin() ssh2_bn_init() +#ifndef ssh2_bn_from_bin +int ssh2_bn_from_bin(ssh2_bn **bn, const unsigned char *bin, size_t len); #endif void ssh2_dh_init(ssh2_dh_ctx *dhctx); diff --git a/src/kex.c b/src/kex.c index f1068655..654767bf 100644 --- a/src/kex.c +++ b/src/kex.c @@ -427,8 +427,7 @@ static int kex_diffie_hellman_sha(LIBSSH2_SESSION *session, exchange_state->ctx = ssh2_bn_ctx_new(); ssh2_dh_init(&exchange_state->x); exchange_state->e = ssh2_bn_init(); /* g^x mod p */ - exchange_state->f = ssh2_bn_init_from_bin(); /* g^(Random from - server) mod p */ + exchange_state->f = NULL; /* g^(Random from server) mod p */ exchange_state->k = ssh2_bn_init(); /* The shared secret: f^x mod p */ /* Zero the whole thing out */ @@ -557,7 +556,7 @@ static int kex_diffie_hellman_sha(LIBSSH2_SESSION *session, goto clean_exit; } - if(ssh2_bn_from_bin(exchange_state->f, + if(ssh2_bn_from_bin(&exchange_state->f, exchange_state->f_value, exchange_state->f_value_len)) { ret = ssh2_err(session, LIBSSH2_ERROR_HOSTKEY_INIT, @@ -761,8 +760,7 @@ static int kex_method_diffie_hellman_group1_sha1_key_exchange( if(key_state->state == ssh2_NB_state_idle) { /* g == 2 */ - key_state->p = ssh2_bn_init_from_bin(); /* SSH2 defined value - (p_value) */ + key_state->p = NULL; /* SSH2 defined value (p_value) */ key_state->g = ssh2_bn_init(); /* SSH2 defined value (2) */ /* Initialize P and G */ @@ -771,7 +769,7 @@ static int kex_method_diffie_hellman_group1_sha1_key_exchange( "Failed to allocate key state g."); goto clean_exit; } - if(!key_state->p || ssh2_bn_from_bin(key_state->p, p_value, 128)) { + if(ssh2_bn_from_bin(&key_state->p, p_value, 128)) { ret = ssh2_err(session, LIBSSH2_ERROR_ALLOC, "Failed to allocate key state p."); goto clean_exit; @@ -856,8 +854,7 @@ static int kex_method_diffie_hellman_group14_key_exchange( int ret; if(key_state->state == ssh2_NB_state_idle) { - key_state->p = ssh2_bn_init_from_bin(); /* SSH2 defined value - (p_value) */ + key_state->p = NULL; /* SSH2 defined value (p_value) */ key_state->g = ssh2_bn_init(); /* SSH2 defined value (2) */ /* g == 2 */ @@ -867,8 +864,7 @@ static int kex_method_diffie_hellman_group14_key_exchange( "Failed to allocate key state g."); goto clean_exit; } - else if(!key_state->p || - ssh2_bn_from_bin(key_state->p, p_value, 256)) { + else if(ssh2_bn_from_bin(&key_state->p, p_value, 256)) { ret = ssh2_err(session, LIBSSH2_ERROR_ALLOC, "Failed to allocate key state p."); goto clean_exit; @@ -968,8 +964,7 @@ static int kex_method_diffie_hellman_group16_sha512_key_exchange( int ret; if(key_state->state == ssh2_NB_state_idle) { - key_state->p = ssh2_bn_init_from_bin(); /* SSH2 defined value - (p_value) */ + key_state->p = NULL; /* SSH2 defined value (p_value) */ key_state->g = ssh2_bn_init(); /* SSH2 defined value (2) */ /* g == 2 */ @@ -979,7 +974,7 @@ static int kex_method_diffie_hellman_group16_sha512_key_exchange( "Failed to allocate key state g."); goto clean_exit; } - if(!key_state->p || ssh2_bn_from_bin(key_state->p, p_value, 512)) { + if(ssh2_bn_from_bin(&key_state->p, p_value, 512)) { ret = ssh2_err(session, LIBSSH2_ERROR_ALLOC, "Failed to allocate key state p."); goto clean_exit; @@ -1101,8 +1096,7 @@ static int kex_method_diffie_hellman_group18_sha512_key_exchange( int ret; if(key_state->state == ssh2_NB_state_idle) { - key_state->p = ssh2_bn_init_from_bin(); /* SSH2 defined value - (p_value) */ + key_state->p = NULL; /* SSH2 defined value (p_value) */ key_state->g = ssh2_bn_init(); /* SSH2 defined value (2) */ /* g == 2 */ @@ -1112,8 +1106,7 @@ static int kex_method_diffie_hellman_group18_sha512_key_exchange( "Failed to allocate key state g."); goto clean_exit; } - else if(!key_state->p || - ssh2_bn_from_bin(key_state->p, p_value, 1024)) { + else if(ssh2_bn_from_bin(&key_state->p, p_value, 1024)) { ret = ssh2_err(session, LIBSSH2_ERROR_ALLOC, "Failed to allocate key state p."); goto clean_exit; @@ -1150,8 +1143,8 @@ static int kex_method_diffie_hellman_group_exchange_sha1_key_exchange( int rc; if(key_state->state == ssh2_NB_state_idle) { - key_state->p = ssh2_bn_init_from_bin(); - key_state->g = ssh2_bn_init_from_bin(); + key_state->p = NULL; + key_state->g = NULL; /* Ask for a P and G pair */ key_state->request[0] = SSH_MSG_KEX_DH_GEX_REQUEST; ssh2_htonu32(key_state->request + 1, SSH2_DH_GEX_MINGROUP); @@ -1222,7 +1215,7 @@ static int kex_method_diffie_hellman_group_exchange_sha1_key_exchange( goto dh_gex_clean_exit; } - if(ssh2_bn_from_bin(key_state->p, p, p_len)) { + if(ssh2_bn_from_bin(&key_state->p, p, p_len)) { ret = ssh2_err(session, LIBSSH2_ERROR_PROTO, "Invalid DH-SHA1 p"); goto dh_gex_clean_exit; } @@ -1235,7 +1228,7 @@ static int kex_method_diffie_hellman_group_exchange_sha1_key_exchange( goto dh_gex_clean_exit; } - if(ssh2_bn_from_bin(key_state->g, g, g_len)) { + if(ssh2_bn_from_bin(&key_state->g, g, g_len)) { ret = ssh2_err(session, LIBSSH2_ERROR_PROTO, "Invalid DH-SHA1 g"); goto dh_gex_clean_exit; } @@ -1270,8 +1263,8 @@ static int kex_method_diffie_hellman_group_exchange_sha256_key_exchange( int rc; if(key_state->state == ssh2_NB_state_idle) { - key_state->p = ssh2_bn_init_from_bin(); - key_state->g = ssh2_bn_init_from_bin(); + key_state->p = NULL; + key_state->g = NULL; /* Ask for a P and G pair */ key_state->request[0] = SSH_MSG_KEX_DH_GEX_REQUEST; ssh2_htonu32(key_state->request + 1, SSH2_DH_GEX_MINGROUP); @@ -1343,7 +1336,7 @@ static int kex_method_diffie_hellman_group_exchange_sha256_key_exchange( goto dh_gex_clean_exit; } - if(ssh2_bn_from_bin(key_state->p, p, p_len)) { + if(ssh2_bn_from_bin(&key_state->p, p, p_len)) { ret = ssh2_err(session, LIBSSH2_ERROR_PROTO, "Invalid DH-SHA256 p"); goto dh_gex_clean_exit; @@ -1357,7 +1350,7 @@ static int kex_method_diffie_hellman_group_exchange_sha256_key_exchange( goto dh_gex_clean_exit; } - if(ssh2_bn_from_bin(key_state->g, g, g_len)) { + if(ssh2_bn_from_bin(&key_state->g, g, g_len)) { ret = ssh2_err(session, LIBSSH2_ERROR_PROTO, "Invalid DH-SHA256 g"); goto dh_gex_clean_exit; diff --git a/src/libgcrypt.h b/src/libgcrypt.h index e1489d15..1f773225 100644 --- a/src/libgcrypt.h +++ b/src/libgcrypt.h @@ -116,11 +116,9 @@ #define ssh2_bn struct gcry_mpi #define ssh2_bn_init() gcry_mpi_new(0) -#define ssh2_bn_init_from_bin() NULL /* because gcry_mpi_scan() - creates a new bignum */ #define ssh2_bn_set_word(bn, word) gcry_mpi_set_ui(bn, word) #define ssh2_bn_from_bin(bn, bin, len) \ - gcry_mpi_scan(&(bn), GCRYMPI_FMT_USG, bin, len, NULL) + gcry_mpi_scan(bn, GCRYMPI_FMT_USG, bin, len, NULL) #define ssh2_bn_to_bin(bn, bin) \ gcry_mpi_print(GCRYMPI_FMT_USG, bin, ssh2_bn_bytes(bn), NULL, bn) #define ssh2_bn_bytes(bn) \ diff --git a/src/mbedtls.c b/src/mbedtls.c index 08fc38f5..caadb019 100644 --- a/src/mbedtls.c +++ b/src/mbedtls.c @@ -373,6 +373,20 @@ void ssh2_bn_free(ssh2_bn *bn) } } +int ssh2_bn_from_bin(ssh2_bn **bn, const unsigned char *bin, size_t len) +{ + if(!bn || !bin || !len) + return -1; + + if(!*bn) { + *bn = ssh2_bn_init(); + if(!*bn) + return -1; + } + + return mbedtls_mpi_read_binary(*bn, bin, len); +} + static int mbed_bn_random(ssh2_bn *bn, int bits, int top, int bottom) { size_t len; diff --git a/src/mbedtls.h b/src/mbedtls.h index e938469d..989fd4af 100644 --- a/src/mbedtls.h +++ b/src/mbedtls.h @@ -219,7 +219,6 @@ struct ssh2_mbed_cipher_ctx { #define ssh2_bn mbedtls_mpi #define ssh2_bn_set_word(bn, word) mbedtls_mpi_lset(bn, word) -#define ssh2_bn_from_bin(bn, bin, len) mbedtls_mpi_read_binary(bn, bin, len) #define ssh2_bn_to_bin(bn, bin) \ mbedtls_mpi_write_binary(bn, bin, mbedtls_mpi_size(bn)) #define ssh2_bn_bytes(bn) mbedtls_mpi_size(bn) diff --git a/src/openssl.c b/src/openssl.c index 3c5d1b17..d138e74b 100644 --- a/src/openssl.c +++ b/src/openssl.c @@ -3715,4 +3715,18 @@ void ssh2_dh_dtor(ssh2_dh_ctx *dhctx) *dhctx = NULL; } +int ssh2_bn_from_bin(ssh2_bn **bn, const unsigned char *bin, size_t len) +{ + if(!bn || !bin || !len) + return -1; + + if(!*bn) { + *bn = ssh2_bn_init(); + if(!*bn) + return -1; + } + + return BN_bin2bn(bin, (int)len, *bn) ? 0 : -1; +} + #endif /* LIBSSH2_OPENSSL || LIBSSH2_WOLFSSL */ diff --git a/src/openssl.h b/src/openssl.h index 4b6009c0..c5bd3a0e 100644 --- a/src/openssl.h +++ b/src/openssl.h @@ -344,7 +344,6 @@ typedef enum { #define ssh2_bn BIGNUM #define ssh2_bn_init() BN_new() #define ssh2_bn_set_word(bn, word) !BN_set_word(bn, word) -#define ssh2_bn_from_bin(bn, bin, l) (BN_bin2bn(bin, (int)(l), bn) ? 0 : -1) #define ssh2_bn_to_bin(bn, bin) (BN_bn2bin(bn, bin) <= 0) #define ssh2_bn_bytes(bn) ((size_t)BN_num_bytes(bn)) #define ssh2_bn_bits(bn) ((size_t)BN_num_bits(bn)) diff --git a/src/os400qc3.c b/src/os400qc3.c index 2e11928f..63d5e64f 100644 --- a/src/os400qc3.c +++ b/src/os400qc3.c @@ -460,29 +460,37 @@ size_t ssh2_bn_bits(const ssh2_bn *bn) return 0; } -int ssh2_bn_from_bin(ssh2_bn *bn, const unsigned char *bin, size_t len) +int ssh2_bn_from_bin(ssh2_bn **bn, const unsigned char *bin, size_t len) { size_t i; if(!bn || (len && !bin)) return -1; + if(!*bn) { + *bn = ssh2_bn_init(); + if(!*bn) + return -1; + } + for(; len && !*bin; len--) bin++; - if(bn_resize(bn, len)) + if(bn_resize(*bn, len)) return -1; for(i = len; i--;) - bn->bignum[i] = *bin++; + (*bn)->bignum[i] = *bin++; return 0; } int ssh2_bn_set_word(ssh2_bn *bn, uint32_t word) { + if(!bn) + return -1; word = htonl(word); - return ssh2_bn_from_bin(bn, (unsigned char *)&word, sizeof(word)); + return ssh2_bn_from_bin(&bn, (unsigned char *)&word, sizeof(word)); } int ssh2_bn_to_bin(const ssh2_bn *bn, unsigned char *bin) @@ -1173,8 +1181,8 @@ int ssh2_rsa_new(ssh2_rsa_ctx **rsa, LIBSSH2_SESSION *session, const unsigned char *coeffdata, size_t coefflen) { ssh2_rsa_ctx *ctx; - ssh2_bn *e = ssh2_bn_init_from_bin(); - ssh2_bn *n = ssh2_bn_init_from_bin(); + ssh2_bn *e = NULL; + ssh2_bn *n = NULL; ssh2_bn *d = NULL; ssh2_bn *p = NULL; ssh2_bn *q = NULL; @@ -1193,25 +1201,19 @@ int ssh2_rsa_new(ssh2_rsa_ctx **rsa, LIBSSH2_SESSION *session, if(!ctx) ret = -1; if(!ret) { - ssh2_bn_from_bin(e, edata, elen); - ssh2_bn_from_bin(n, ndata, nlen); + ssh2_bn_from_bin(&e, edata, elen); + ssh2_bn_from_bin(&n, ndata, nlen); if(!e || !n) ret = -1; } if(!ret && ddata) { /* Private key. */ - d = ssh2_bn_init_from_bin(); - ssh2_bn_from_bin(d, ddata, dlen); - p = ssh2_bn_init_from_bin(); - ssh2_bn_from_bin(p, pdata, plen); - q = ssh2_bn_init_from_bin(); - ssh2_bn_from_bin(q, qdata, qlen); - e1 = ssh2_bn_init_from_bin(); - ssh2_bn_from_bin(e1, e1data, e1len); - e2 = ssh2_bn_init_from_bin(); - ssh2_bn_from_bin(e2, e2data, e2len); - coeff = ssh2_bn_init_from_bin(); - ssh2_bn_from_bin(coeff, coeffdata, coefflen); + ssh2_bn_from_bin(&d, ddata, dlen); + ssh2_bn_from_bin(&p, pdata, plen); + ssh2_bn_from_bin(&q, qdata, qlen); + ssh2_bn_from_bin(&e1, e1data, e1len); + ssh2_bn_from_bin(&e2, e2data, e2len); + ssh2_bn_from_bin(&coeff, coeffdata, coefflen); if(!d || !p || !q || !e1 || !e2 || !coeff) ret = -1; @@ -1289,6 +1291,8 @@ int ssh2_dh_key_pair(ssh2_dh_ctx *dhctx, ssh2_bn *pub, const ssh2_bn *g, (void)bnctx; + if(!pub) + return -1; if(group_order <= 0) return -1; @@ -1319,7 +1323,7 @@ int ssh2_dh_key_pair(ssh2_dh_ctx *dhctx, ssh2_bn *pub, const ssh2_bn *g, asn1delete(pkcs3); if(errcode.Bytes_Available) return -1; - return ssh2_bn_from_bin(pub, (unsigned char *)pubkey, pubkeylen); + return ssh2_bn_from_bin(&pub, (unsigned char *)pubkey, pubkeylen); } int ssh2_dh_validate(const ssh2_bn *f, const ssh2_bn *p) @@ -1356,6 +1360,8 @@ int ssh2_dh_secret(ssh2_dh_ctx *dhctx, ssh2_bn *secret, (void)bnctx; + if(!secret) + return -1; if(ssh2_dh_validate(f, p)) return -1; @@ -1369,7 +1375,7 @@ int ssh2_dh_secret(ssh2_dh_ctx *dhctx, ssh2_bn *secret, &secretbufsize, &secretbuflen, &errcode); if(errcode.Bytes_Available) return -1; - return ssh2_bn_from_bin(secret, (unsigned char *)secretbuf, secretbuflen); + return ssh2_bn_from_bin(&secret, (unsigned char *)secretbuf, secretbuflen); } void ssh2_dh_dtor(ssh2_dh_ctx *dhctx) diff --git a/src/wincng.c b/src/wincng.c index b601782f..16658acd 100644 --- a/src/wincng.c +++ b/src/wincng.c @@ -318,7 +318,7 @@ size_t ssh2_bn_bits(const ssh2_bn *bn) return bits; } -int ssh2_bn_from_bin(ssh2_bn *bn, const unsigned char *bin, size_t len) +int ssh2_bn_from_bin(ssh2_bn **bn, const unsigned char *bin, size_t len) { unsigned char *bignum; size_t offset, length, bits; @@ -326,24 +326,30 @@ int ssh2_bn_from_bin(ssh2_bn *bn, const unsigned char *bin, size_t len) if(!bn || !bin || !len) return -1; - if(wcng_bn_resize(bn, len)) + if(!*bn) { + *bn = ssh2_bn_init(); + if(!*bn) + return -1; + } + + if(wcng_bn_resize(*bn, len)) return -1; - memcpy(bn->bignum, bin, len); + memcpy((*bn)->bignum, bin, len); - bits = ssh2_bn_bits(bn); + bits = ssh2_bn_bits(*bn); length = (bits + 7) / 8; - offset = bn->length - length; + offset = (*bn)->length - length; if(offset > 0) { - memmove(bn->bignum, bn->bignum + offset, length); + memmove((*bn)->bignum, (*bn)->bignum + offset, length); - ssh2_explicit_zero(bn->bignum + length, offset); + ssh2_explicit_zero((*bn)->bignum + length, offset); - bignum = realloc(bn->bignum, length); + bignum = realloc((*bn)->bignum, length); if(bignum) { - bn->bignum = bignum; - bn->length = length; + (*bn)->bignum = bignum; + (*bn)->length = length; } else return -1;