Attachment 2544 Details for Bug 2081 – revised diff

[patch] revised diff

keys_command_args.diff (text/plain), 14.54 KB, created by Damien Miller on 2015-02-10 00:11:52 AEDT

(hide)

Description:

Filename:

MIME Type:

Creator: Damien Miller

Created: 2015-02-10 00:11:52 AEDT

Size: 14.54 KB

patch

obsolete

>diff --git a/auth2-pubkey.c b/auth2-pubkey.c
>index 0e98254..9fa20cc 100644
>--- a/auth2-pubkey.c
>+++ b/auth2-pubkey.c
>@@ -62,6 +62,9 @@
> #include "monitor_wrap.h"
> #include "authfile.h"
> #include "match.h"
>+#include "ssherr.h"
>+#include "channels.h" /* XXX for session.h */
>+#include "session.h" /* XXX for child_set_env(); refactor? */
> 
> /* import */
> extern ServerOptions options;
>@@ -517,6 +520,83 @@ user_key_allowed2(struct passwd *pw, Key *key, char *file)
> }
> 
> /*
>+ * Splits 's' into an argument vector. Handles quoted string, basic escape
>+ * characters (\\, \", \'), but not nested quotes. Caller must free the
>+ * argument vector and its members.
>+ */
>+static int
>+split_argv(const char *s, int *argcp, char ***argvp)
>+{
>+	int r = SSH_ERR_INTERNAL_ERROR;
>+	int argc = 0, quote, wrong_quote, i, j;
>+	char *arg, **argv = xcalloc(1, sizeof(*argv));
>+
>+	*argvp = NULL;
>+	*argcp = 0;
>+
>+	for (i = 0; s[i] != '\0'; i++) {
>+		/* Start of a token */
>+		if (s[i] == ' ' || s[i] == '\t')
>+			continue; /* Skip leading whitespace */
>+
>+		wrong_quote = quote = 0;
>+		if (s[i] == '\\' &&
>+		    (s[i + 1] == '\'' || s[i + 1] == '\"' || s[i + 1] == '\\'))
>+			i++;
>+		else if (s[i] == '\'' || s[i] == '"') {
>+			quote = s[i++];
>+			wrong_quote = (quote == '\'') ? '\"' : '\'';
>+		}
>+		argv = xrealloc(argv, (argc + 2), sizeof(*argv));
>+		arg = argv[argc++] = xcalloc(1, strlen(s + i) + 1);
>+		argv[argc] = NULL;
>+
>+		/* Copy the token in, removing escapes */
>+		for (j = 0; s[i] != '\0'; i++) {
>+			if (s[i] == '\\') {
>+				if (s[i + 1] == '\'' ||
>+				    s[i + 1] == '\"' ||
>+				    s[i + 1] == '\\') {
>+					i++; /* Skip '\' */
>+					arg[j++] = s[i];
>+				} else
>+					arg[j++] = s[i];
>+			} else if (s[i] == wrong_quote) {
>+				/* disallow nested quotes */
>+				/* XXX needs smarter parser for this */
>+				r = SSH_ERR_INVALID_FORMAT;
>+				goto out;
>+			} else if (quote == 0 && (s[i] == ' ' || s[i] == '\t'))
>+				break; /* done */
>+			else if (quote != 0 && s[i] == quote)
>+				break; /* done */
>+			else
>+				arg[j++] = s[i];
>+		}
>+		if (s[i] == '\0') {
>+			if (quote != 0) {
>+				/* Ran out of string looking for close quote */
>+				r = SSH_ERR_INVALID_FORMAT;
>+				goto out;
>+			}
>+			break;
>+		}
>+	}
>+	/* Success */
>+	*argcp = argc;
>+	*argvp = argv;
>+	argc = 0;
>+	argv = NULL;
>+	r = 0;
>+ out:
>+	if (argc != 0 && argv != NULL) {
>+		for (i = 0; i < argc; i++)
>+			free(argv[i]);
>+		free(argv);
>+	}
>+	return r;
>+}
>+/*
>  * Checks whether key is allowed in output of command.
>  * returns 1 if the key is allowed or 0 otherwise.
>  */
>@@ -524,53 +604,109 @@ static int
> user_key_command_allowed2(struct passwd *user_pw, Key *key)
> {
> 	FILE *f;
>-	int ok, found_key = 0;
>+	int r, ok, found_key = 0;
> 	struct passwd *pw;
> 	struct stat st;
>-	int status, devnull, p[2], i;
>+	int status, devnull, p[3], i, ac = 0;
> 	pid_t pid;
>-	char *username, errmsg[512];
>+	char *username = NULL, *key_fp = NULL, *keytext = NULL;
>+	char *command = NULL, **av = NULL, *cp, errmsg[512];
>+	u_int envsize;
>+	char **child_env;
>+	void (*osigchld)(int);
> 
>-	if (options.authorized_keys_command == NULL ||
>-	    options.authorized_keys_command[0] != '/')
>+	if (options.authorized_keys_command == NULL)
> 		return 0;
>-
> 	if (options.authorized_keys_command_user == NULL) {
> 		error("No user for AuthorizedKeysCommand specified, skipping");
> 		return 0;
> 	}
> 
>+	/*
>+	 * NB. all returns later this function should go via "out" to
>+	 * ensure the original SIGCHLD handler is restored properly.
>+	 */
>+	osigchld = signal(SIGCHLD, SIG_DFL);
>+
>+	/* Prepare and verify the user for the command */
> 	username = percent_expand(options.authorized_keys_command_user,
> 	    "u", user_pw->pw_name, (char *)NULL);
> 	pw = getpwnam(username);
> 	if (pw == NULL) {
> 		error("AuthorizedKeysCommandUser \"%s\" not found: %s",
> 		    username, strerror(errno));
>-		free(username);
>-		return 0;
>+		goto out;
> 	}
>-	free(username);
> 
>+	/* Prepare AuthorizedKeysCommand */
>+	if ((key_fp = sshkey_fingerprint(key, options.fingerprint_hash,
>+	    SSH_FP_DEFAULT)) == NULL) {
>+		error("%s: sshkey_fingerprint failed", __func__);
>+		goto out;
>+	}
>+	if ((r = sshkey_to_base64(key, &keytext)) != 0) {
>+		error("%s: sshkey_to_base64 failed: %s", __func__, ssh_err(r));
>+		goto out;
>+	}
>+	command = percent_expand(options.authorized_keys_command,
>+	    "u", user_pw->pw_name, "h", user_pw->pw_dir,
>+	    "t", sshkey_ssh_name(key), "f", key_fp, "k", keytext, (char *)NULL);
>+
>+	/* Turn the command into an argument vector */
>+	if (split_argv(command, &ac, &av) != 0) {
>+		error("AuthorizedKeysCommand \"%s\" contains invalid quotes",
>+		    command);
>+		goto out;
>+	}
>+	if (ac == 0) {
>+		error("AuthorizedKeysCommand \"%s\" yielded no arguments",
>+		    command);
>+		goto out;
>+	}
>+
>+	/*
>+	 * If AuthorizedKeysCommand was run without arguments
>+	 * then fall back to the old behaviour of passing the
>+	 * target username as a single argument.
>+	 */
>+	if (ac == 1) {
>+		av = xrealloc(av, ac + 2, sizeof(*av));
>+		av[1] = xstrdup(user_pw->pw_name);
>+		av[2] = NULL;
>+		/* Fix up command too, since it is used in log messages */
>+		free(command);
>+		xasprintf(&command, "%s %s",
>+		    options.authorized_keys_command, user_pw->pw_name);
>+	}
>+
>+	/* Verify the path exists and is safe-ish to execute */
>+	if (*av[0] != '/') {
>+		error("AuthorizedKeysCommand path is not absolute");
>+		goto out;
>+	}
> 	temporarily_use_uid(pw);
>-
>-	if (stat(options.authorized_keys_command, &st) < 0) {
>+	if (stat(av[0], &st) < 0) {
> 		error("Could not stat AuthorizedKeysCommand \"%s\": %s",
>-		    options.authorized_keys_command, strerror(errno));
>+		    av[0], strerror(errno));
> 		goto out;
> 	}
>-	if (auth_secure_path(options.authorized_keys_command, &st, NULL, 0,
>+	if (auth_secure_path(av[0], &st, NULL, 0,
> 	    errmsg, sizeof(errmsg)) != 0) {
>-		error("Unsafe AuthorizedKeysCommand: %s", errmsg);
>+		error("Unsafe AuthorizedKeysCommand \"%s\": %s", av[0], errmsg);
> 		goto out;
> 	}
> 
>+	/*
>+	 * Run the command; stderr is left in place, stdout is the
>+	 * authorized_keys output.
>+	 */
> 	if (pipe(p) != 0) {
> 		error("%s: pipe: %s", __func__, strerror(errno));
> 		goto out;
> 	}
> 
>-	debug3("Running AuthorizedKeysCommand: \"%s %s\" as \"%s\"",
>-	    options.authorized_keys_command, user_pw->pw_name, pw->pw_name);
>+	debug3("Running AuthorizedKeysCommand: \"%s\" as \"%s\"",
>+	    command, pw->pw_name);
> 
> 	/*
> 	 * Don't want to call this in the child, where it can fatal() and
>@@ -585,6 +721,16 @@ user_key_command_allowed2(struct passwd *user_pw, Key *key)
> 		close(p[1]);
> 		return 0;
> 	case 0: /* child */
>+		/* Prepare a minimal environment for the child. */
>+		envsize = 5;
>+		child_env = xcalloc(sizeof(*child_env), envsize);
>+		child_set_env(&child_env, &envsize, "PATH", _PATH_STDPATH);
>+		child_set_env(&child_env, &envsize, "USER", pw->pw_name);
>+		child_set_env(&child_env, &envsize, "LOGNAME", pw->pw_name);
>+		child_set_env(&child_env, &envsize, "HOME", pw->pw_dir);
>+		if ((cp = getenv("LANG")) != NULL)
>+			child_set_env(&child_env, &envsize, "LANG", cp);
>+
> 		for (i = 0; i < NSIG; i++)
> 			signal(i, SIG_DFL);
> 
>@@ -618,11 +764,9 @@ user_key_command_allowed2(struct passwd *user_pw, Key *key)
> 			_exit(1);
> 		}
> 
>-		execl(options.authorized_keys_command,
>-		    options.authorized_keys_command, user_pw->pw_name, NULL);
>-
>-		error("AuthorizedKeysCommand %s exec failed: %s",
>-		    options.authorized_keys_command, strerror(errno));
>+		execve(av[0], av, child_env);
>+		error("AuthorizedKeysCommand exec \"%s\" failed: %s",
>+		    command, strerror(errno));
> 		_exit(127);
> 	default: /* parent */
> 		break;
>@@ -658,9 +802,20 @@ user_key_command_allowed2(struct passwd *user_pw, Key *key)
> 		    options.authorized_keys_command, WEXITSTATUS(status));
> 		goto out;
> 	}
>+	/* Read completed successfully */
> 	found_key = ok;
>  out:
>+	signal(SIGCHLD, osigchld);
>+	if (ac != 0) {
>+		for (i = 0; i < ac; i++)
>+			free(av[i]);
>+	}
>+	free(av);
> 	restore_uid();
>+	free(command);
>+	free(username);
>+	free(key_fp);
>+	free(keytext);
> 	return found_key;
> }
> 
>diff --git a/sshd_config.5 b/sshd_config.5
>index d116df6..1086e07 100644
>--- a/sshd_config.5
>+++ b/sshd_config.5
>@@ -230,9 +230,21 @@ The default is not to require multiple authentication; successful completion
> of a single authentication method is sufficient.
> .It Cm AuthorizedKeysCommand
> Specifies a program to be used to look up the user's public keys.
>-The program must be owned by root and not writable by group or others.
>-It will be invoked with a single argument of the username
>-being authenticated, and should produce on standard output zero or
>+The program must be owned by root, not writable by group or others and
>+specified by an absolute path.
>+.Pp
>+Arguments to
>+.Cm AuthorizedKeysCommand
>+may be provided using the following tokens, which will be expanded
>+at runtime: %% is replaced by a literal '%', %u is replaced by the
>+username being authenticated, %h is replaced by the home directory
>+of the user being authenticated, %t is replaced with the key type
>+offered for authentication, %f is replaced with the fingerprint of
>+the key, and %k is replaced with the key being offered for authentication.
>+If no arguments are specified then the username of the target user
>+will be supplied.
>+.Pp
>+The program should produce on standard output zero or
> more lines of authorized_keys output (see AUTHORIZED_KEYS in
> .Xr sshd 8 ) .
> If a key supplied by AuthorizedKeysCommand does not successfully authenticate
>diff --git a/sshkey.c b/sshkey.c
>index c311834..9cf0dad 100644
>--- a/sshkey.c
>+++ b/sshkey.c
>@@ -737,6 +737,12 @@ to_blob_buf(const struct sshkey *key, struct sshbuf *b, int force_plain)
> 	if (key == NULL)
> 		return SSH_ERR_INVALID_ARGUMENT;
> 
>+	if (sshkey_is_cert(key)) {
>+		if (key->cert == NULL)
>+			return SSH_ERR_EXPECTED_CERT;
>+		if (sshbuf_len(key->cert->certblob) == 0)
>+			return SSH_ERR_KEY_LACKS_CERTBLOB;
>+	}
> 	type = force_plain ? sshkey_type_plain(key->type) : key->type;
> 	typename = sshkey_ssh_name_from_type_nid(type, key->ecdsa_nid);
> 
>@@ -1381,98 +1387,116 @@ sshkey_read(struct sshkey *ret, char **cpp)
> }
> 
> int
>-sshkey_write(const struct sshkey *key, FILE *f)
>+sshkey_to_base64(const struct sshkey *key, char **b64p)
> {
>-	int ret = SSH_ERR_INTERNAL_ERROR;
>-	struct sshbuf *b = NULL, *bb = NULL;
>+	int r = SSH_ERR_INTERNAL_ERROR;
>+	struct sshbuf *b = NULL;
> 	char *uu = NULL;
>+
>+	if (b64p != NULL)
>+		*b64p = NULL;
>+	if ((b = sshbuf_new()) == NULL)
>+		return SSH_ERR_ALLOC_FAIL;
>+	if ((r = sshkey_putb(key, b)) != 0)
>+		goto out;
>+	if ((uu = sshbuf_dtob64(b)) == NULL) {
>+		r = SSH_ERR_ALLOC_FAIL;
>+		goto out;
>+	}
>+	/* Success */
>+	if (b64p != NULL) {
>+		*b64p = uu;
>+		uu = NULL;
>+	}
>+	r = 0;
>+ out:
>+	sshbuf_free(b);
>+	free(uu);
>+	return r;
>+}
>+
>+static int
>+sshkey_format_rsa1(const struct sshkey *key, struct sshbuf *b)
>+{
>+	int r = SSH_ERR_INTERNAL_ERROR;
> #ifdef WITH_SSH1
> 	u_int bits = 0;
> 	char *dec_e = NULL, *dec_n = NULL;
>-#endif /* WITH_SSH1 */
> 
>-	if (sshkey_is_cert(key)) {
>-		if (key->cert == NULL)
>-			return SSH_ERR_EXPECTED_CERT;
>-		if (sshbuf_len(key->cert->certblob) == 0)
>-			return SSH_ERR_KEY_LACKS_CERTBLOB;
>+	if (key->rsa == NULL || key->rsa->e == NULL ||
>+	    key->rsa->n == NULL) {
>+		r = SSH_ERR_INVALID_ARGUMENT;
>+		goto out;
> 	}
>-	if ((b = sshbuf_new()) == NULL)
>-		return SSH_ERR_ALLOC_FAIL;
>-	switch (key->type) {
>-#ifdef WITH_SSH1
>-	case KEY_RSA1:
>-		if (key->rsa == NULL || key->rsa->e == NULL ||
>-		    key->rsa->n == NULL) {
>-			ret = SSH_ERR_INVALID_ARGUMENT;
>-			goto out;
>-		}
>-		if ((dec_e = BN_bn2dec(key->rsa->e)) == NULL ||
>-		    (dec_n = BN_bn2dec(key->rsa->n)) == NULL) {
>-			ret = SSH_ERR_ALLOC_FAIL;
>-			goto out;
>-		}
>-		/* size of modulus 'n' */
>-		if ((bits = BN_num_bits(key->rsa->n)) <= 0) {
>-			ret = SSH_ERR_INVALID_ARGUMENT;
>-			goto out;
>-		}
>-		if ((ret = sshbuf_putf(b, "%u %s %s", bits, dec_e, dec_n)) != 0)
>-			goto out;
>-#endif /* WITH_SSH1 */
>-		break;
>-#ifdef WITH_OPENSSL
>-	case KEY_DSA:
>-	case KEY_DSA_CERT_V00:
>-	case KEY_DSA_CERT:
>-	case KEY_ECDSA:
>-	case KEY_ECDSA_CERT:
>-	case KEY_RSA:
>-	case KEY_RSA_CERT_V00:
>-	case KEY_RSA_CERT:
>-#endif /* WITH_OPENSSL */
>-	case KEY_ED25519:
>-	case KEY_ED25519_CERT:
>-		if ((bb = sshbuf_new()) == NULL) {
>-			ret = SSH_ERR_ALLOC_FAIL;
>-			goto out;
>-		}
>-		if ((ret = sshkey_putb(key, bb)) != 0)
>-			goto out;
>-		if ((uu = sshbuf_dtob64(bb)) == NULL) {
>-			ret = SSH_ERR_ALLOC_FAIL;
>-			goto out;
>-		}
>-		if ((ret = sshbuf_putf(b, "%s ", sshkey_ssh_name(key))) != 0)
>-			goto out;
>-		if ((ret = sshbuf_put(b, uu, strlen(uu))) != 0)
>-			goto out;
>-		break;
>-	default:
>-		ret = SSH_ERR_KEY_TYPE_UNKNOWN;
>+	if ((dec_e = BN_bn2dec(key->rsa->e)) == NULL ||
>+	    (dec_n = BN_bn2dec(key->rsa->n)) == NULL) {
>+		r = SSH_ERR_ALLOC_FAIL;
> 		goto out;
> 	}
>-	if (fwrite(sshbuf_ptr(b), sshbuf_len(b), 1, f) != 1) {
>-		if (feof(f))
>-			errno = EPIPE;
>-		ret = SSH_ERR_SYSTEM_ERROR;
>+	/* size of modulus 'n' */
>+	if ((bits = BN_num_bits(key->rsa->n)) <= 0) {
>+		r = SSH_ERR_INVALID_ARGUMENT;
> 		goto out;
> 	}
>-	ret = 0;
>+	if ((r = sshbuf_putf(b, "%u %s %s", bits, dec_e, dec_n)) != 0)
>+		goto out;
>+
>+	/* Success */
>+	r = 0;
>  out:
>-	if (b != NULL)
>-		sshbuf_free(b);
>-	if (bb != NULL)
>-		sshbuf_free(bb);
>-	if (uu != NULL)
>-		free(uu);
>-#ifdef WITH_SSH1
> 	if (dec_e != NULL)
> 		OPENSSL_free(dec_e);
> 	if (dec_n != NULL)
> 		OPENSSL_free(dec_n);
> #endif /* WITH_SSH1 */
>-	return ret;
>+
>+	return r;
>+}
>+
>+static int
>+sshkey_format_text(const struct sshkey *key, struct sshbuf *b)
>+{
>+	int r = SSH_ERR_INTERNAL_ERROR;
>+	char *uu = NULL;
>+
>+	if (key->type == KEY_RSA1) {
>+		if ((r = sshkey_format_rsa1(key, b)) != 0)
>+			goto out;
>+	} else {
>+		/* Unsupported key types handled in sshkey_to_base64() */
>+		if ((r = sshkey_to_base64(key, &uu)) != 0)
>+			goto out;
>+		if ((r = sshbuf_putf(b, "%s %s",
>+		    sshkey_ssh_name(key), uu)) != 0)
>+			goto out;
>+	}
>+	r = 0;
>+ out:
>+	free(uu);
>+	return r;
>+}
>+
>+int
>+sshkey_write(const struct sshkey *key, FILE *f)
>+{
>+	struct sshbuf *b = NULL;
>+	int r = SSH_ERR_INTERNAL_ERROR;
>+
>+	if ((b = sshbuf_new()) == NULL)
>+		return SSH_ERR_ALLOC_FAIL;
>+	if ((r = sshkey_format_text(key, b)) != 0)
>+		goto out;
>+	if (fwrite(sshbuf_ptr(b), sshbuf_len(b), 1, f) != 1) {
>+		if (feof(f))
>+			errno = EPIPE;
>+		r = SSH_ERR_SYSTEM_ERROR;
>+		goto out;
>+	}
>+	/* Success */
>+	r = 0;
>+ out:
>+	sshbuf_free(b);
>+	return r;
> }
> 
> const char *
>diff --git a/sshkey.h b/sshkey.h
>index e05b407..9cbc1fb 100644
>--- a/sshkey.h
>+++ b/sshkey.h
>@@ -157,6 +157,7 @@ int	 sshkey_from_blob(const u_char *, size_t, struct sshkey **);
> int	 sshkey_fromb(struct sshbuf *, struct sshkey **);
> int	 sshkey_froms(struct sshbuf *, struct sshkey **);
> int	 sshkey_to_blob(const struct sshkey *, u_char **, size_t *);
>+int	 sshkey_to_base64(const struct sshkey *, char **);
> int	 sshkey_putb(const struct sshkey *, struct sshbuf *);
> int	 sshkey_puts(const struct sshkey *, struct sshbuf *);
> int	 sshkey_plain_to_blob(const struct sshkey *, u_char **, size_t *);

Actions: View | Diff

Attachments on bug 2081: 2412 | 2416 | 2417 | 2438 | 2477 | 2478 | 2479 | 2522 | 2544 | 2545 | 2546 | 2549 | 2556 | 2557