Mailing List Archive

avoid sending pointer values in struct passwd
Hi, I wrote a patch to improve sending struct passwd value.
And I believe one ToDo comment will be finished.

When ssh forks child process for authentication,
parent process proxies pwnamallow() execution in mm_answer_pwnamallow().

Through this proxy, struct passwd values are sent via UNIX domain socket.
That includes pointer values because whole structure memory is copied
by memcpy() .
Fortunately all pointer members are replaced by actual strings
(e.g. pw_name, pw_passwd, ...) for now.

But if a new pointer member is added, it will never be rewritten and can be a
wild pointer. My patch avoids this.

Take a look at my GitHub pull request to see my patch.

https://github.com/openssh/openssh-portable/pull/216

Thank you!

--
Yuichiro NAITO (naito.yuichiro@gmail.com)
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
Re: avoid sending pointer values in struct passwd [ In reply to ]
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.

Also, adding passwd.fields seems to be an unrelated change, better
placed in a separate commit.

Finally, sshbuf_free_passwd() is added but never called. If it is not
needed then I think it's better to not add it (yet).


//Peter
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
Re: avoid sending pointer values in struct passwd [ In reply to ]
Thanks for reviewing my patch.

> 2020/11/20 23:45?Peter Stuge <peter@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.

> Also, adding passwd.fields seems to be an unrelated change, better
> placed in a separate commit.

It is used for FreeBSD sturct passwd.
I made a different commit and wrote about this in the message.
So I force pushed new branch to the same Pull Request.
Please refer to the new one.

> Finally, sshbuf_free_passwd() is added but never called. If it is not
> needed then I think it's better to not add it (yet).

sshbuf_free_passwd() is used for error case of sshbuf_get_passwd().
I think it is easy to read and worth to be remained.


Yuichiro NAITO
naito.yuichiro@gmail.com




_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
Re: avoid sending pointer values in struct passwd [ In reply to ]
On Wed, 25 Nov 2020, Yuichiro NAITO wrote:

> Thanks for reviewing my patch.
>
> > 2020/11/20 23:45?Peter Stuge <peter@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 ||
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
Re: avoid sending pointer values in struct passwd [ In reply to ]
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.

*************************************************
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
Re: avoid sending pointer values in struct passwd [ In reply to ]
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 ||
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
Re: avoid sending pointer values in struct passwd [ In reply to ]
On Wed, 25 Nov 2020, Damien Miller wrote:

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

Right, that’ll work just as well. (But I at least got a way
to determine signedness out of this ;-)

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.

*************************************************
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
Re: avoid sending pointer values in struct passwd [ In reply to ]
2020?11?25?(?) 9:11 Damien Miller <djm@mindrot.org>:
>
> On Wed, 25 Nov 2020, Yuichiro NAITO wrote:
>
> > Thanks for reviewing my patch.
> >
> > > 2020/11/20 23:45?Peter Stuge <peter@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@gmail.com)
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev