avoid sending pointer values in struct passwd

[Damien Miller]

On Wed, 25 Nov 2020, Thorsten Glaser wrote:

> On Wed, 25 Nov 2020, Damien Miller wrote:
> 
> > +		    (r = sshbuf_put_u64(b, pwent->id < 0 ? \
> > +		    -pwent->id : pwent->id)) != 0) \
> 
> *cough* -2⁶³ will trigger UB/IB on this.

ugh yeah.

Well, then we can copy their bytes directly. Copying integer
types via an intermediate binary representation of u_char[] is safe*

* http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf s6.2.6.1p4


diff --git a/monitor.c b/monitor.c
index d71520b..be47e8c 100644
--- a/monitor.c
+++ b/monitor.c
@@ -639,8 +639,14 @@ mm_answer_sign(struct ssh *ssh, int sock, struct sshbuf *m)
 	return (0);
 }
 
+#define PUTPW(b, id) \
+	do { \
+		if ((r = sshbuf_put_string(b, \
+		    &pwent->id, sizeof(pwent->id))) != 0) \
+			fatal_fr(r, "assemble %s", #id); \
+	} while (0)
+
 /* Retrieves the password entry and also checks if the user is permitted */
-
 int
 mm_answer_pwnamallow(struct ssh *ssh, int sock, struct sshbuf *m)
 {
@@ -676,10 +682,14 @@ mm_answer_pwnamallow(struct ssh *ssh, int sock, struct sshbuf *m)
 	authctxt->pw = pwent;
 	authctxt->valid = 1;
 
-	/* XXX don't sent pwent to unpriv; send fake class/dir/shell too */
-	if ((r = sshbuf_put_u8(m, 1)) != 0 ||
-	    (r = sshbuf_put_string(m, pwent, sizeof(*pwent))) != 0 ||
-	    (r = sshbuf_put_cstring(m, pwent->pw_name)) != 0 ||
+	/* XXX send fake class/dir/shell, etc. */
+	if ((r = sshbuf_put_u8(m, 1)) != 0)
+		fatal_fr(r, "assemble ok");
+	PUTPW(m, pw_uid);
+	PUTPW(m, pw_gid);
+	PUTPW(m, pw_change);
+	PUTPW(m, pw_expire);
+	if ((r = sshbuf_put_cstring(m, pwent->pw_name)) != 0 ||
 	    (r = sshbuf_put_cstring(m, "*")) != 0 ||
 	    (r = sshbuf_put_cstring(m, pwent->pw_gecos)) != 0 ||
 	    (r = sshbuf_put_cstring(m, pwent->pw_class)) != 0 ||
diff --git a/monitor_wrap.c b/monitor_wrap.c
index d4ab862..0bd1536 100644
--- a/monitor_wrap.c
+++ b/monitor_wrap.c
@@ -242,6 +242,15 @@ mm_sshkey_sign(struct ssh *ssh, struct sshkey *key, u_char **sigp, size_t *lenp,
 	return (0);
 }
 
+#define GETPW(b, id) \
+	do { \
+		if ((r = sshbuf_get_string_direct(b, &p, &len)) != 0) \
+			fatal_fr(r, "parse pw %s", #id); \
+		if (len != sizeof(pw->id)) \
+			fatal_fr(r, "bad length for %s", #id); \
+		memcpy(&pw->id, p, len); \
+	} while (0)
+
 struct passwd *
 mm_getpwnamallow(struct ssh *ssh, const char *username)
 {
@@ -273,14 +282,11 @@ mm_getpwnamallow(struct ssh *ssh, const char *username)
 		goto out;
 	}
 
-	/* XXX don't like passing struct passwd like this */
 	pw = xcalloc(sizeof(*pw), 1);
-	if ((r = sshbuf_get_string_direct(m, &p, &len)) != 0)
-		fatal_fr(r, "parse");
-	if (len != sizeof(*pw))
-		fatal_f("struct passwd size mismatch");
-	memcpy(pw, p, sizeof(*pw));
-
+	GETPW(m, pw_uid);
+	GETPW(m, pw_gid);
+	GETPW(m, pw_change);
+	GETPW(m, pw_expire);
 	if ((r = sshbuf_get_cstring(m, &pw->pw_name, NULL)) != 0 ||
 	    (r = sshbuf_get_cstring(m, &pw->pw_passwd, NULL)) != 0 ||
 	    (r = sshbuf_get_cstring(m, &pw->pw_gecos, NULL)) != 0 ||