[PATCH 2/4] add signing option -O hashalg=algorithm

[Fabian Stelzer]

On 22.12.2021 09:23, Linus Nordberg wrote:
>---
[...]
>diff --git a/ssh-keygen.c b/ssh-keygen.c
>index d31dc503..ef99b9b6 100644
>--- a/ssh-keygen.c
>+++ b/ssh-keygen.c
>@@ -142,6 +142,9 @@ struct cert_ext {
> static struct cert_ext *cert_ext;
> static size_t ncert_ext;
>
>+/* Default hash algorithm for -Y sign and verify. */
>+#define DEFAULT_SIGN_HASHALG_NAME "sha512"

I'm not a fan of redefining this. sshsig.c has HASHALG_DEFAULT we could 
resue when moved to sshsig.h. But I don't think we even need this default 
(see below).

[...]
>
> static int
>-sig_process_opts(char * const *opts, size_t nopts, uint64_t *verify_timep,
>+sig_process_opts(char * const *opts, size_t nopts, char *hashalg, size_t hashalg_size, uint64_t *verify_timep,
>     int *print_pubkey)
> {
> 	size_t i;
> 	time_t now;
>
>+	if (hashalg != NULL)
>+		strlcpy(hashalg, DEFAULT_SIGN_HASHALG_NAME, hashalg_size);
> 	if (verify_timep != NULL)
> 		*verify_timep = 0;
> 	if (print_pubkey != NULL)
> 		*print_pubkey = 0;
> 	for (i = 0; i < nopts; i++) {
>-		if (verify_timep &&
>+		if (hashalg &&
>+		    strncasecmp(opts[i], "hashalg=", 8) == 0) {
>+			strlcpy(hashalg, opts[i] + 8, hashalg_size);

Why copy at all? If we simply return a pointer to the option (or NULL if not 
present) we can pass it on as is. This would use the default from sshsig.c 
and we would not need to care about one here. The size wouldn't be needed as 
well. Basically we could return hashalg = opts[i] + 8, no?

>+		} else if (verify_timep &&
> 		    strncasecmp(opts[i], "verify-time=", 12) == 0) {
> 			if (parse_absolute_time(opts[i] + 12,
> 			    verify_timep) != 0 || *verify_timep == 0) {
>@@ -2640,12 +2648,13 @@ sig_process_opts(char * const *opts, size_t nopts, uint64_t *verify_timep,
> }
>
> static int
>-sig_sign(const char *keypath, const char *sig_namespace, int argc, char **argv)
>+sig_sign(const char *keypath, const char *sig_namespace, int argc, char **argv, char * const *opts, size_t nopts)
> {
> 	int i, fd = -1, r, ret = -1;
> 	int agent_fd = -1;
> 	struct sshkey *pubkey = NULL, *privkey = NULL, *signkey = NULL;
> 	sshsig_signer *signer = NULL;
>+	char hashalg[7];	/* "shaXXX" */

If we use the pointer into opts directly we are also safe in case sha1024 
(or some other hash with a longer name) comes along, without adjusting this 
one.

>
> 	/* Check file arguments. */
> 	for (i = 0; i < argc; i++) {
>@@ -2655,6 +2664,9 @@ sig_sign(const char *keypath, const char *sig_namespace, int argc, char **argv)
> 			fatal("Cannot sign mix of paths and standard input");
> 	}
>
>+	if (sig_process_opts(opts, nopts, hashalg, sizeof(hashalg), NULL, NULL) != 0)
>+		goto done; /* error already logged */
>+
> 	if ((r = sshkey_load_public(keypath, &pubkey, NULL)) != 0) {
> 		error_r(r, "Couldn't load public key %s", keypath);
> 		goto done;
>@@ -2681,7 +2693,7 @@ sig_sign(const char *keypath, const char *sig_namespace, int argc, char **argv)
>
> 	if (argc == 0) {
> 		if ((r = sign_one(signkey, "(stdin)", STDIN_FILENO,
>-		    sig_namespace, signer, &agent_fd)) != 0)
>+		    sig_namespace, hashalg, signer, &agent_fd)) != 0)
> 			goto done;
> 	} else {
> 		for (i = 0; i < argc; i++) {
>@@ -2693,7 +2705,7 @@ sig_sign(const char *keypath, const char *sig_namespace, int argc, char **argv)
> 				goto done;
> 			}
> 			if ((r = sign_one(signkey, argv[i], fd, sig_namespace,
>-			    signer, &agent_fd)) != 0)
>+			    hashalg, signer, &agent_fd)) != 0)
> 				goto done;
> 			if (fd != STDIN_FILENO)
> 				close(fd);
>@@ -2723,7 +2735,7 @@ sig_verify(const char *signature, const char *sig_namespace,
> 	struct sshkey_sig_details *sig_details = NULL;
> 	uint64_t verify_time = 0;
>
>-	if (sig_process_opts(opts, nopts, &verify_time, &print_pubkey) != 0)
>+	if (sig_process_opts(opts, nopts, NULL, 0, &verify_time, &print_pubkey) != 0)
> 		goto done; /* error already logged */
>
> 	memset(&sig_details, 0, sizeof(sig_details));
>@@ -2811,7 +2823,7 @@ sig_find_principals(const char *signature, const char *allowed_keys,
> 	char *principals = NULL, *cp, *tmp;
> 	uint64_t verify_time = 0;
>
>-	if (sig_process_opts(opts, nopts, &verify_time, NULL) != 0)
>+	if (sig_process_opts(opts, nopts, NULL, 0, &verify_time, NULL) != 0)
> 		goto done; /* error already logged */
>
> 	if ((r = sshbuf_load_file(signature, &abuf)) != 0) {
>@@ -2857,7 +2869,7 @@ sig_match_principals(const char *allowed_keys, char *principal,
> 	char **principals = NULL;
> 	size_t i, nprincipals = 0;
>
>-	if ((r = sig_process_opts(opts, nopts, NULL, NULL)) != 0)
>+	if ((r = sig_process_opts(opts, nopts, NULL, 0, NULL, NULL)) != 0)
> 		return r; /* error already logged */
>
> 	if ((r = sshsig_match_principals(allowed_keys, principal,
>@@ -3215,7 +3227,7 @@ usage(void)
> 	    "       ssh-keygen -Y find-principals -s signature_file -f allowed_signers_file\n"
> 	    "       ssh-keygen -Y match-principals -I signer_identity -f allowed_signers_file\n"
> 	    "       ssh-keygen -Y check-novalidate -n namespace -s signature_file\n"
>-	    "       ssh-keygen -Y sign -f key_file -n namespace file ...\n"
>+	    "       ssh-keygen -Y sign -f key_file -n namespace file [-O option] ...\n"
> 	    "       ssh-keygen -Y verify -f allowed_signers_file -I signer_identity\n"
> 	    "                  -n namespace -s signature_file [-r revocation_file]\n");
> 	exit(1);
>@@ -3521,7 +3533,7 @@ main(int argc, char **argv)
> 				exit(1);
> 			}
> 			return sig_sign(identity_file, cert_principals,
>-			    argc, argv);
>+			    argc, argv, opts, nopts);
> 		} else if (strncmp(sign_op, "check-novalidate", 16) == 0) {
> 			if (ca_key_path == NULL) {
> 				error("Too few arguments for check-novalidate: "
>-- 
>2.30.2
>
>_______________________________________________
>openssh-unix-dev mailing list
>openssh-unix-dev at mindrot.org
>https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev