From 794ac8d88e6fff0a7b6f79fc3eb17f7b56e9bed7 Mon Sep 17 00:00:00 2001 From: djm Date: Tue, 26 Jan 2021 00:54:49 +0000 Subject: [PATCH] refactor key constraint parsing in ssh-agent Key constraints parsing code previously existed in both the "add regular key" and "add smartcard key" path. This unifies them but also introduces more consistency checking: duplicated constraints and constraints that are nonsensical for a particular situation (e.g. FIDO provider for a smartcard key) are now banned. ok markus@ --- usr.bin/ssh/ssh-agent.c | 164 +++++++++++++++++++++++----------------- 1 file changed, 95 insertions(+), 69 deletions(-) diff --git a/usr.bin/ssh/ssh-agent.c b/usr.bin/ssh/ssh-agent.c index eabbc113786..da2f5a06b37 100644 --- a/usr.bin/ssh/ssh-agent.c +++ b/usr.bin/ssh/ssh-agent.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ssh-agent.c,v 1.270 2021/01/26 00:53:31 djm Exp $ */ +/* $OpenBSD: ssh-agent.c,v 1.271 2021/01/26 00:54:49 djm Exp $ */ /* * Author: Tatu Ylonen * Copyright (c) 1995 Tatu Ylonen , Espoo, Finland @@ -560,44 +560,52 @@ reaper(void) return (deadline - now); } -static void -process_add_identity(SocketEntry *e) +static int +parse_key_constraints(struct sshbuf *m, struct sshkey *k, time_t *deathp, + u_int *secondsp, int *confirmp, char **sk_providerp) { - Identity *id; - int success = 0, confirm = 0; - u_int seconds = 0, maxsign; - char *fp, *comment = NULL, *ext_name = NULL, *sk_provider = NULL; - char canonical_provider[PATH_MAX]; - time_t death = 0; - struct sshkey *k = NULL; u_char ctype; - int r = SSH_ERR_INTERNAL_ERROR; + int r; + u_int seconds, maxsign = 0; + char *ext_name = NULL, *sk_provider = NULL; + size_t pos; + struct sshbuf *b = NULL; - debug2_f("entering"); - if ((r = sshkey_private_deserialize(e->request, &k)) != 0 || - k == NULL || - (r = sshbuf_get_cstring(e->request, &comment, NULL)) != 0) { - error_fr(r, "parse"); - goto err; - } - while (sshbuf_len(e->request)) { - if ((r = sshbuf_get_u8(e->request, &ctype)) != 0) { + while (sshbuf_len(m)) { + if ((r = sshbuf_get_u8(m, &ctype)) != 0) { error_fr(r, "parse constraint type"); goto err; } switch (ctype) { case SSH_AGENT_CONSTRAIN_LIFETIME: - if ((r = sshbuf_get_u32(e->request, &seconds)) != 0) { + if (*deathp != 0) { + error_f("lifetime already set"); + goto err; + } + if ((r = sshbuf_get_u32(m, &seconds)) != 0) { error_fr(r, "parse lifetime constraint"); goto err; } - death = monotime() + seconds; + *deathp = monotime() + seconds; + *secondsp = seconds; break; case SSH_AGENT_CONSTRAIN_CONFIRM: - confirm = 1; + if (*confirmp != 0) { + error_f("confirm already set"); + goto err; + } + *confirmp = 1; break; case SSH_AGENT_CONSTRAIN_MAXSIGN: - if ((r = sshbuf_get_u32(e->request, &maxsign)) != 0) { + if (k == NULL) { + error_f("maxsign not valid here"); + goto err; + } + if (maxsign != 0) { + error_f("maxsign already set"); + goto err; + } + if ((r = sshbuf_get_u32(m, &maxsign)) != 0) { error_fr(r, "parse maxsign constraint"); goto err; } @@ -607,19 +615,22 @@ process_add_identity(SocketEntry *e) } break; case SSH_AGENT_CONSTRAIN_EXTENSION: - if ((r = sshbuf_get_cstring(e->request, - &ext_name, NULL)) != 0) { + if ((r = sshbuf_get_cstring(m, &ext_name, NULL)) != 0) { error_fr(r, "parse constraint extension"); goto err; } debug_f("constraint ext %s", ext_name); if (strcmp(ext_name, "sk-provider@openssh.com") == 0) { - if (sk_provider != NULL) { - error("%s already set", ext_name); + if (sk_providerp == NULL) { + error_f("%s not valid here", ext_name); goto err; } - if ((r = sshbuf_get_cstring(e->request, - &sk_provider, NULL)) != 0) { + if (*sk_providerp != NULL) { + error_f("%s already set", ext_name); + goto err; + } + if ((r = sshbuf_get_cstring(m, + sk_providerp, NULL)) != 0) { error_fr(r, "parse %s", ext_name); goto err; } @@ -633,20 +644,46 @@ process_add_identity(SocketEntry *e) default: error_f("Unknown constraint %d", ctype); err: - free(sk_provider); free(ext_name); - sshbuf_reset(e->request); - free(comment); - sshkey_free(k); - goto send; + sshbuf_free(b); + return -1; } } + /* success */ + return 0; +} + +static void +process_add_identity(SocketEntry *e) +{ + Identity *id; + int success = 0, confirm = 0; + char *fp, *comment = NULL, *ext_name = NULL, *sk_provider = NULL; + char canonical_provider[PATH_MAX]; + time_t death = 0; + u_int seconds = 0; + struct sshkey *k = NULL; + int r = SSH_ERR_INTERNAL_ERROR; + + debug2_f("entering"); + if ((r = sshkey_private_deserialize(e->request, &k)) != 0 || + k == NULL || + (r = sshbuf_get_cstring(e->request, &comment, NULL)) != 0) { + error_fr(r, "parse"); + goto out; + } + if (parse_key_constraints(e->request, k, &death, &seconds, &confirm, + &sk_provider) != 0) { + error_f("failed to parse constraints"); + sshbuf_reset(e->request); + goto out; + } + if (sk_provider != NULL) { if (!sshkey_is_sk(k)) { error("Cannot add provider: %s is not an " "authenticator-hosted key", sshkey_type(k)); - free(sk_provider); - goto send; + goto out; } if (strcasecmp(sk_provider, "internal") == 0) { debug_f("internal provider"); @@ -655,8 +692,7 @@ process_add_identity(SocketEntry *e) verbose("failed provider \"%.100s\": " "realpath: %s", sk_provider, strerror(errno)); - free(sk_provider); - goto send; + goto out; } free(sk_provider); sk_provider = xstrdup(canonical_provider); @@ -664,17 +700,14 @@ process_add_identity(SocketEntry *e) allowed_providers, 0) != 1) { error("Refusing add key: " "provider %s not allowed", sk_provider); - free(sk_provider); - goto send; + goto out; } } } if ((r = sshkey_shield_private(k)) != 0) { error_fr(r, "shield private"); - goto err; + goto out; } - - success = 1; if (lifetime && !death) death = monotime() + lifetime; if ((id = lookup_identity(k)) == NULL) { @@ -688,6 +721,7 @@ process_add_identity(SocketEntry *e) free(id->comment); free(id->sk_provider); } + /* success */ id->key = k; id->comment = comment; id->death = death; @@ -698,10 +732,18 @@ process_add_identity(SocketEntry *e) SSH_FP_DEFAULT)) == NULL) fatal_f("sshkey_fingerprint failed"); debug_f("add %s %s \"%.100s\" (life: %u) (confirm: %u) " - "(provider: %s)", sshkey_ssh_name(k), fp, comment, - seconds, confirm, sk_provider == NULL ? "none" : sk_provider); + "(provider: %s)", sshkey_ssh_name(k), fp, comment, seconds, + confirm, sk_provider == NULL ? "none" : sk_provider); free(fp); -send: + /* transferred */ + k = NULL; + comment = NULL; + sk_provider = NULL; + success = 1; + out: + free(sk_provider); + free(comment); + sshkey_free(k); send_status(e, success); } @@ -780,7 +822,7 @@ process_add_smartcard_key(SocketEntry *e) char *provider = NULL, *pin = NULL, canonical_provider[PATH_MAX]; char **comments = NULL; int r, i, count = 0, success = 0, confirm = 0; - u_int seconds; + u_int seconds = 0; time_t death = 0; u_char type; struct sshkey **keys = NULL, *k; @@ -792,27 +834,10 @@ process_add_smartcard_key(SocketEntry *e) error_fr(r, "parse"); goto send; } - - while (sshbuf_len(e->request)) { - if ((r = sshbuf_get_u8(e->request, &type)) != 0) { - error_fr(r, "parse type"); - goto send; - } - switch (type) { - case SSH_AGENT_CONSTRAIN_LIFETIME: - if ((r = sshbuf_get_u32(e->request, &seconds)) != 0) { - error_fr(r, "parse lifetime"); - goto send; - } - death = monotime() + seconds; - break; - case SSH_AGENT_CONSTRAIN_CONFIRM: - confirm = 1; - break; - default: - error_f("Unknown constraint type %d", type); - goto send; - } + if (parse_key_constraints(e->request, NULL, &death, &seconds, &confirm, + NULL) != 0) { + error_f("failed to parse constraints"); + goto send; } if (realpath(provider, canonical_provider) == NULL) { verbose("failed PKCS#11 add of \"%.100s\": realpath: %s", @@ -848,6 +873,7 @@ process_add_smartcard_key(SocketEntry *e) idtab->nentries++; success = 1; } + /* XXX update constraints for existing keys */ sshkey_free(keys[i]); free(comments[i]); } -- 2.20.1