On Wed, 25 Nov 2020, Yuichiro NAITO wrote: > Thanks for reviewing my patch. > > > 2020/11/20 23:45、Peter Stuge <peter@xxxxxxxx>のメール: > > > > 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 || _______________________________________________ openssh-unix-dev mailing list openssh-unix-dev@xxxxxxxxxxx https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
