aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorDirkjan Bussink <d.bussink@gmail.com>2020-12-10 14:01:32 +0000
committerAndreas Schneider <asn@cryptomilk.org>2020-12-11 13:32:02 +0100
commitdaeee74edd8ac25c1d246d40333e78518574eded (patch)
treee6646a06bcc8e4338d3a094bcd5af034fe8a0c1f
parentf6a2f6190c2aa047d901547720ae6d1729e1e2c0 (diff)
downloadlibssh-daeee74edd8ac25c1d246d40333e78518574eded.tar.gz
libssh-daeee74edd8ac25c1d246d40333e78518574eded.tar.xz
libssh-daeee74edd8ac25c1d246d40333e78518574eded.zip
Add safety checks for all ssh_string_fill calls
These calls can fail and the return code should always be checked. These issues were identified when code review called it out on new code. The updates here are to existing code with no behavior changes to make review simpler. Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> Reviewed-by: Jakub Jelen <jjelen@redhat.com> Reviewed-by: Andreas Schneider <asn@cryptomilk.org>
-rw-r--r--src/curve25519.c20
-rw-r--r--src/pki.c17
-rw-r--r--src/pki_crypto.c31
-rw-r--r--src/pki_ed25519_common.c11
-rw-r--r--src/pki_gcrypt.c25
-rw-r--r--src/pki_mbedcrypto.c13
-rw-r--r--tests/unittests/torture_pki_ed25519.c6
-rw-r--r--tests/unittests/torture_session_keys.c4
8 files changed, 97 insertions, 30 deletions
diff --git a/src/curve25519.c b/src/curve25519.c
index c13b3604..d2517551 100644
--- a/src/curve25519.c
+++ b/src/curve25519.c
@@ -377,12 +377,12 @@ void ssh_server_curve25519_init(ssh_session session){
*/
static SSH_PACKET_CALLBACK(ssh_packet_server_curve25519_init){
/* ECDH keys */
- ssh_string q_c_string;
- ssh_string q_s_string;
+ ssh_string q_c_string = NULL;
+ ssh_string q_s_string = NULL;
ssh_string server_pubkey_blob = NULL;
/* SSH host keys (rsa,dsa,ecdsa) */
- ssh_key privkey;
+ ssh_key privkey = NULL;
enum ssh_digest_e digest = SSH_DIGEST_AUTO;
ssh_string sig_blob = NULL;
int rc;
@@ -402,7 +402,6 @@ static SSH_PACKET_CALLBACK(ssh_packet_server_curve25519_init){
SSH_FATAL,
"Incorrect size for server Curve25519 public key: %zu",
ssh_string_len(q_c_string));
- SSH_STRING_FREE(q_c_string);
goto error;
}
@@ -460,12 +459,17 @@ static SSH_PACKET_CALLBACK(ssh_packet_server_curve25519_init){
/* add ecdh public key */
q_s_string = ssh_string_new(CURVE25519_PUBKEY_SIZE);
if (q_s_string == NULL) {
+ ssh_set_error_oom(session);
goto error;
}
- ssh_string_fill(q_s_string,
- session->next_crypto->curve25519_server_pubkey,
- CURVE25519_PUBKEY_SIZE);
+ rc = ssh_string_fill(q_s_string,
+ session->next_crypto->curve25519_server_pubkey,
+ CURVE25519_PUBKEY_SIZE);
+ if (rc < 0) {
+ ssh_set_error(session, SSH_FATAL, "Could not copy public key");
+ goto error;
+ }
rc = ssh_buffer_add_ssh_string(session->out_buffer, q_s_string);
SSH_STRING_FREE(q_s_string);
@@ -508,6 +512,8 @@ static SSH_PACKET_CALLBACK(ssh_packet_server_curve25519_init){
return SSH_PACKET_USED;
error:
+ SSH_STRING_FREE(q_c_string);
+ SSH_STRING_FREE(q_s_string);
ssh_buffer_reinit(session->out_buffer);
session->session_state=SSH_SESSION_STATE_ERROR;
return SSH_PACKET_USED;
diff --git a/src/pki.c b/src/pki.c
index 0d86fbcd..9248c9b3 100644
--- a/src/pki.c
+++ b/src/pki.c
@@ -2238,8 +2238,12 @@ int ssh_pki_export_signature_blob(const ssh_signature sig,
return SSH_ERROR;
}
- ssh_string_fill(str, ssh_buffer_get(buf), ssh_buffer_get_len(buf));
+ rc = ssh_string_fill(str, ssh_buffer_get(buf), ssh_buffer_get_len(buf));
SSH_BUFFER_FREE(buf);
+ if (rc < 0) {
+ SSH_STRING_FREE(str);
+ return SSH_ERROR;
+ }
*sig_blob = str;
@@ -2558,7 +2562,10 @@ ssh_string ssh_pki_do_sign(ssh_session session,
if (session_id == NULL) {
return NULL;
}
- ssh_string_fill(session_id, crypto->session_id, crypto->digest_len);
+ rc = ssh_string_fill(session_id, crypto->session_id, crypto->digest_len);
+ if (rc < 0) {
+ goto end;
+ }
/* Fill the input */
sign_input = ssh_buffer_new();
@@ -2619,7 +2626,11 @@ ssh_string ssh_pki_do_sign_agent(ssh_session session,
if (session_id == NULL) {
return NULL;
}
- ssh_string_fill(session_id, crypto->session_id, crypto->digest_len);
+ rc = ssh_string_fill(session_id, crypto->session_id, crypto->digest_len);
+ if (rc < 0) {
+ SSH_STRING_FREE(session_id);
+ return NULL;
+ }
sig_buf = ssh_buffer_new();
if (sig_buf == NULL) {
diff --git a/src/pki_crypto.c b/src/pki_crypto.c
index 08409209..3c3e0a40 100644
--- a/src/pki_crypto.c
+++ b/src/pki_crypto.c
@@ -840,7 +840,11 @@ ssh_string pki_private_key_to_pem(const ssh_key key,
goto err;
}
- ssh_string_fill(blob, buf->data, buf->length);
+ rc = ssh_string_fill(blob, buf->data, buf->length);
+ if (rc < 0) {
+ goto err;
+ }
+
BIO_free(mem);
return blob;
@@ -1411,6 +1415,7 @@ static ssh_string pki_dsa_signature_to_blob(const ssh_signature sig)
const unsigned char *raw_sig_data = NULL;
size_t raw_sig_len;
+ int rc;
DSA_SIG *dsa_sig;
@@ -1467,7 +1472,11 @@ static ssh_string pki_dsa_signature_to_blob(const ssh_signature sig)
return NULL;
}
- ssh_string_fill(sig_blob, buffer, 40);
+ rc = ssh_string_fill(sig_blob, buffer, 40);
+ if (rc < 0) {
+ SSH_STRING_FREE(sig_blob);
+ return NULL;
+ }
return sig_blob;
@@ -1544,7 +1553,10 @@ static ssh_string pki_ecdsa_signature_to_blob(const ssh_signature sig)
goto error;
}
- ssh_string_fill(sig_blob, ssh_buffer_get(buf), ssh_buffer_get_len(buf));
+ rc = ssh_string_fill(sig_blob, ssh_buffer_get(buf), ssh_buffer_get_len(buf));
+ if (rc < 0) {
+ goto error;
+ }
SSH_STRING_FREE(r);
SSH_STRING_FREE(s);
@@ -1554,6 +1566,7 @@ static ssh_string pki_ecdsa_signature_to_blob(const ssh_signature sig)
return sig_blob;
error:
+ SSH_STRING_FREE(sig_blob);
SSH_STRING_FREE(r);
SSH_STRING_FREE(s);
ECDSA_SIG_free(ecdsa_sig);
@@ -1698,7 +1711,11 @@ static int pki_signature_from_dsa_blob(UNUSED_PARAM(const ssh_key pubkey),
if (r == NULL) {
goto error;
}
- ssh_string_fill(r, ssh_string_data(sig_blob), 20);
+ rc = ssh_string_fill(r, ssh_string_data(sig_blob), 20);
+ if (rc < 0) {
+ SSH_STRING_FREE(r);
+ goto error;
+ }
pr = ssh_make_string_bn(r);
ssh_string_burn(r);
@@ -1711,7 +1728,11 @@ static int pki_signature_from_dsa_blob(UNUSED_PARAM(const ssh_key pubkey),
if (s == NULL) {
goto error;
}
- ssh_string_fill(s, (char *)ssh_string_data(sig_blob) + 20, 20);
+ rc = ssh_string_fill(s, (char *)ssh_string_data(sig_blob) + 20, 20);
+ if (rc < 0) {
+ SSH_STRING_FREE(s);
+ goto error;
+ }
ps = ssh_make_string_bn(s);
ssh_string_burn(s);
diff --git a/src/pki_ed25519_common.c b/src/pki_ed25519_common.c
index 9db14dac..7aa05269 100644
--- a/src/pki_ed25519_common.c
+++ b/src/pki_ed25519_common.c
@@ -214,6 +214,7 @@ int pki_ed25519_public_key_to_blob(ssh_buffer buffer, ssh_key key)
ssh_string pki_ed25519_signature_to_blob(ssh_signature sig)
{
ssh_string sig_blob;
+ int rc;
#ifdef HAVE_OPENSSL_ED25519
/* When using the OpenSSL implementation, the signature is stored in raw_sig
@@ -235,11 +236,15 @@ ssh_string pki_ed25519_signature_to_blob(ssh_signature sig)
}
#ifdef HAVE_OPENSSL_ED25519
- ssh_string_fill(sig_blob, ssh_string_data(sig->raw_sig),
- ssh_string_len(sig->raw_sig));
+ rc = ssh_string_fill(sig_blob, ssh_string_data(sig->raw_sig),
+ ssh_string_len(sig->raw_sig));
#else
- ssh_string_fill(sig_blob, sig->ed25519_sig, ED25519_SIG_LEN);
+ rc = ssh_string_fill(sig_blob, sig->ed25519_sig, ED25519_SIG_LEN);
#endif
+ if (rc < 0) {
+ SSH_STRING_FREE(sig_blob);
+ return NULL;
+ }
return sig_blob;
}
diff --git a/src/pki_gcrypt.c b/src/pki_gcrypt.c
index 0373cdae..7f8b140e 100644
--- a/src/pki_gcrypt.c
+++ b/src/pki_gcrypt.c
@@ -1781,6 +1781,7 @@ ssh_string pki_signature_to_blob(const ssh_signature sig)
gcry_sexp_t sexp;
size_t size = 0;
ssh_string sig_blob = NULL;
+ int rc;
switch(sig->type) {
case SSH_KEYTYPE_DSS:
@@ -1828,7 +1829,11 @@ ssh_string pki_signature_to_blob(const ssh_signature sig)
return NULL;
}
- ssh_string_fill(sig_blob, buffer, 40);
+ rc = ssh_string_fill(sig_blob, buffer, 40);
+ if (rc < 0) {
+ SSH_STRING_FREE(sig_blob);
+ return NULL;
+ }
break;
case SSH_KEYTYPE_RSA:
sexp = gcry_sexp_find_token(sig->rsa_sig, "s", 0);
@@ -1845,13 +1850,16 @@ ssh_string pki_signature_to_blob(const ssh_signature sig)
if (sig_blob == NULL) {
return NULL;
}
- ssh_string_fill(sig_blob, discard_const_p(char, s), size);
-
+ rc = ssh_string_fill(sig_blob, discard_const_p(char, s), size);
gcry_sexp_release(sexp);
+ if (rc < 0) {
+ SSH_STRING_FREE(sig_blob);
+ return NULL;
+ }
break;
case SSH_KEYTYPE_ED25519:
- sig_blob = pki_ed25519_signature_to_blob(sig);
- break;
+ sig_blob = pki_ed25519_signature_to_blob(sig);
+ break;
case SSH_KEYTYPE_ECDSA_P256:
case SSH_KEYTYPE_ECDSA_P384:
case SSH_KEYTYPE_ECDSA_P521:
@@ -1860,7 +1868,6 @@ ssh_string pki_signature_to_blob(const ssh_signature sig)
ssh_string R;
ssh_string S;
ssh_buffer b;
- int rc;
b = ssh_buffer_new();
if (b == NULL) {
@@ -1901,9 +1908,13 @@ ssh_string pki_signature_to_blob(const ssh_signature sig)
return NULL;
}
- ssh_string_fill(sig_blob,
+ rc = ssh_string_fill(sig_blob,
ssh_buffer_get(b), ssh_buffer_get_len(b));
SSH_BUFFER_FREE(b);
+ if (rc < 0) {
+ SSH_STRING_FREE(sig_blob);
+ return NULL;
+ }
break;
}
#endif
diff --git a/src/pki_mbedcrypto.c b/src/pki_mbedcrypto.c
index cac357f8..720fe1de 100644
--- a/src/pki_mbedcrypto.c
+++ b/src/pki_mbedcrypto.c
@@ -845,8 +845,13 @@ ssh_string pki_signature_to_blob(const ssh_signature sig)
return NULL;
}
- ssh_string_fill(sig_blob, ssh_buffer_get(b), ssh_buffer_get_len(b));
+ rc = ssh_string_fill(sig_blob, ssh_buffer_get(b), ssh_buffer_get_len(b));
SSH_BUFFER_FREE(b);
+ if (rc < 0) {
+ SSH_STRING_FREE(sig_blob);
+ return NULL;
+ }
+
break;
}
case SSH_KEYTYPE_ED25519:
@@ -1089,9 +1094,13 @@ static ssh_string rsa_do_sign_hash(const unsigned char *digest,
return NULL;
}
- ssh_string_fill(sig_blob, sig, slen);
+ ok = ssh_string_fill(sig_blob, sig, slen);
explicit_bzero(sig, slen);
SAFE_FREE(sig);
+ if (ok < 0) {
+ SSH_STRING_FREE(sig_blob);
+ return NULL;
+ }
return sig_blob;
}
diff --git a/tests/unittests/torture_pki_ed25519.c b/tests/unittests/torture_pki_ed25519.c
index 07ccfd67..ff59b190 100644
--- a/tests/unittests/torture_pki_ed25519.c
+++ b/tests/unittests/torture_pki_ed25519.c
@@ -796,7 +796,8 @@ static void torture_pki_ed25519_verify(void **state){
assert_true(rc == SSH_OK);
assert_non_null(pubkey);
- ssh_string_fill(blob, ref_signature, ED25519_SIG_LEN);
+ rc = ssh_string_fill(blob, ref_signature, ED25519_SIG_LEN);
+ assert_int_equal(rc, 0);
sig = pki_signature_from_blob(pubkey, blob, SSH_KEYTYPE_ED25519, SSH_DIGEST_AUTO);
assert_non_null(sig);
@@ -853,7 +854,8 @@ static void torture_pki_ed25519_verify_bad(void **state){
/* alter signature and expect false result */
for (i=0; i < ED25519_SIG_LEN; ++i){
- ssh_string_fill(blob, ref_signature, ED25519_SIG_LEN);
+ rc = ssh_string_fill(blob, ref_signature, ED25519_SIG_LEN);
+ assert_int_equal(rc, 0);
((uint8_t *)ssh_string_data(blob))[i] ^= 0xff;
sig = pki_signature_from_blob(pubkey, blob, SSH_KEYTYPE_ED25519, SSH_DIGEST_AUTO);
assert_non_null(sig);
diff --git a/tests/unittests/torture_session_keys.c b/tests/unittests/torture_session_keys.c
index f220e010..6ae58831 100644
--- a/tests/unittests/torture_session_keys.c
+++ b/tests/unittests/torture_session_keys.c
@@ -68,7 +68,9 @@ static void torture_session_keys(UNUSED_PARAM(void **state))
int rc;
k_string = ssh_string_new(32);
- ssh_string_fill(k_string, key, 32);
+ rc = ssh_string_fill(k_string, key, 32);
+ assert_int_equal(rc, 0);
+
test_crypto.shared_secret = ssh_make_string_bn(k_string);
rc = ssh_generate_session_keys(&session);