Skip to content

Commit

Permalink
upstream: UpdateHostkeys: better CheckHostIP handling
Browse files Browse the repository at this point in the history 
When preparing to update the known_hosts file, fully check both
entries for both the host and the address (if CheckHostIP enabled)
and ensure that, at the end of the operation, entries for both are
recorded.

Make sure this works with HashKnownHosts too, which requires maintaining
a list of entry-types seen across the whole file for each key.

ok markus@

OpenBSD-Commit-ID: 374dc263103f6b343d9671f87dbf81ffd0d6abdd
  • Loading branch information
@djmdjm
djmdjm committed Oct 12, 2020
1 parent af5941a commit d98f14b
Show file tree
Hide file tree
Showing 2 changed files with 95 additions and 53 deletions.
71 changes: 46 additions & 25 deletions clientloop.c
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/* $OpenBSD: clientloop.c,v 1.350 2020/10/11 22:12:44 djm Exp $ */
/* $OpenBSD: clientloop.c,v 1.351 2020/10/11 22:13:37 djm Exp $ */
/*
* Author: Tatu Ylonen <[email protected]>
* Copyright (c) 1995 Tatu Ylonen <[email protected]>, Espoo, Finland
Expand Down Expand Up @@ -1814,12 +1814,13 @@ struct hostkeys_update_ctx {
/*
* Keys received from the server and a flag for each indicating
* whether they already exist in known_hosts.
* keys_seen is filled in by hostkeys_find() and later (for new
* keys_match is filled in by hostkeys_find() and later (for new
* keys) by client_global_hostkeys_private_confirm().
*/
struct sshkey **keys;
int *keys_seen;
size_t nkeys, nnew;
u_int *keys_match; /* mask of HKF_MATCH_* from hostfile.h */
int *keys_verified; /* flag for new keys verified by server */
size_t nkeys, nnew, nincomplete; /* total, new keys, incomplete match */

/*
* Keys that are in known_hosts, but were not present in the update
Expand All @@ -1844,7 +1845,8 @@ hostkeys_update_ctx_free(struct hostkeys_update_ctx *ctx)
for (i = 0; i < ctx->nkeys; i++)
sshkey_free(ctx->keys[i]);
free(ctx->keys);
free(ctx->keys_seen);
free(ctx->keys_match);
free(ctx->keys_verified);
for (i = 0; i < ctx->nold; i++)
sshkey_free(ctx->old_keys[i]);
free(ctx->old_keys);
Expand Down Expand Up @@ -1900,12 +1902,12 @@ hostkeys_find(struct hostkey_foreach_line *l, void *_ctx)

/* Mark off keys we've already seen for this host */
for (i = 0; i < ctx->nkeys; i++) {
if (sshkey_equal(l->key, ctx->keys[i])) {
debug3("%s: found %s key at %s:%ld", __func__,
sshkey_ssh_name(ctx->keys[i]), l->path, l->linenum);
ctx->keys_seen[i] = 1;
return 0;
}
if (!sshkey_equal(l->key, ctx->keys[i]))
continue;
debug3("%s: found %s key at %s:%ld", __func__,
sshkey_ssh_name(ctx->keys[i]), l->path, l->linenum);
ctx->keys_match[i] |= l->match;
return 0;
}
/* This line contained a key that not offered by the server */
debug3("%s: deprecated %s key at %s:%ld", __func__,
Expand Down Expand Up @@ -1940,7 +1942,7 @@ update_known_hosts(struct hostkeys_update_ctx *ctx)
struct stat sb;

for (i = 0; i < ctx->nkeys; i++) {
if (ctx->keys_seen[i] != 2)
if (!ctx->keys_verified[i])
continue;
if ((fp = sshkey_fingerprint(ctx->keys[i],
options.fingerprint_hash, SSH_FP_DEFAULT)) == NULL)
Expand Down Expand Up @@ -2053,10 +2055,10 @@ client_global_hostkeys_private_confirm(struct ssh *ssh, int type,
/*
* Expect a signature for each of the ctx->nnew private keys we
* haven't seen before. They will be in the same order as the
* ctx->keys where the corresponding ctx->keys_seen[i] == 0.
* ctx->keys where the corresponding ctx->keys_match[i] == 0.
*/
for (ndone = i = 0; i < ctx->nkeys; i++) {
if (ctx->keys_seen[i])
if (ctx->keys_match[i])
continue;
/* Prepare data to be signed: session ID, unique string, key */
sshbuf_reset(signdata);
Expand Down Expand Up @@ -2088,7 +2090,7 @@ client_global_hostkeys_private_confirm(struct ssh *ssh, int type,
goto out;
}
/* Key is good. Mark it as 'seen' */
ctx->keys_seen[i] = 2;
ctx->keys_verified[i] = 1;
ndone++;
}
if (ndone != ctx->nnew)
Expand Down Expand Up @@ -2141,6 +2143,7 @@ client_input_hostkeys(struct ssh *ssh)
static int hostkeys_seen = 0; /* XXX use struct ssh */
extern struct sockaddr_storage hostaddr; /* XXX from ssh.c */
struct hostkeys_update_ctx *ctx = NULL;
u_int want;

if (hostkeys_seen)
fatal("%s: server already sent hostkeys", __func__);
Expand Down Expand Up @@ -2205,8 +2208,10 @@ client_input_hostkeys(struct ssh *ssh)
goto out;
}

if ((ctx->keys_seen = calloc(ctx->nkeys,
sizeof(*ctx->keys_seen))) == NULL)
if ((ctx->keys_match = calloc(ctx->nkeys,
sizeof(*ctx->keys_match))) == NULL ||
(ctx->keys_verified = calloc(ctx->nkeys,
sizeof(*ctx->keys_verified))) == NULL)
fatal("%s: calloc failed", __func__);

get_hostfile_hostname_ipaddr(host,
Expand Down Expand Up @@ -2234,21 +2239,37 @@ client_input_hostkeys(struct ssh *ssh)
}

/* Figure out if we have any new keys to add */
ctx->nnew = 0;
ctx->nnew = ctx->nincomplete = 0;
want = HKF_MATCH_HOST | ( options.check_host_ip ? HKF_MATCH_IP : 0);
for (i = 0; i < ctx->nkeys; i++) {
if (!ctx->keys_seen[i])
if (ctx->keys_match[i] == 0)
ctx->nnew++;
if ((ctx->keys_match[i] & want) != want)
ctx->nincomplete++;
}

debug3("%s: %zu keys from server: %zu new, %zu retained. %zu to remove",
__func__, ctx->nkeys, ctx->nnew, ctx->nkeys - ctx->nnew, ctx->nold);
/*
* XXX if removing keys, check whether they appear under different
* names/addresses and refuse to proceed if they do.
*/

debug3("%s: %zu server keys: %zu new, %zu retained, "
"%zu incomplete match. %zu to remove", __func__, ctx->nkeys,
ctx->nnew, ctx->nkeys - ctx->nnew - ctx->nincomplete,
ctx->nincomplete, ctx->nold);

if (ctx->complex_hostspec && (ctx->nnew != 0 || ctx->nold != 0)) {
if (ctx->complex_hostspec &&
(ctx->nnew != 0 || ctx->nold != 0 || ctx->nincomplete != 0)) {
debug("%s: manual list or wildcard host pattern found, "
"skipping UserKnownHostsFile update", __func__);
goto out;
} else if (ctx->nnew == 0 && ctx->nold != 0) {
/* We have some keys to remove. Just do it. */
} else if (ctx->nnew == 0 &&
(ctx->nold != 0 || ctx->nincomplete != 0)) {
/*
* We have some keys to remove or fix matching for.
* We can proceed to do this without requiring a fresh proof
* from the server.
*/
update_known_hosts(ctx);
} else if (ctx->nnew != 0) {
/*
Expand All @@ -2266,7 +2287,7 @@ client_input_hostkeys(struct ssh *ssh)
if ((buf = sshbuf_new()) == NULL)
fatal("%s: sshbuf_new", __func__);
for (i = 0; i < ctx->nkeys; i++) {
if (ctx->keys_seen[i])
if (ctx->keys_match[i])
continue;
sshbuf_reset(buf);
if ((r = sshkey_putb(ctx->keys[i], buf)) != 0)
Expand Down
77 changes: 49 additions & 28 deletions hostfile.c
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/* $OpenBSD: hostfile.c,v 1.84 2020/10/07 02:25:43 djm Exp $ */
/* $OpenBSD: hostfile.c,v 1.85 2020/10/11 22:13:37 djm Exp $ */
/*
* Author: Tatu Ylonen <[email protected]>
* Copyright (c) 1995 Tatu Ylonen <[email protected]>, Espoo, Finland
Expand Down Expand Up @@ -520,8 +520,8 @@ add_host_to_hostfile(const char *filename, const char *host,
struct host_delete_ctx {
FILE *out;
int quiet;
const char *host;
int *skip_keys; /* XXX split for host/ip? might want to ensure both */
const char *host, *ip;
u_int *match_keys; /* mask of HKF_MATCH_* for this key */
struct sshkey * const *keys;
size_t nkeys;
int modified;
Expand All @@ -534,26 +534,21 @@ host_delete(struct hostkey_foreach_line *l, void *_ctx)
int loglevel = ctx->quiet ? SYSLOG_LEVEL_DEBUG1 : SYSLOG_LEVEL_VERBOSE;
size_t i;

if (l->status == HKF_STATUS_MATCHED) {
if (l->marker != MRK_NONE) {
/* Don't remove CA and revocation lines */
fprintf(ctx->out, "%s\n", l->line);
return 0;
}

/* Don't remove CA and revocation lines */
if (l->status == HKF_STATUS_MATCHED && l->marker == MRK_NONE) {
/*
* If this line contains one of the keys that we will be
* adding later, then don't change it and mark the key for
* skipping.
*/
for (i = 0; i < ctx->nkeys; i++) {
if (sshkey_equal(ctx->keys[i], l->key)) {
ctx->skip_keys[i] = 1;
fprintf(ctx->out, "%s\n", l->line);
debug3("%s: %s key already at %s:%ld", __func__,
sshkey_type(l->key), l->path, l->linenum);
return 0;
}
if (!sshkey_equal(ctx->keys[i], l->key))
continue;
ctx->match_keys[i] |= l->match;
fprintf(ctx->out, "%s\n", l->line);
debug3("%s: %s key already at %s:%ld", __func__,
sshkey_type(l->key), l->path, l->linenum);
return 0;
}

/*
Expand Down Expand Up @@ -584,15 +579,19 @@ hostfile_replace_entries(const char *filename, const char *host, const char *ip,
int loglevel = quiet ? SYSLOG_LEVEL_DEBUG1 : SYSLOG_LEVEL_VERBOSE;
struct host_delete_ctx ctx;
char *fp, *temp = NULL, *back = NULL;
const char *what;
mode_t omask;
size_t i;
u_int want;

omask = umask(077);

memset(&ctx, 0, sizeof(ctx));
ctx.host = host;
ctx.ip = ip;
ctx.quiet = quiet;
if ((ctx.skip_keys = calloc(nkeys, sizeof(*ctx.skip_keys))) == NULL)

if ((ctx.match_keys = calloc(nkeys, sizeof(*ctx.match_keys))) == NULL)
return SSH_ERR_ALLOC_FAIL;
ctx.keys = keys;
ctx.nkeys = nkeys;
Expand Down Expand Up @@ -621,31 +620,53 @@ hostfile_replace_entries(const char *filename, const char *host, const char *ip,
goto fail;
}

/* Remove all entries for the specified host from the file */
/* Remove stale/mismatching entries for the specified host */
if ((r = hostkeys_foreach(filename, host_delete, &ctx, host, ip,
HKF_WANT_PARSE_KEY)) != 0) {
oerrno = errno;
error("%s: hostkeys_foreach failed: %s", __func__, ssh_err(r));
goto fail;
}

/* Add the requested keys */
/* Re-add the requested keys */
want = HKF_MATCH_HOST | (ip == NULL ? 0 : HKF_MATCH_IP);
for (i = 0; i < nkeys; i++) {
if (ctx.skip_keys[i])
if ((want & ctx.match_keys[i]) == want)
continue;
if ((fp = sshkey_fingerprint(keys[i], hash_alg,
SSH_FP_DEFAULT)) == NULL) {
r = SSH_ERR_ALLOC_FAIL;
goto fail;
}
do_log2(loglevel, "%s%sAdding new key for %s to %s: %s %s",
quiet ? __func__ : "", quiet ? ": " : "", host, filename,
/* write host/ip */
what = "";
if (ctx.match_keys[i] == 0) {
what = "Adding new key";
if (!write_host_entry(ctx.out, host, ip,
keys[i], store_hash)) {
r = SSH_ERR_INTERNAL_ERROR;
goto fail;
}
} else if ((want & ~ctx.match_keys[i]) == HKF_MATCH_HOST) {
what = "Fixing match (hostname)";
if (!write_host_entry(ctx.out, host, NULL,
keys[i], store_hash)) {
r = SSH_ERR_INTERNAL_ERROR;
goto fail;
}
} else if ((want & ~ctx.match_keys[i]) == HKF_MATCH_IP) {
what = "Fixing match (address)";
if (!write_host_entry(ctx.out, ip, NULL,
keys[i], store_hash)) {
r = SSH_ERR_INTERNAL_ERROR;
goto fail;
}
}
do_log2(loglevel, "%s%s%s for %s%s%s to %s: %s %s",
quiet ? __func__ : "", quiet ? ": " : "", what,
host, ip == NULL ? "" : ",", ip == NULL ? "" : ip, filename,
sshkey_ssh_name(keys[i]), fp);
free(fp);
if (!write_host_entry(ctx.out, host, ip, keys[i], store_hash)) {
r = SSH_ERR_INTERNAL_ERROR;
goto fail;
}
ctx.modified = 1;
}
fclose(ctx.out);
Expand Down Expand Up @@ -690,7 +711,7 @@ hostfile_replace_entries(const char *filename, const char *host, const char *ip,
free(back);
if (ctx.out != NULL)
fclose(ctx.out);
free(ctx.skip_keys);
free(ctx.match_keys);
umask(omask);
if (r == SSH_ERR_SYSTEM_ERROR)
errno = oerrno;
Expand Down

0 comments on commit d98f14b

Please sign in to comment.