On Wed, 25 Nov 2020, Yuichiro NAITO wrote:
> Thanks for reviewing my patch.
>
> > 2020/11/20 23:45?Peter Stuge <peter at stuge.se>????:
> >
> > Yuichiro NAITO wrote:
> >> Take a look at my GitHub pull request to see my patch.
> >>
> >> https://github.com/openssh/openssh-portable/pull/216
> >
> > I think the length at the beginning should be tied to the (number of?)
> > members that are sent instead of the struct passwd size on either
side.
>
> OK.
> I fixed to send number of struct passwd members at first in
sshbuf_put_passwd().
> And sshbuf_get_passwd() checks it.
Thanks for reminding me about this.
IMO sshbuf-*.c isn't the right place for this. Sending/receiving password
structs is only done in one place in OpenSSH, so I'd prefer to leave it
where it is.
I think that its easier to always send the values as 64 bit quantities,
but POSIX doesn't guarantee that uid_t, gid_t and time_t are unsigned
so I think it is safest to explicitly encode the sign of these values.
(I'm not worried about signed/unsigned overflow upon decoding: the
process doing the decoding is unprivileged, sandboxed and already
completely trusts the privileged process to send it good data.)
So something like this perhaps (against OpenBSD):
diff --git a/monitor.c b/monitor.c
index d71520b..ec484ca 100644
--- a/monitor.c
+++ b/monitor.c
@@ -639,8 +639,15 @@ mm_answer_sign(struct ssh *ssh, int sock, struct sshbuf *m)
return (0);
}
+#define PUTPW(b, id) \
+ do { \
+ if ((r = sshbuf_put_u8(b, pwent->id < 0)) != 0 || \
+ (r = sshbuf_put_u64(b, pwent->id < 0 ? \
+ -pwent->id : pwent->id)) != 0) \
+ fatal_fr(r, "assemble pw %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 +683,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..a6193be 100644
--- a/monitor_wrap.c
+++ b/monitor_wrap.c
@@ -242,6 +242,16 @@ mm_sshkey_sign(struct ssh *ssh, struct sshkey *key, u_char
**sigp, size_t *lenp,
return (0);
}
+#define GETPW(b, id) \
+ do { \
+ u_char _neg = 0; \
+ int64_t _i = 0; \
+ if ((r = sshbuf_get_u8(b, &_neg)) != 0 || \
+ (r = sshbuf_get_u64(b, &_i)) != 0) \
+ fatal_fr(r, "parse pw %s", #id); \
+ pw->id = _neg ? -_i : _i; \
+ } while (0)
+
struct passwd *
mm_getpwnamallow(struct ssh *ssh, const char *username)
{
@@ -273,14 +283,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 ||