openssh unix dev - Nov 2020 - avoid sending pointer values in struct passwd

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.


Since sender and recipient are on the same system, you can just?

union a64bitquantity {
	uint64_t u;
	int64_t i;
}

#define PUTPW(b, id) do {				\
	unsigned char issigned = pwent->id < 0;		\
	union a64bitquantity q;				\
							\
	if (issigned)					\
		q.i = pwent->id;			\
	else						\
		q.u = pwent->id;			\
							\
	if ((r = sshbuf_put_u8(b, issigned)) != 0 ||	\
	    (r = sshbuf_put_u64(b, q.u)) != 0)		\
		fatal_fr(r, "assemble pw %s", #id);	\
} while (/* CONSTCOND */ 0)

#define GETPW(b, id) do {				\
	unsigned char issigned;				\
	union a64bitquantity q;				\
							\
	if ((r = sshbuf_get_u8(b, &issigned)) != 0 ||	\
	    (r = sshbuf_get_u64(b, &q.u)) != 0)		\
		fatal_fr(r, "parse pw %s", #id);	\
	/* don?t use a ternary expression here */	\
	if (issigned)					\
		pwent->id = q.i;			\
	else						\
		pwent->id = q.u;			\
} while (/* CONSTCOND */ 0)

Use of such a union is AFAIR safe.

Using a ternary operation could trigger UB/IB by
trying to bring the operands to the same type.


It might actually be easier to try and find out whether
the type is signed or not. I don?t know of a good method,
?the ?net? suggests:

#define	ISUNSIGNED(T)	((T)-1 > 0)

This, unfortunately, doesn?t work as a compile-time check.
But we can do this during configure time:

~ $ gcc -DI='<stdlib.h>' -DT=size_t -c a.c
~ $ gcc -DI='<stdlib.h>' -DT=ssize_t -c a.c
a.c: In function ?foo?:
a.c:4:6: error: size of array ?statictest? is negative
    4 |  int statictest[((T)-1 > 0) ? 1 : -1];
      |      ^~~~~~~~~~
1|~ $ cat a.c
#include I

int foo(void) {
	int statictest[((T)-1 > 0) ? 1 : -1];
	return (0);
}
~ $

Check whether this compiles to set preprocessor
definitions, then use the to select the suitable
function to pass the value.

This is what I feel is safest; ofc YMMV?

bye,
//mirabilos
-- 
tarent solutions GmbH
Rochusstra?e 2-4, D-53123 Bonn ? http://www.tarent.de/
Tel: +49 228 54881-393 ? Fax: +49 228 54881-235
HRB 5168 (AG Bonn) ? USt-ID (VAT): DE122264941
Gesch?ftsf?hrer: Dr. Stefan Barth, Kai Ebenrett, Boris Esser, Alexander Steeg

*************************************************

Mit unserem Consulting bieten wir Unternehmen ma?geschneiderte Angebote in
Form von Beratung, Trainings sowie Workshops in den Bereichen
Softwaretechnologie, IT Strategie und Architektur, Innovation und Umsetzung
sowie Agile Organisation.

Besuchen Sie uns auf https://www.tarent.de/consulting .
Wir freuen uns auf Ihren Kontakt.

*************************************************

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 ||