Vulnerability: 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.
Vulnerability ID: CVE-2021-28041
CWE: CWE-415
Severity score: 6.4
Project: https://github.com/openssh/openssh-portable
Published: 2021-03-05
File: ssh-agent.c
Commit message: upstream: factor SSH_AGENT_CONSTRAIN_EXTENSION parsing into its own

function and remove an unused variable; ok dtucker@

OpenBSD-Commit-ID: e1a938657fbf7ef0ba5e73b30365734a0cc96559
Commit author: djm@openbsd.org

Vulnerable version (before fix):
 /* $OpenBSD: ssh-agent.c,v 1.276 2021/02/02 22:35:14 djm Exp $ */ /*  * Author: Tatu Ylonen <ylo@cs.hut.fi>  * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland reaper(void) 		return (deadline - now); }  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; 		} 		switch (ctype) { 		case SSH_AGENT_CONSTRAIN_LIFETIME: 			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; 			} 			*deathp = monotime() + seconds; 			*secondsp = seconds; 			break; 		case SSH_AGENT_CONSTRAIN_CONFIRM: 			if (*confirmp != 0) { 				error_f("confirm already set"); 				goto err; 			} 			*confirmp = 1; 			break; 		case SSH_AGENT_CONSTRAIN_MAXSIGN: 			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; 			} 			if ((r = sshkey_enable_maxsign(k, maxsign)) != 0) { 				error_fr(r, "enable maxsign"); 				goto err; 			} 			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); 			break; 		default: 			error_f("Unknown constraint %d", ctype);  err: 			free(ext_name); 			sshbuf_free(b); 			return -1; 		} 	} 	/* success */ 	return 0; }  static void

Fixed version (after fix):
 /* $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 reaper(void) 		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;  	while (sshbuf_len(m)) { 		if ((r = sshbuf_get_u8(m, &ctype)) != 0) { 			error_fr(r, "parse constraint type"); 			goto out; 		} 		switch (ctype) { 		case SSH_AGENT_CONSTRAIN_LIFETIME: 			if (*deathp != 0) { 				error_f("lifetime already set"); 				r = SSH_ERR_INVALID_FORMAT; 				goto out; 			} 			if ((r = sshbuf_get_u32(m, &seconds)) != 0) { 				error_fr(r, "parse lifetime constraint"); 				goto out; 			} 			*deathp = monotime() + seconds; 			*secondsp = seconds; 			break; 		case SSH_AGENT_CONSTRAIN_CONFIRM: 			if (*confirmp != 0) { 				error_f("confirm already set"); 				r = SSH_ERR_INVALID_FORMAT; 				goto out; 			} 			*confirmp = 1; 			break; 		case SSH_AGENT_CONSTRAIN_MAXSIGN: 			if (k == NULL) { 				error_f("maxsign not valid here"); 				r = SSH_ERR_INVALID_FORMAT; 				goto out; 			} 			if (maxsign != 0) { 				error_f("maxsign already set"); 				r = SSH_ERR_INVALID_FORMAT; 				goto out; 			} 			if ((r = sshbuf_get_u32(m, &maxsign)) != 0) { 				error_fr(r, "parse maxsign constraint"); 				goto out; 			} 			if ((r = sshkey_enable_maxsign(k, maxsign)) != 0) { 				error_fr(r, "enable maxsign"); 				goto out; 			} 			break; 		case SSH_AGENT_CONSTRAIN_EXTENSION: 			if ((r = parse_key_constraint_extension(m, 			    sk_providerp)) != 0) 				goto out; /* error already logged */ 			break; 		default: 			error_f("Unknown constraint %d", ctype); 			r = SSH_ERR_FEATURE_UNSUPPORTED; 			goto out; 		} 	} 	/* success */ 	r = 0;  out: 	return r; }  static void

Patch:
patch: @@ -1,4 +1,4 @@
-/* $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
@@ -574,96 +574,114 @@ reaper(void)
 		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
path: ssh-agent.c