Markus Schmidt
2018-Dec-10 11:29 UTC
[PATCH] cleanup of global variables server/client_version_string in sshconnect.c
In sshconnect.c there are two global variables for server_version_string
client_version_string.
These are used just in a few functions and can easily be passed as
parameters.
Also, there is a strange construct, where their memory is allocated to
the global pointers, then copies of these pointers are assigned to the
kex structure. The kex_free finally frees them via cleanup of the kex
structure while the global pointers remain.
This can easily be rearranged so that is clearer who owns which memory
and who is thus responsible for freeing.
Also, the the ssh_login function leaks the memory allocated to the host
variable.
Also, there is another global variable (previous_host_key) which could
be removed by placing it into the ssh/active_state structure, but which,
for now, could at least be made module-static.
Proposal: ssh-connect allocates the initial memory and creates values
and passes these as parameters to the relevant functions. It passes
them also to ssh_kex2 which makes a true copy. ssh-connect frees its
memory and the kex system finally frees its copy.
Along the way, fix the host leak also and make previous_host_key static
because both changes are trivial.
The patch is attached. It's from the portable version but applies to
the original version with a few lines offset.
-------------- next part --------------
diff --git a/sshconnect.c b/sshconnect.c
index 6d81927..7736bc2 100644
--- a/sshconnect.c
+++ b/sshconnect.c
@@ -69,9 +69,7 @@
#include "ssherr.h"
#include "authfd.h"
-char *client_version_string = NULL;
-char *server_version_string = NULL;
-struct sshkey *previous_host_key = NULL;
+static struct sshkey *previous_host_key = NULL;
static int matching_host_key_dns = 0;
@@ -605,16 +603,16 @@ ssh_connect(struct ssh *ssh, const char *host, struct
addrinfo *addrs,
}
static void
-send_client_banner(int connection_out, int minor1)
+send_client_banner(int connection_out, int minor1, char
**client_version_stringp)
{
/* Send our own protocol version identification. */
- xasprintf(&client_version_string, "SSH-%d.%d-%.100s\r\n",
+ xasprintf(client_version_stringp, "SSH-%d.%d-%.100s\r\n",
PROTOCOL_MAJOR_2, PROTOCOL_MINOR_2, SSH_VERSION);
- if (atomicio(vwrite, connection_out, client_version_string,
- strlen(client_version_string)) != strlen(client_version_string))
+ if (atomicio(vwrite, connection_out, *client_version_stringp,
+ strlen(*client_version_stringp)) != strlen(*client_version_stringp))
fatal("write: %.100s", strerror(errno));
- chop(client_version_string);
- debug("Local version string %.100s", client_version_string);
+ chop(*client_version_stringp);
+ debug("Local version string %.100s", *client_version_stringp);
}
/*
@@ -622,7 +620,7 @@ send_client_banner(int connection_out, int minor1)
* identification string.
*/
void
-ssh_exchange_identification(int timeout_ms)
+ssh_exchange_identification(int timeout_ms, char **client_version_stringp, char
**server_version_stringp)
{
char buf[256], remote_version[256]; /* must be same size! */
int remote_major, remote_minor, mismatch;
@@ -632,7 +630,7 @@ ssh_exchange_identification(int timeout_ms)
size_t len;
int rc;
- send_client_banner(connection_out, 0);
+ send_client_banner(connection_out, 0, client_version_stringp);
/* Read other side's version identification. */
for (n = 0;;) {
@@ -673,13 +671,13 @@ ssh_exchange_identification(int timeout_ms)
break;
debug("ssh_exchange_identification: %s", buf);
}
- server_version_string = xstrdup(buf);
+ *server_version_stringp = xstrdup(buf);
/*
* Check that the versions match. In future this might accept
* several versions and set appropriate flags to handle them.
*/
- if (sscanf(server_version_string, "SSH-%d.%d-%[^\n]\n",
+ if (sscanf(*server_version_stringp, "SSH-%d.%d-%[^\n]\n",
&remote_major, &remote_minor, remote_version) != 3)
fatal("Bad remote protocol version identification:
'%.100s'", buf);
debug("Remote protocol version %d.%d, remote software version
%.100s",
@@ -705,7 +703,7 @@ ssh_exchange_identification(int timeout_ms)
if ((datafellows & SSH_BUG_RSASIGMD5) != 0)
logit("Server version \"%.100s\" uses unsafe RSA signature
"
"scheme; disabling use of RSA keys", remote_version);
- chop(server_version_string);
+ chop(*server_version_stringp);
}
/* defaults to 'no' */
@@ -1406,6 +1404,8 @@ ssh_login(Sensitive *sensitive, const char *orighost,
{
char *host;
char *server_user, *local_user;
+ char *client_version_string = NULL;
+ char *server_version_string = NULL;
local_user = xstrdup(pw->pw_name);
server_user = options.user ? options.user : local_user;
@@ -1415,7 +1415,7 @@ ssh_login(Sensitive *sensitive, const char *orighost,
lowercase(host);
/* Exchange protocol version identification strings with the server. */
- ssh_exchange_identification(timeout_ms);
+ ssh_exchange_identification(timeout_ms, &client_version_string,
&server_version_string);
/* Put the connection into non-blocking mode. */
packet_set_nonblocking();
@@ -1423,9 +1423,12 @@ ssh_login(Sensitive *sensitive, const char *orighost,
/* key exchange */
/* authenticate user */
debug("Authenticating to %s:%d as '%s'", host, port,
server_user);
- ssh_kex2(host, hostaddr, port);
+ ssh_kex2(host, hostaddr, port, client_version_string, server_version_string);
ssh_userauth2(local_user, server_user, host, sensitive);
+ free(client_version_string);
+ free(server_version_string);
free(local_user);
+ free(host);
}
void
diff --git a/sshconnect.h b/sshconnect.h
index 890d857..d6de9fe 100644
--- a/sshconnect.h
+++ b/sshconnect.h
@@ -40,7 +40,7 @@ void ssh_kill_proxy_command(void);
void ssh_login(Sensitive *, const char *, struct sockaddr *, u_short,
struct passwd *, int);
-void ssh_exchange_identification(int);
+void ssh_exchange_identification(int, char **, char **);
int verify_host_key(char *, struct sockaddr *, struct sshkey *);
@@ -48,7 +48,7 @@ void get_hostfile_hostname_ipaddr(char *, struct sockaddr *,
u_short,
char **, char **);
void ssh_kex(char *, struct sockaddr *);
-void ssh_kex2(char *, struct sockaddr *, u_short);
+void ssh_kex2(char *, struct sockaddr *, u_short, const char *, const char *);
void ssh_userauth1(const char *, const char *, char *, Sensitive *);
void ssh_userauth2(const char *, const char *, char *, Sensitive *);
diff --git a/sshconnect2.c b/sshconnect2.c
index cc527e1..8a9b6c0 100644
--- a/sshconnect2.c
+++ b/sshconnect2.c
@@ -78,8 +78,6 @@
#endif
/* import */
-extern char *client_version_string;
-extern char *server_version_string;
extern Options options;
/*
@@ -155,7 +153,7 @@ order_hostkeyalgs(char *host, struct sockaddr *hostaddr,
u_short port)
}
void
-ssh_kex2(char *host, struct sockaddr *hostaddr, u_short port)
+ssh_kex2(char *host, struct sockaddr *hostaddr, u_short port, const char
*client_version_string, const char *server_version_string)
{
char *myproposal[PROPOSAL_MAX] = { KEX_CLIENT };
char *s, *all_key;
Damien Miller
2018-Dec-14 05:29 UTC
[PATCH] cleanup of global variables server/client_version_string in sshconnect.c
On Mon, 10 Dec 2018, Markus Schmidt wrote:> > In sshconnect.c there are two global variables for server_version_string > client_version_string. > > These are used just in a few functions and can easily be passed as parameters.IMO it makes more sense to store these in struct ssh as buffers, since they have pretty common handling between the client and server and because we already need them for kex. The below diff attempts this and seems to pass regress testing ;) The next step would be to factor out the common code between the client and server banner exchange... commit 61855995885ebe418c2b53b94b4b0d437c698c3e Author: Damien Miller <djm at mindrot.org> Date: Fri Dec 14 16:19:42 2018 +1100 move client/server banners to buffers under *ssh This eliminates some global state from the client and server, and will make it easier to factor out common parts of banner processing in the future. Inspired by a patch from Markus Schmidt. Also be more strict about handling \r characters - these should only be accepted immediately before \n (pointed out by Jann Horn). diff --git a/kex.c b/kex.c index c2dab79..e543a18 100644 --- a/kex.c +++ b/kex.c @@ -641,8 +641,6 @@ kex_free(struct kex *kex) sshbuf_free(kex->peer); sshbuf_free(kex->my); free(kex->session_id); - free(kex->client_version_string); - free(kex->server_version_string); free(kex->failed_choice); free(kex->hostkey_alg); free(kex->name); diff --git a/kex.h b/kex.h index a7f916b..238395c 100644 --- a/kex.h +++ b/kex.h @@ -133,8 +133,6 @@ struct kex { u_int flags; int hash_alg; int ec_nid; - char *client_version_string; - char *server_version_string; char *failed_choice; int (*verify_host_key)(struct sshkey *, struct ssh *); struct sshkey *(*load_host_public_key)(int, int, struct ssh *); @@ -183,22 +181,23 @@ int kexecdh_server(struct ssh *); int kexc25519_client(struct ssh *); int kexc25519_server(struct ssh *); -int kex_dh_hash(int, const char *, const char *, +int kex_dh_hash(int, const struct sshbuf *, const struct sshbuf *, const u_char *, size_t, const u_char *, size_t, const u_char *, size_t, const BIGNUM *, const BIGNUM *, const BIGNUM *, u_char *, size_t *); -int kexgex_hash(int, const char *, const char *, +int kexgex_hash(int, const struct sshbuf *, const struct sshbuf *, const u_char *, size_t, const u_char *, size_t, const u_char *, size_t, int, int, int, const BIGNUM *, const BIGNUM *, const BIGNUM *, const BIGNUM *, const BIGNUM *, u_char *, size_t *); -int kex_ecdh_hash(int, const EC_GROUP *, const char *, const char *, +int kex_ecdh_hash(int, const EC_GROUP *, + const struct sshbuf *, const struct sshbuf *, const u_char *, size_t, const u_char *, size_t, const u_char *, size_t, const EC_POINT *, const EC_POINT *, const BIGNUM *, u_char *, size_t *); -int kex_c25519_hash(int, const char *, const char *, +int kex_c25519_hash(int, const struct sshbuf *, const struct sshbuf *, const u_char *, size_t, const u_char *, size_t, const u_char *, size_t, const u_char *, const u_char *, const u_char *, size_t, u_char *, size_t *); diff --git a/kexc25519.c b/kexc25519.c index 86158c5..e7c9ad1 100644 --- a/kexc25519.c +++ b/kexc25519.c @@ -82,8 +82,8 @@ kexc25519_shared_key(const u_char key[CURVE25519_SIZE], int kex_c25519_hash( int hash_alg, - const char *client_version_string, - const char *server_version_string, + const struct sshbuf *client_banner, + const struct sshbuf *server_banner, const u_char *ckexinit, size_t ckexinitlen, const u_char *skexinit, size_t skexinitlen, const u_char *serverhostkeyblob, size_t sbloblen, @@ -99,8 +99,8 @@ kex_c25519_hash( return SSH_ERR_INVALID_ARGUMENT; if ((b = sshbuf_new()) == NULL) return SSH_ERR_ALLOC_FAIL; - if ((r = sshbuf_put_cstring(b, client_version_string)) < 0 || - (r = sshbuf_put_cstring(b, server_version_string)) < 0 || + if ((r = sshbuf_put_stringb(b, client_banner)) < 0 || + (r = sshbuf_put_stringb(b, server_banner)) < 0 || /* kexinit messages: fake header: len+SSH2_MSG_KEXINIT */ (r = sshbuf_put_u32(b, ckexinitlen+1)) < 0 || (r = sshbuf_put_u8(b, SSH2_MSG_KEXINIT)) < 0 || diff --git a/kexc25519c.c b/kexc25519c.c index 0d72f8f..dd3a324 100644 --- a/kexc25519c.c +++ b/kexc25519c.c @@ -127,8 +127,8 @@ input_kex_c25519_reply(int type, u_int32_t seq, struct ssh *ssh) hashlen = sizeof(hash); if ((r = kex_c25519_hash( kex->hash_alg, - kex->client_version_string, - kex->server_version_string, + ssh->client_banner, + ssh->server_banner, sshbuf_ptr(kex->my), sshbuf_len(kex->my), sshbuf_ptr(kex->peer), sshbuf_len(kex->peer), server_host_key_blob, sbloblen, diff --git a/kexc25519s.c b/kexc25519s.c index 83fcf73..aff1bca 100644 --- a/kexc25519s.c +++ b/kexc25519s.c @@ -108,8 +108,8 @@ input_kex_c25519_init(int type, u_int32_t seq, struct ssh *ssh) hashlen = sizeof(hash); if ((r = kex_c25519_hash( kex->hash_alg, - kex->client_version_string, - kex->server_version_string, + ssh->client_banner, + ssh->server_banner, sshbuf_ptr(kex->peer), sshbuf_len(kex->peer), sshbuf_ptr(kex->my), sshbuf_len(kex->my), server_host_key_blob, sbloblen, diff --git a/kexdh.c b/kexdh.c index f55bf8c..7b72460 100644 --- a/kexdh.c +++ b/kexdh.c @@ -40,8 +40,8 @@ int kex_dh_hash( int hash_alg, - const char *client_version_string, - const char *server_version_string, + const struct sshbuf *client_banner, + const struct sshbuf *server_banner, const u_char *ckexinit, size_t ckexinitlen, const u_char *skexinit, size_t skexinitlen, const u_char *serverhostkeyblob, size_t sbloblen, @@ -57,8 +57,8 @@ kex_dh_hash( return SSH_ERR_INVALID_ARGUMENT; if ((b = sshbuf_new()) == NULL) return SSH_ERR_ALLOC_FAIL; - if ((r = sshbuf_put_cstring(b, client_version_string)) != 0 || - (r = sshbuf_put_cstring(b, server_version_string)) != 0 || + if ((r = sshbuf_put_stringb(b, client_banner)) < 0 || + (r = sshbuf_put_stringb(b, server_banner)) < 0 || /* kexinit messages: fake header: len+SSH2_MSG_KEXINIT */ (r = sshbuf_put_u32(b, ckexinitlen+1)) != 0 || (r = sshbuf_put_u8(b, SSH2_MSG_KEXINIT)) != 0 || diff --git a/kexdhc.c b/kexdhc.c index 4cfbdfb..b538d92 100644 --- a/kexdhc.c +++ b/kexdhc.c @@ -171,8 +171,8 @@ input_kex_dh(int type, u_int32_t seq, struct ssh *ssh) hashlen = sizeof(hash); if ((r = kex_dh_hash( kex->hash_alg, - kex->client_version_string, - kex->server_version_string, + ssh->client_banner, + ssh->server_banner, sshbuf_ptr(kex->my), sshbuf_len(kex->my), sshbuf_ptr(kex->peer), sshbuf_len(kex->peer), server_host_key_blob, sbloblen, diff --git a/kexdhs.c b/kexdhs.c index acd2224..68dc027 100644 --- a/kexdhs.c +++ b/kexdhs.c @@ -159,8 +159,8 @@ input_kex_dh_init(int type, u_int32_t seq, struct ssh *ssh) hashlen = sizeof(hash); if ((r = kex_dh_hash( kex->hash_alg, - kex->client_version_string, - kex->server_version_string, + ssh->client_banner, + ssh->server_banner, sshbuf_ptr(kex->peer), sshbuf_len(kex->peer), sshbuf_ptr(kex->my), sshbuf_len(kex->my), server_host_key_blob, sbloblen, diff --git a/kexecdh.c b/kexecdh.c index 3898498..3afa53a 100644 --- a/kexecdh.c +++ b/kexecdh.c @@ -46,8 +46,8 @@ int kex_ecdh_hash( int hash_alg, const EC_GROUP *ec_group, - const char *client_version_string, - const char *server_version_string, + const struct sshbuf *client_banner, + const struct sshbuf *server_banner, const u_char *ckexinit, size_t ckexinitlen, const u_char *skexinit, size_t skexinitlen, const u_char *serverhostkeyblob, size_t sbloblen, @@ -63,8 +63,8 @@ kex_ecdh_hash( return SSH_ERR_INVALID_ARGUMENT; if ((b = sshbuf_new()) == NULL) return SSH_ERR_ALLOC_FAIL; - if ((r = sshbuf_put_cstring(b, client_version_string)) != 0 || - (r = sshbuf_put_cstring(b, server_version_string)) != 0 || + if ((r = sshbuf_put_stringb(b, client_banner)) < 0 || + (r = sshbuf_put_stringb(b, server_banner)) < 0 || /* kexinit messages: fake header: len+SSH2_MSG_KEXINIT */ (r = sshbuf_put_u32(b, ckexinitlen+1)) != 0 || (r = sshbuf_put_u8(b, SSH2_MSG_KEXINIT)) != 0 || diff --git a/kexecdhc.c b/kexecdhc.c index 0ff264b..6a81ab9 100644 --- a/kexecdhc.c +++ b/kexecdhc.c @@ -171,8 +171,8 @@ input_kex_ecdh_reply(int type, u_int32_t seq, struct ssh *ssh) if ((r = kex_ecdh_hash( kex->hash_alg, group, - kex->client_version_string, - kex->server_version_string, + ssh->client_banner, + ssh->server_banner, sshbuf_ptr(kex->my), sshbuf_len(kex->my), sshbuf_ptr(kex->peer), sshbuf_len(kex->peer), server_host_key_blob, sbloblen, diff --git a/kexecdhs.c b/kexecdhs.c index 89201c7..ab3fe4f 100644 --- a/kexecdhs.c +++ b/kexecdhs.c @@ -141,8 +141,8 @@ input_kex_ecdh_init(int type, u_int32_t seq, struct ssh *ssh) if ((r = kex_ecdh_hash( kex->hash_alg, group, - kex->client_version_string, - kex->server_version_string, + ssh->client_banner, + ssh->server_banner, sshbuf_ptr(kex->peer), sshbuf_len(kex->peer), sshbuf_ptr(kex->my), sshbuf_len(kex->my), server_host_key_blob, sbloblen, diff --git a/kexgex.c b/kexgex.c index 1aab51b..25db907 100644 --- a/kexgex.c +++ b/kexgex.c @@ -40,8 +40,8 @@ int kexgex_hash( int hash_alg, - const char *client_version_string, - const char *server_version_string, + const struct sshbuf *client_banner, + const struct sshbuf *server_banner, const u_char *ckexinit, size_t ckexinitlen, const u_char *skexinit, size_t skexinitlen, const u_char *serverhostkeyblob, size_t sbloblen, @@ -60,8 +60,8 @@ kexgex_hash( return SSH_ERR_INVALID_ARGUMENT; if ((b = sshbuf_new()) == NULL) return SSH_ERR_ALLOC_FAIL; - if ((r = sshbuf_put_cstring(b, client_version_string)) != 0 || - (r = sshbuf_put_cstring(b, server_version_string)) != 0 || + if ((r = sshbuf_put_stringb(b, client_banner)) < 0 || + (r = sshbuf_put_stringb(b, server_banner)) < 0 || /* kexinit messages: fake header: len+SSH2_MSG_KEXINIT */ (r = sshbuf_put_u32(b, ckexinitlen+1)) != 0 || (r = sshbuf_put_u8(b, SSH2_MSG_KEXINIT)) != 0 || diff --git a/kexgexc.c b/kexgexc.c index b7ea468..07ddabe 100644 --- a/kexgexc.c +++ b/kexgexc.c @@ -215,8 +215,8 @@ input_kex_dh_gex_reply(int type, u_int32_t seq, struct ssh *ssh) hashlen = sizeof(hash); if ((r = kexgex_hash( kex->hash_alg, - kex->client_version_string, - kex->server_version_string, + ssh->client_banner, + ssh->server_banner, sshbuf_ptr(kex->my), sshbuf_len(kex->my), sshbuf_ptr(kex->peer), sshbuf_len(kex->peer), server_host_key_blob, sbloblen, diff --git a/kexgexs.c b/kexgexs.c index fe66c3c..927671f 100644 --- a/kexgexs.c +++ b/kexgexs.c @@ -191,8 +191,8 @@ input_kex_dh_gex_init(int type, u_int32_t seq, struct ssh *ssh) hashlen = sizeof(hash); if ((r = kexgex_hash( kex->hash_alg, - kex->client_version_string, - kex->server_version_string, + ssh->client_banner, + ssh->server_banner, sshbuf_ptr(kex->peer), sshbuf_len(kex->peer), sshbuf_ptr(kex->my), sshbuf_len(kex->my), server_host_key_blob, sbloblen, diff --git a/packet.c b/packet.c index ab9b198..572ca12 100644 --- a/packet.c +++ b/packet.c @@ -210,6 +210,8 @@ ssh_alloc_session_state(void) if ((ssh = calloc(1, sizeof(*ssh))) == NULL || (state = calloc(1, sizeof(*state))) == NULL || + (ssh->client_banner = sshbuf_new()) == NULL || + (ssh->server_banner = sshbuf_new()) == NULL || (state->input = sshbuf_new()) == NULL || (state->output = sshbuf_new()) == NULL || (state->outgoing_packet = sshbuf_new()) == NULL || @@ -232,6 +234,11 @@ ssh_alloc_session_state(void) ssh->state = state; return ssh; fail: + if (ssh) { + sshbuf_free(ssh->client_banner); + sshbuf_free(ssh->server_banner); + free(ssh); + } if (state) { sshbuf_free(state->input); sshbuf_free(state->output); @@ -239,7 +246,6 @@ ssh_alloc_session_state(void) sshbuf_free(state->outgoing_packet); free(state); } - free(ssh); return NULL; } @@ -2143,9 +2149,7 @@ kex_to_blob(struct sshbuf *m, struct kex *kex) (r = sshbuf_put_u32(m, kex->kex_type)) != 0 || (r = sshbuf_put_stringb(m, kex->my)) != 0 || (r = sshbuf_put_stringb(m, kex->peer)) != 0 || - (r = sshbuf_put_u32(m, kex->flags)) != 0 || - (r = sshbuf_put_cstring(m, kex->client_version_string)) != 0 || - (r = sshbuf_put_cstring(m, kex->server_version_string)) != 0) + (r = sshbuf_put_u32(m, kex->flags)) != 0) return r; return 0; } @@ -2215,7 +2219,9 @@ ssh_packet_get_state(struct ssh *ssh, struct sshbuf *m) (r = sshbuf_put_u32(m, state->p_read.packets)) != 0 || (r = sshbuf_put_u64(m, state->p_read.bytes)) != 0 || (r = sshbuf_put_stringb(m, state->input)) != 0 || - (r = sshbuf_put_stringb(m, state->output)) != 0) + (r = sshbuf_put_stringb(m, state->output)) != 0 || + (r = sshbuf_put_stringb(m, ssh->client_banner)) != 0 || + (r = sshbuf_put_stringb(m, ssh->server_banner)) != 0) return r; return 0; @@ -2309,9 +2315,7 @@ kex_from_blob(struct sshbuf *m, struct kex **kexp) (r = sshbuf_get_u32(m, &kex->kex_type)) != 0 || (r = sshbuf_get_stringb(m, kex->my)) != 0 || (r = sshbuf_get_stringb(m, kex->peer)) != 0 || - (r = sshbuf_get_u32(m, &kex->flags)) != 0 || - (r = sshbuf_get_cstring(m, &kex->client_version_string, NULL)) != 0 || - (r = sshbuf_get_cstring(m, &kex->server_version_string, NULL)) != 0) + (r = sshbuf_get_u32(m, &kex->flags)) != 0) goto out; kex->server = 1; kex->done = 1; @@ -2339,8 +2343,8 @@ int ssh_packet_set_state(struct ssh *ssh, struct sshbuf *m) { struct session_state *state = ssh->state; - const u_char *input, *output; - size_t ilen, olen; + const u_char *input, *output, *cbanner, *sbanner; + size_t ilen, olen, cblen, sblen; int r; if ((r = kex_from_blob(m, &ssh->kex)) != 0 || @@ -2372,10 +2376,16 @@ ssh_packet_set_state(struct ssh *ssh, struct sshbuf *m) sshbuf_reset(state->input); sshbuf_reset(state->output); + sshbuf_reset(ssh->client_banner); + sshbuf_reset(ssh->server_banner); if ((r = sshbuf_get_string_direct(m, &input, &ilen)) != 0 || (r = sshbuf_get_string_direct(m, &output, &olen)) != 0 || + (r = sshbuf_get_string_direct(m, &cbanner, &cblen)) != 0 || + (r = sshbuf_get_string_direct(m, &sbanner, &sblen)) != 0 || (r = sshbuf_put(state->input, input, ilen)) != 0 || - (r = sshbuf_put(state->output, output, olen)) != 0) + (r = sshbuf_put(state->output, output, olen)) != 0 || + (r = sshbuf_put(ssh->client_banner, cbanner, cblen)) != 0 || + (r = sshbuf_put(ssh->server_banner, sbanner, sblen)) != 0) return r; if (sshbuf_len(m)) diff --git a/packet.h b/packet.h index 1c67b9f..c0a72a0 100644 --- a/packet.h +++ b/packet.h @@ -40,6 +40,7 @@ struct ssh { struct session_state *state; /* Key exchange */ + struct sshbuf *client_banner, *server_banner; struct kex *kex; /* cached local and remote ip addresses and ports */ diff --git a/ssh.c b/ssh.c index 3e815de..ead1395 100644 --- a/ssh.c +++ b/ssh.c @@ -1463,7 +1463,7 @@ main(int ac, char **av) signal(SIGCHLD, main_sigchld_handler); /* Log into the remote system. Never returns if the login fails. */ - ssh_login(&sensitive_data, host, (struct sockaddr *)&hostaddr, + ssh_login(ssh, &sensitive_data, host, (struct sockaddr *)&hostaddr, options.port, pw, timeout_ms); if (packet_connection_is_on_socket()) { diff --git a/ssh.h b/ssh.h index 84fddee..cb8e893 100644 --- a/ssh.h +++ b/ssh.h @@ -91,3 +91,7 @@ /* Listen backlog for sshd, ssh-agent and forwarding sockets */ #define SSH_LISTEN_BACKLOG 128 + +/* Limits for banner exchange */ +#define SSH_MAX_BANNER_LEN 8192 +#define SSH_MAX_PRE_BANNER_LINES 1024 diff --git a/ssh_api.c b/ssh_api.c index 173d632..6cc108d 100644 --- a/ssh_api.c +++ b/ssh_api.c @@ -30,8 +30,8 @@ #include <string.h> int _ssh_exchange_banner(struct ssh *); -int _ssh_send_banner(struct ssh *, char **); -int _ssh_read_banner(struct ssh *, char **); +int _ssh_send_banner(struct ssh *, struct sshbuf *); +int _ssh_read_banner(struct ssh *, struct sshbuf *); int _ssh_order_hostkeyalgs(struct ssh *); int _ssh_verify_host_key(struct sshkey *, struct ssh *); struct sshkey *_ssh_host_public_key(int, int, struct ssh *); @@ -228,8 +228,8 @@ ssh_packet_next(struct ssh *ssh, u_char *typep) * enough data. */ *typep = SSH_MSG_NONE; - if (ssh->kex->client_version_string == NULL || - ssh->kex->server_version_string == NULL) + if (sshbuf_len(ssh->client_banner) == 0 || + sshbuf_len(ssh->server_banner) == 0) return _ssh_exchange_banner(ssh); /* * If we enough data and a dispatch function then @@ -304,39 +304,46 @@ ssh_input_space(struct ssh *ssh, size_t len) /* Read other side's version identification. */ int -_ssh_read_banner(struct ssh *ssh, char **bannerp) +_ssh_read_banner(struct ssh *ssh, struct sshbuf *banner) { - struct sshbuf *input; - const char *s; - char buf[256], remote_version[256]; /* must be same size! */ + struct sshbuf *input = ssh_packet_get_input(ssh); const char *mismatch = "Protocol mismatch.\r\n"; - int r, remote_major, remote_minor; - size_t i, n, j, len; + const u_char *s = sshbuf_ptr(input); + u_char c; + char *cp, *remote_version; + int r, remote_major, remote_minor, expect_nl; + size_t n, j; - *bannerp = NULL; - input = ssh_packet_get_input(ssh); - len = sshbuf_len(input); - s = (const char *)sshbuf_ptr(input); for (j = n = 0;;) { - for (i = 0; i < sizeof(buf) - 1; i++) { - if (j >= len) - return (0); - buf[i] = s[j++]; - if (buf[i] == '\r') { - buf[i] = '\n'; - buf[i + 1] = 0; - continue; /**XXX wait for \n */ + sshbuf_reset(banner); + expect_nl = 0; + for (;;) { + if (j >= sshbuf_len(input)) + return 0; /* insufficient data in input buf */ + c = s[j++]; + if (c == '\r') { + expect_nl = 1; + continue; } - if (buf[i] == '\n') { - buf[i + 1] = 0; + if (c == '\n') break; - } + if (expect_nl) + goto bad; + if ((r = sshbuf_put_u8(banner, c)) != 0) + return r; + if (sshbuf_len(banner) > SSH_MAX_BANNER_LEN) + goto bad; } - buf[sizeof(buf) - 1] = 0; - if (strncmp(buf, "SSH-", 4) == 0) + if (sshbuf_len(banner) >= 4 && + memcmp(sshbuf_ptr(banner), "SSH-", 4) == 0) break; - debug("ssh_exchange_identification: %s", buf); - if (ssh->kex->server || ++n > 65536) { + if ((cp = sshbuf_dup_string(banner)) == NULL) + return SSH_ERR_ALLOC_FAIL; + debug("%s: %s", __func__, cp); + free(cp); + /* Accept lines before banner only on client */ + if (ssh->kex->server || ++n > SSH_MAX_PRE_BANNER_LINES) { + bad: if ((r = sshbuf_put(ssh_packet_get_output(ssh), mismatch, strlen(mismatch))) != 0) return r; @@ -346,11 +353,17 @@ _ssh_read_banner(struct ssh *ssh, char **bannerp) if ((r = sshbuf_consume(input, j)) != 0) return r; + if ((cp = sshbuf_dup_string(banner)) == NULL) + return SSH_ERR_ALLOC_FAIL; + /* XXX remote version must be the same size as banner for sscanf */ + if ((remote_version = calloc(1, sshbuf_len(banner))) == NULL) + return SSH_ERR_ALLOC_FAIL; + /* * Check that the versions match. In future this might accept * several versions and set appropriate flags to handle them. */ - if (sscanf(buf, "SSH-%d.%d-%[^\n]\n", + if (sscanf(cp, "SSH-%d.%d-%[^\n]\n", &remote_major, &remote_minor, remote_version) != 3) return SSH_ERR_INVALID_FORMAT; debug("Remote protocol version %d.%d, remote software version %.100s", @@ -363,27 +376,29 @@ _ssh_read_banner(struct ssh *ssh, char **bannerp) } if (remote_major != 2) return SSH_ERR_PROTOCOL_MISMATCH; - chop(buf); - debug("Remote version string %.100s", buf); - if ((*bannerp = strdup(buf)) == NULL) - return SSH_ERR_ALLOC_FAIL; + debug("Remote version string %.100s", cp); + free(cp); return 0; } /* Send our own protocol version identification. */ int -_ssh_send_banner(struct ssh *ssh, char **bannerp) +_ssh_send_banner(struct ssh *ssh, struct sshbuf *banner) { - char buf[256]; + char *cp; int r; - snprintf(buf, sizeof buf, "SSH-2.0-%.100s\r\n", SSH_VERSION); - if ((r = sshbuf_put(ssh_packet_get_output(ssh), buf, strlen(buf))) != 0) + if ((r = sshbuf_putf(banner, "SSH-2.0-%.100s\r\n", SSH_VERSION)) != 0) return r; - chop(buf); - debug("Local version string %.100s", buf); - if ((*bannerp = strdup(buf)) == NULL) + if ((r = sshbuf_putb(ssh_packet_get_output(ssh), banner)) != 0) + return r; + /* Remove trailing \r\n */ + if ((r = sshbuf_consume_end(banner, 2)) != 0) + return r; + if ((cp = sshbuf_dup_string(banner)) == NULL) return SSH_ERR_ALLOC_FAIL; + debug("Local version string %.100s", cp); + free(cp); return 0; } @@ -400,25 +415,25 @@ _ssh_exchange_banner(struct ssh *ssh) r = 0; if (kex->server) { - if (kex->server_version_string == NULL) - r = _ssh_send_banner(ssh, &kex->server_version_string); + if (sshbuf_len(ssh->server_banner) == 0) + r = _ssh_send_banner(ssh, ssh->server_banner); if (r == 0 && - kex->server_version_string != NULL && - kex->client_version_string == NULL) - r = _ssh_read_banner(ssh, &kex->client_version_string); + sshbuf_len(ssh->server_banner) != 0 && + sshbuf_len(ssh->client_banner) == 0) + r = _ssh_read_banner(ssh, ssh->client_banner); } else { - if (kex->server_version_string == NULL) - r = _ssh_read_banner(ssh, &kex->server_version_string); + if (sshbuf_len(ssh->server_banner) == 0) + r = _ssh_read_banner(ssh, ssh->server_banner); if (r == 0 && - kex->server_version_string != NULL && - kex->client_version_string == NULL) - r = _ssh_send_banner(ssh, &kex->client_version_string); + sshbuf_len(ssh->server_banner) != 0 && + sshbuf_len(ssh->client_banner) == 0) + r = _ssh_send_banner(ssh, ssh->client_banner); } if (r != 0) return r; /* start initial kex as soon as we have exchanged the banners */ - if (kex->server_version_string != NULL && - kex->client_version_string != NULL) { + if (sshbuf_len(ssh->server_banner) != 0 && + sshbuf_len(ssh->client_banner) != 0) { if ((r = _ssh_order_hostkeyalgs(ssh)) != 0 || (r = kex_send_kexinit(ssh)) != 0) return r; diff --git a/sshconnect.c b/sshconnect.c index ecf8130..a6783b7 100644 --- a/sshconnect.c +++ b/sshconnect.c @@ -56,8 +56,6 @@ #include "ssherr.h" #include "authfd.h" -char *client_version_string = NULL; -char *server_version_string = NULL; struct sshkey *previous_host_key = NULL; static int matching_host_key_dns = 0; @@ -606,39 +604,47 @@ ssh_connect(struct ssh *ssh, const char *host, struct addrinfo *addrs, return ssh_proxy_connect(ssh, host, port, options.proxy_command); } -static void -send_client_banner(int connection_out, int minor1) -{ - /* Send our own protocol version identification. */ - xasprintf(&client_version_string, "SSH-%d.%d-%.100s\r\n", - PROTOCOL_MAJOR_2, PROTOCOL_MINOR_2, SSH_VERSION); - if (atomicio(vwrite, connection_out, client_version_string, - strlen(client_version_string)) != strlen(client_version_string)) - fatal("write: %.100s", strerror(errno)); - chop(client_version_string); - debug("Local version string %.100s", client_version_string); -} - /* * Waits for the server identification string, and sends our own * identification string. */ -void -ssh_exchange_identification(int timeout_ms) +static void +ssh_exchange_identification(struct ssh *ssh, int timeout_ms) { - char buf[256], remote_version[256]; /* must be same size! */ + char *client_version_string, *server_version_string; + char *cp, *remote_version; int remote_major, remote_minor, mismatch; - int connection_in = packet_get_connection_in(); - int connection_out = packet_get_connection_out(); + int connection_in = ssh_packet_get_connection_in(ssh); + int connection_out = ssh_packet_get_connection_out(ssh); u_int i, n; size_t len; - int rc; + int r, rc, expect_nl; + u_char c; - send_client_banner(connection_out, 0); + /* XXX factor out code common with sshd_exchange_identification */ + + /* Prepare and send client banner */ + sshbuf_reset(ssh->client_banner); + if ((r = sshbuf_putf(ssh->client_banner, "SSH-%d.%d-%.100s\r\n", + PROTOCOL_MAJOR_2, PROTOCOL_MINOR_2, SSH_VERSION)) != 0) + fatal("%s: sshbuf_putf: %s", __func__, ssh_err(r)); + if (atomicio(vwrite, connection_out, + sshbuf_mutable_ptr(ssh->client_banner), + sshbuf_len(ssh->client_banner)) != sshbuf_len(ssh->client_banner)) + fatal("%s: write: %.100s", __func__, strerror(errno)); + /* chop off \r\n */ + if ((r = sshbuf_consume_end(ssh->client_banner, 2)) != 0) + fatal("%s: sshbuf_consume_end: %s", __func__, ssh_err(r)); + client_version_string = sshbuf_dup_string(ssh->client_banner); + if (client_version_string == NULL) + fatal("%s: sshbuf_dup_string failed", __func__); + debug("Local version string %.100s", client_version_string); /* Read other side's version identification. */ - for (n = 0;;) { - for (i = 0; i < sizeof(buf) - 1; i++) { + for (n = 0; ; n++) { + sshbuf_reset(ssh->server_banner); + expect_nl = 0; + for (i = 0; ; i++) { if (timeout_ms > 0) { rc = waitrfd(connection_in, &timeout_ms); if (rc == -1 && errno == ETIMEDOUT) { @@ -650,46 +656,67 @@ ssh_exchange_identification(int timeout_ms) } } - len = atomicio(read, connection_in, &buf[i], 1); - if (len != 1 && errno == EPIPE) - fatal("ssh_exchange_identification: " - "Connection closed by remote host"); - else if (len != 1) - fatal("ssh_exchange_identification: " - "read: %.100s", strerror(errno)); - if (buf[i] == '\r') { - buf[i] = '\n'; - buf[i + 1] = 0; - continue; /**XXX wait for \n */ + len = atomicio(read, connection_in, &c, 1); + if (len != 1 && errno == EPIPE) { + fatal("%s: Connection closed by remote host", + __func__); + } else if (len != 1) { + fatal("%s: read: %.100s", + __func__, strerror(errno)); } - if (buf[i] == '\n') { - buf[i + 1] = 0; + if (c == '\r') { + expect_nl = 1; + continue; + } + if (c == '\n') break; + if (expect_nl) { + fatal("%s: banner line contains interior " + "carriage return character", __func__); } - if (++n > 65536) - fatal("ssh_exchange_identification: " - "No banner received"); + if ((r = sshbuf_put_u8(ssh->server_banner, c)) != 0) { + fatal("%s: sshbuf_put: %s", + __func__, ssh_err(r)); + } + if (sshbuf_len(ssh->server_banner) > SSH_MAX_BANNER_LEN) + fatal("%s: banner line too long", __func__); } - buf[sizeof(buf) - 1] = 0; - if (strncmp(buf, "SSH-", 4) == 0) + /* Is this an actual protocol banner? */ + if (sshbuf_len(ssh->server_banner) > 4 && + memcmp(sshbuf_ptr(ssh->server_banner), "SSH-", 4) == 0) break; - debug("ssh_exchange_identification: %s", buf); + + /* If not, then just log the line and continue */ + if ((cp = sshbuf_dup_string(ssh->server_banner)) == NULL) + fatal("%s: sshbuf_dup_string failed", __func__); + debug("%s: pre-banner line %u: %s", __func__, n, cp); + free(cp); + if (n > SSH_MAX_PRE_BANNER_LINES) { + fatal("%s: No banner received in first %u lines " + "from server", __func__, SSH_MAX_PRE_BANNER_LINES); + } } - server_version_string = xstrdup(buf); + server_version_string = sshbuf_dup_string(ssh->server_banner); + if (server_version_string == NULL) + fatal("%s: sshbuf_dup_string failed", __func__); + /* XXX must be same size for sscanf */ + remote_version = xcalloc(1, sshbuf_len(ssh->server_banner)); /* * Check that the versions match. In future this might accept * several versions and set appropriate flags to handle them. */ if (sscanf(server_version_string, "SSH-%d.%d-%[^\n]\n", - &remote_major, &remote_minor, remote_version) != 3) - fatal("Bad remote protocol version identification: '%.100s'", buf); + &remote_major, &remote_minor, remote_version) != 3) { + fatal("Bad remote protocol version identification: '%.100s'", + server_version_string); + } debug("Remote protocol version %d.%d, remote software version %.100s", remote_major, remote_minor, remote_version); - active_state->compat = compat_datafellows(remote_version); + ssh->compat = compat_datafellows(remote_version); + mismatch = 0; - switch (remote_major) { case 2: break; @@ -707,7 +734,9 @@ ssh_exchange_identification(int timeout_ms) if ((datafellows & SSH_BUG_RSASIGMD5) != 0) logit("Server version \"%.100s\" uses unsafe RSA signature " "scheme; disabling use of RSA keys", remote_version); - chop(server_version_string); + free(client_version_string); + free(server_version_string); + free(remote_version); } /* defaults to 'no' */ @@ -1387,7 +1416,7 @@ out: * This function does not require super-user privileges. */ void -ssh_login(Sensitive *sensitive, const char *orighost, +ssh_login(struct ssh *ssh, Sensitive *sensitive, const char *orighost, struct sockaddr *hostaddr, u_short port, struct passwd *pw, int timeout_ms) { char *host; @@ -1401,16 +1430,16 @@ ssh_login(Sensitive *sensitive, const char *orighost, lowercase(host); /* Exchange protocol version identification strings with the server. */ - ssh_exchange_identification(timeout_ms); + ssh_exchange_identification(ssh, timeout_ms); /* Put the connection into non-blocking mode. */ - packet_set_nonblocking(); + ssh_packet_set_nonblocking(ssh); /* key exchange */ /* authenticate user */ debug("Authenticating to %s:%d as '%s'", host, port, server_user); - ssh_kex2(host, hostaddr, port); - ssh_userauth2(local_user, server_user, host, sensitive); + ssh_kex2(ssh, host, hostaddr, port); + ssh_userauth2(ssh, local_user, server_user, host, sensitive); free(local_user); } diff --git a/sshconnect.h b/sshconnect.h index 890d857..b26c287 100644 --- a/sshconnect.h +++ b/sshconnect.h @@ -37,21 +37,18 @@ int ssh_connect(struct ssh *, const char *, struct addrinfo *, struct sockaddr_storage *, u_short, int, int, int *, int); void ssh_kill_proxy_command(void); -void ssh_login(Sensitive *, const char *, struct sockaddr *, u_short, - struct passwd *, int); - -void ssh_exchange_identification(int); +void ssh_login(struct ssh *, Sensitive *, const char *, + struct sockaddr *, u_short, struct passwd *, int); int verify_host_key(char *, struct sockaddr *, struct sshkey *); void get_hostfile_hostname_ipaddr(char *, struct sockaddr *, u_short, char **, char **); -void ssh_kex(char *, struct sockaddr *); -void ssh_kex2(char *, struct sockaddr *, u_short); +void ssh_kex2(struct ssh *ssh, char *, struct sockaddr *, u_short); -void ssh_userauth1(const char *, const char *, char *, Sensitive *); -void ssh_userauth2(const char *, const char *, char *, Sensitive *); +void ssh_userauth2(struct ssh *ssh, const char *, const char *, + char *, Sensitive *); void ssh_put_password(char *); int ssh_local_cmd(const char *); diff --git a/sshconnect2.c b/sshconnect2.c index b3c1cff..eed0c1d 100644 --- a/sshconnect2.c +++ b/sshconnect2.c @@ -149,11 +149,10 @@ order_hostkeyalgs(char *host, struct sockaddr *hostaddr, u_short port) } void -ssh_kex2(char *host, struct sockaddr *hostaddr, u_short port) +ssh_kex2(struct ssh *ssh, char *host, struct sockaddr *hostaddr, u_short port) { char *myproposal[PROPOSAL_MAX] = { KEX_CLIENT }; char *s, *all_key; - struct kex *kex; int r; xxx_host = host; @@ -193,34 +192,31 @@ ssh_kex2(char *host, struct sockaddr *hostaddr, u_short port) options.rekey_interval); /* start key exchange */ - if ((r = kex_setup(active_state, myproposal)) != 0) + if ((r = kex_setup(ssh, myproposal)) != 0) fatal("kex_setup: %s", ssh_err(r)); - kex = active_state->kex; #ifdef WITH_OPENSSL - kex->kex[KEX_DH_GRP1_SHA1] = kexdh_client; - kex->kex[KEX_DH_GRP14_SHA1] = kexdh_client; - kex->kex[KEX_DH_GRP14_SHA256] = kexdh_client; - kex->kex[KEX_DH_GRP16_SHA512] = kexdh_client; - kex->kex[KEX_DH_GRP18_SHA512] = kexdh_client; - kex->kex[KEX_DH_GEX_SHA1] = kexgex_client; - kex->kex[KEX_DH_GEX_SHA256] = kexgex_client; - kex->kex[KEX_ECDH_SHA2] = kexecdh_client; + ssh->kex->kex[KEX_DH_GRP1_SHA1] = kexdh_client; + ssh->kex->kex[KEX_DH_GRP14_SHA1] = kexdh_client; + ssh->kex->kex[KEX_DH_GRP14_SHA256] = kexdh_client; + ssh->kex->kex[KEX_DH_GRP16_SHA512] = kexdh_client; + ssh->kex->kex[KEX_DH_GRP18_SHA512] = kexdh_client; + ssh->kex->kex[KEX_DH_GEX_SHA1] = kexgex_client; + ssh->kex->kex[KEX_DH_GEX_SHA256] = kexgex_client; + ssh->kex->kex[KEX_ECDH_SHA2] = kexecdh_client; #endif - kex->kex[KEX_C25519_SHA256] = kexc25519_client; - kex->client_version_string=client_version_string; - kex->server_version_string=server_version_string; - kex->verify_host_key=&verify_host_key_callback; + ssh->kex->kex[KEX_C25519_SHA256] = kexc25519_client; + ssh->kex->verify_host_key=&verify_host_key_callback; - ssh_dispatch_run_fatal(active_state, DISPATCH_BLOCK, &kex->done); + ssh_dispatch_run_fatal(ssh, DISPATCH_BLOCK, &ssh->kex->done); /* remove ext-info from the KEX proposals for rekeying */ myproposal[PROPOSAL_KEX_ALGS] compat_kex_proposal(options.kex_algorithms); - if ((r = kex_prop2buf(kex->my, myproposal)) != 0) + if ((r = kex_prop2buf(ssh->kex->my, myproposal)) != 0) fatal("kex_prop2buf: %s", ssh_err(r)); - session_id2 = kex->session_id; - session_id2_len = kex->session_id_len; + session_id2 = ssh->kex->session_id; + session_id2_len = ssh->kex->session_id_len; #ifdef DEBUG_KEXDH /* send 1st encrypted/maced/compressed message */ @@ -357,10 +353,9 @@ Authmethod authmethods[] = { }; void -ssh_userauth2(const char *local_user, const char *server_user, char *host, - Sensitive *sensitive) +ssh_userauth2(struct ssh *ssh, const char *local_user, + const char *server_user, char *host, Sensitive *sensitive) { - struct ssh *ssh = active_state; Authctxt authctxt; int r; @@ -384,8 +379,10 @@ ssh_userauth2(const char *local_user, const char *server_user, char *host, authctxt.info_req_seen = 0; authctxt.agent_fd = -1; pubkey_prepare(&authctxt); - if (authctxt.method == NULL) - fatal("ssh_userauth2: internal error: cannot send userauth none request"); + if (authctxt.method == NULL) { + fatal("%s: internal error: cannot send userauth none request", + __func__); + } if ((r = sshpkt_start(ssh, SSH2_MSG_SERVICE_REQUEST)) != 0 || (r = sshpkt_put_cstring(ssh, "ssh-userauth")) != 0 || diff --git a/sshd.c b/sshd.c index 5ee7eb0..2de01e9 100644 --- a/sshd.c +++ b/sshd.c @@ -161,13 +161,6 @@ char **rexec_argv; int listen_socks[MAX_LISTEN_SOCKS]; int num_listen_socks = 0; -/* - * the client's version string, passed by sshd2 in compat mode. if != NULL, - * sshd will skip the version-number exchange - */ -char *client_version_string = NULL; -char *server_version_string = NULL; - /* Daemon's agent connection */ int auth_sock = -1; int have_agent = 0; @@ -340,52 +333,67 @@ grace_alarm_handler(int sig) } static void -sshd_exchange_identification(struct ssh *ssh, int sock_in, int sock_out) +sshd_exchange_identification(struct ssh *ssh) { u_int i; - int remote_major, remote_minor; - char *s; - char buf[256]; /* Must not be larger than remote_version. */ - char remote_version[256]; /* Must be at least as big as buf. */ + int sock_in = ssh_packet_get_connection_in(ssh); + int sock_out = ssh_packet_get_connection_out(ssh); + int r, remote_major, remote_minor, expect_nl = 0; + char *client_version_string, *server_version_string; + char *s, *remote_version; + u_char c; - xasprintf(&server_version_string, "SSH-%d.%d-%.100s%s%s\r\n", + /* XXX factor out code common with ssh_exchange_identification */ + + /* prepare and send server banner */ + if ((r = sshbuf_putf(ssh->server_banner, "SSH-%d.%d-%.100s%s%s\r\n", PROTOCOL_MAJOR_2, PROTOCOL_MINOR_2, SSH_VERSION, *options.version_addendum == '\0' ? "" : " ", - options.version_addendum); - - /* Send our protocol version identification. */ - if (atomicio(vwrite, sock_out, server_version_string, - strlen(server_version_string)) - != strlen(server_version_string)) { + options.version_addendum)) != 0) + fatal("%s: sshbuf_putv: %s", __func__, ssh_err(r)); + if (atomicio(vwrite, sock_out, sshbuf_mutable_ptr(ssh->server_banner), + sshbuf_len(ssh->server_banner)) != sshbuf_len(ssh->server_banner)) { logit("Could not write ident string to %s port %d", ssh_remote_ipaddr(ssh), ssh_remote_port(ssh)); cleanup_exit(255); } + /* remove trailing \r\n */ + if ((r = sshbuf_consume_end(ssh->server_banner, 2)) != 0) + fatal("%s: sshbuf_consume_end: %s", __func__, ssh_err(r)); + server_version_string = sshbuf_dup_string(ssh->server_banner); + if (server_version_string == NULL) + fatal("%s: sshbuf_dup_string failed", __func__); /* Read other sides version identification. */ - memset(buf, 0, sizeof(buf)); - for (i = 0; i < sizeof(buf) - 1; i++) { - if (atomicio(read, sock_in, &buf[i], 1) != 1) { + sshbuf_reset(ssh->client_banner); + for (i = 0; ; i++) { + if (atomicio(read, sock_in, &c, 1) != 1) { logit("Did not receive identification string " "from %s port %d", ssh_remote_ipaddr(ssh), ssh_remote_port(ssh)); cleanup_exit(255); } - if (buf[i] == '\r') { - buf[i] = 0; - /* Kludge for F-Secure Macintosh < 1.0.2 */ - if (i == 12 && - strncmp(buf, "SSH-1.5-W1.0", 12) == 0) - break; + if (c == '\r') { + expect_nl = 1; continue; } - if (buf[i] == '\n') { - buf[i] = 0; + if (c == '\n') break; + if (expect_nl) { + logit("%s: banner line contains interior carriage " + "return character", __func__); + cleanup_exit(255); } + if ((r = sshbuf_put_u8(ssh->client_banner, c)) != 0) + fatal("%s: sshbuf_put_u8: %s", __func__, ssh_err(r)); + if (sshbuf_len(ssh->client_banner) > SSH_MAX_BANNER_LEN) + fatal("%s: client banner too long", __func__); } - buf[sizeof(buf) - 1] = 0; - client_version_string = xstrdup(buf); + client_version_string = sshbuf_dup_string(ssh->client_banner); + if (client_version_string == NULL) + fatal("%s: sshbuf_dup_string failed", __func__); + /* XXX must be same size for sscanf */ + remote_version = xcalloc(1, sshbuf_len(ssh->client_banner)); /* * Check that the versions match. In future this might accept @@ -1960,7 +1968,7 @@ main(int ac, char **av) if (!debug_flag) alarm(options.login_grace_time); - sshd_exchange_identification(ssh, sock_in, sock_out); + sshd_exchange_identification(ssh); packet_set_nonblocking(); /* allocate authentication context */ @@ -2120,8 +2128,6 @@ do_ssh2_kex(void) #endif kex->kex[KEX_C25519_SHA256] = kexc25519_server; kex->server = 1; - kex->client_version_string=client_version_string; - kex->server_version_string=server_version_string; kex->load_host_public_key=&get_hostkey_public_by_type; kex->load_host_private_key=&get_hostkey_private_by_type; kex->host_key_index=&get_hostkey_index;
Markus Schmidt
2018-Dec-14 09:06 UTC
[PATCH] cleanup of global variables server/client_version_string in sshconnect.c
On 12.14.18 06:29 , Damien Miller wrote:> On Mon, 10 Dec 2018, Markus Schmidt wrote: > >> >> These are used just in a few functions and can easily be passed as parameters. > > IMO it makes more sense to store these in struct ssh as buffers, since > they have pretty common handling between the client and server and > because we already need them for kex. >Sounds good to me :-D Markus