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 ||
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. *************************************************
2020?11?25?(?) 9:11 Damien Miller <djm at mindrot.org>:> > 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.Yes, code of sending struct passwd appears in one place. I've tried to reduce the difference between FreeBSD sources, but it can not be a motivation to OpenSSH (that I'm guessing). And you have written the size aware macros which is better than my code in portability. There is no advantage to my code. I think the issue in the subject is solved by your patch. -- Yuichiro NAITO (naito.yuichiro at gmail.com)