CVE-2021-28041: Double Free (CWE-415)
value: ssh-agent in OpenSSH before 8.5 has a double free that may be relevant in a few less-common scenarios, such as unconstrained agent-socket access on a legacy operating system, or the forwarding of an agent to an attacker-controlled host.
CWE description: The product calls free() twice on the same memory address, potentially leading to modification of unexpected memory locations.
CVSS v3 base score: 7.1
Published: 2021-03-05T21:15Z
Repository: https://github.com/openssh/openssh-portable
Language: C
Files changed: ssh-agent.c
Commit date: 2021-02-17 15:03:41 +1100
Commit message: upstream: factor SSH_AGENT_CONSTRAIN_EXTENSION parsing into its own

function and remove an unused variable; ok dtucker@

OpenBSD-Commit-ID: e1a938657fbf7ef0ba5e73b30365734a0cc96559

Diff: diff --git a/ssh-agent.c b/ssh-agent.c
index 9bf6b08050..58fe6ddf75 100644
--- a/ssh-agent.c
+++ b/ssh-agent.c
@@ -1,6 +1,6 @@
-/* $OpenBSD: ssh-agent.c,v 1.276 2021/02/02 22:35:14 djm Exp $ */
+/* $OpenBSD: ssh-agent.c,v 1.277 2021/02/12 03:14:18 djm Exp $ */
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
  * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
  *                    All rights reserved
  * The authentication agent program.
@@ -572,100 +572,118 @@ reaper(void)
 		return 0;
 	else
 		return (deadline - now);
 }
 
+static int
+parse_key_constraint_extension(struct sshbuf *m, char **sk_providerp)
+{
+	char *ext_name = NULL;
+	int r;
+
+	if ((r = sshbuf_get_cstring(m, &ext_name, NULL)) != 0) {
+		error_fr(r, "parse constraint extension");
+		goto out;
+	}
+	debug_f("constraint ext %s", ext_name);
+	if (strcmp(ext_name, "sk-provider@openssh.com") == 0) {
+		if (sk_providerp == NULL) {
+			error_f("%s not valid here", ext_name);
+			r = SSH_ERR_INVALID_FORMAT;
+			goto out;
+		}
+		if (*sk_providerp != NULL) {
+			error_f("%s already set", ext_name);
+			r = SSH_ERR_INVALID_FORMAT;
+			goto out;
+		}
+		if ((r = sshbuf_get_cstring(m, sk_providerp, NULL)) != 0) {
+			error_fr(r, "parse %s", ext_name);
+			goto out;
+		}
+	} else {
+		error_f("unsupported constraint \"%s\"", ext_name);
+		r = SSH_ERR_FEATURE_UNSUPPORTED;
+		goto out;
+	}
+	/* success */
+	r = 0;
+ out:
+	free(ext_name);
+	return r;
+}
+
 static int
 parse_key_constraints(struct sshbuf *m, struct sshkey *k, time_t *deathp,
     u_int *secondsp, int *confirmp, char **sk_providerp)
 {
 	u_char ctype;
 	int r;
 	u_int seconds, maxsign = 0;
-	char *ext_name = NULL;
-	struct sshbuf *b = NULL;
 
 	while (sshbuf_len(m)) {
 		if ((r = sshbuf_get_u8(m, &ctype)) != 0) {
 			error_fr(r, "parse constraint type");
-			goto err;
+			goto out;
 		}
 		switch (ctype) {
 		case SSH_AGENT_CONSTRAIN_LIFETIME:
 			if (*deathp != 0) {
 				error_f("lifetime already set");
-				goto err;
+				r = SSH_ERR_INVALID_FORMAT;
+				goto out;
 			}
 			if ((r = sshbuf_get_u32(m, &seconds)) != 0) {
 				error_fr(r, "parse lifetime constraint");
-				goto err;
+				goto out;
 			}
 			*deathp = monotime() + seconds;
 			*secondsp = seconds;
 			break;
 		case SSH_AGENT_CONSTRAIN_CONFIRM:
 			if (*confirmp != 0) {
 				error_f("confirm already set");
-				goto err;
+				r = SSH_ERR_INVALID_FORMAT;
+				goto out;
 			}
 			*confirmp = 1;
 			break;
 		case SSH_AGENT_CONSTRAIN_MAXSIGN:
 			if (k == NULL) {
 				error_f("maxsign not valid here");
-				goto err;
+				r = SSH_ERR_INVALID_FORMAT;
+				goto out;
 			}
 			if (maxsign != 0) {
 				error_f("maxsign already set");
-				goto err;
+				r = SSH_ERR_INVALID_FORMAT;
+				goto out;
 			}
 			if ((r = sshbuf_get_u32(m, &maxsign)) != 0) {
 				error_fr(r, "parse maxsign constraint");
-				goto err;
+				goto out;
 			}
 			if ((r = sshkey_enable_maxsign(k, maxsign)) != 0) {
 				error_fr(r, "enable maxsign");
-				goto err;
+				goto out;
 			}
 			break;
 		case SSH_AGENT_CONSTRAIN_EXTENSION:
-			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_providerp == NULL) {
-					error_f("%s not valid here", ext_name);
-					goto err;
-				}
-				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;
-				}
-			} else {
-				error_f("unsupported constraint \"%s\"",
-				    ext_name);
-				goto err;
-			}
-			free(ext_name);
+			if ((r = parse_key_constraint_extension(m,
+			    sk_providerp)) != 0)
+				goto out; /* error already logged */
 			break;
 		default:
 			error_f("Unknown constraint %d", ctype);
- err:
-			free(ext_name);
-			sshbuf_free(b);
-			return -1;
+			r = SSH_ERR_FEATURE_UNSUPPORTED;
+			goto out;
 		}
 	}
 	/* success */
-	return 0;
+	r = 0;
+ out:
+	return r;
 }
 
 static void
 process_add_identity(SocketEntry *e)
 {

Vulnerable code: /* $OpenBSD: ssh-agent.c,v 1.276 2021/02/02 22:35:14 djm Exp $ */
	char *ext_name = NULL;
	struct sshbuf *b = NULL;
			goto err;
				goto err;
				goto err;
				goto err;
				goto err;
				goto err;
				goto err;
				goto err;
			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_providerp == NULL) {
					error_f("%s not valid here", ext_name);
					goto err;
				}
				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;
				}
			} else {
				error_f("unsupported constraint \"%s\"",
				    ext_name);
				goto err;
			}
			free(ext_name);
 err:
			free(ext_name);
			sshbuf_free(b);
			return -1;
	return 0;
Fixed code: /* $OpenBSD: ssh-agent.c,v 1.277 2021/02/12 03:14:18 djm Exp $ */
static int
parse_key_constraint_extension(struct sshbuf *m, char **sk_providerp)
{
	char *ext_name = NULL;
	int r;

	if ((r = sshbuf_get_cstring(m, &ext_name, NULL)) != 0) {
		error_fr(r, "parse constraint extension");
		goto out;
	}
	debug_f("constraint ext %s", ext_name);
	if (strcmp(ext_name, "sk-provider@openssh.com") == 0) {
		if (sk_providerp == NULL) {
			error_f("%s not valid here", ext_name);
			r = SSH_ERR_INVALID_FORMAT;
			goto out;
		}
		if (*sk_providerp != NULL) {
			error_f("%s already set", ext_name);
			r = SSH_ERR_INVALID_FORMAT;
			goto out;
		}
		if ((r = sshbuf_get_cstring(m, sk_providerp, NULL)) != 0) {
			error_fr(r, "parse %s", ext_name);
			goto out;
		}
	} else {
		error_f("unsupported constraint \"%s\"", ext_name);
		r = SSH_ERR_FEATURE_UNSUPPORTED;
		goto out;
	}
	/* success */
	r = 0;
 out:
	free(ext_name);
	return r;
}

			goto out;
				r = SSH_ERR_INVALID_FORMAT;
				goto out;
				goto out;
				r = SSH_ERR_INVALID_FORMAT;
				goto out;
				r = SSH_ERR_INVALID_FORMAT;
				goto out;
				r = SSH_ERR_INVALID_FORMAT;
				goto out;
				goto out;
				goto out;
			if ((r = parse_key_constraint_extension(m,
			    sk_providerp)) != 0)
				goto out; /* error already logged */
			r = SSH_ERR_FEATURE_UNSUPPORTED;
			goto out;
	r = 0;
 out:
	return r;