Re: multiuser disabled - fail more gracefully
On Wed, 10 Mar 2021 at 12:14, Hans Harder wrote:
> Indeed that is the correct question, because you can easily do
>
> #if DROPBEAR_SVR_MULTIUSER
>if (getuid() != ses.authstate.pw_uid) {
> setgid and setuid part
>}
> #endif
Well yes, if you're confident that setgid() and initgroups() won't
need to be called when the root user logs in, then you could do that.
Here's what I have; it seems to work for me, although I've not done
any wide testing on it other than "it runs and lets me log in to my
system running both the old (multiuser) and the new (non-multiuser)
linux kernel".
Geoff
diff -U 3 -bB dropbear-2020.81/svr-agentfwd.c dropbear-2020.81_gw/svr-agentfwd.c
--- dropbear-2020.81/svr-agentfwd.c 2020-10-29 13:35:50.0 +
+++ dropbear-2020.81_gw/svr-agentfwd.c 2021-03-10 13:28:20.303227469 +
@@ -154,12 +154,14 @@
#if DROPBEAR_SVR_MULTIUSER
/* Remove the dir as the user. That way they can't
cause problems except
* for themselves */
+ if (ses.authstate.pw_uid != 0) {
uid = getuid();
gid = getgid();
if ((setegid(ses.authstate.pw_gid)) < 0 ||
(seteuid(ses.authstate.pw_uid)) < 0) {
dropbear_exit("Failed to set euid");
}
+ }
#endif
/* 2 for "/" and "\0" */
@@ -173,10 +175,12 @@
rmdir(chansess->agentdir);
#if DROPBEAR_SVR_MULTIUSER
+ if (ses.authstate.pw_uid != 0) {
if ((seteuid(uid)) < 0 ||
(setegid(gid)) < 0) {
dropbear_exit("Failed to revert euid");
}
+ }
#endif
m_free(chansess->agentfile);
@@ -221,6 +225,7 @@
int ret = DROPBEAR_FAILURE;
#if DROPBEAR_SVR_MULTIUSER
+ if (ses.authstate.pw_uid != 0) {
/* drop to user privs to make the dir/file */
uid = getuid();
gid = getgid();
@@ -228,6 +233,7 @@
(seteuid(ses.authstate.pw_uid)) < 0) {
dropbear_exit("Failed to set euid");
}
+ }
#endif
memset((void*), 0x0, sizeof(addr));
@@ -269,10 +275,12 @@
out:
#if DROPBEAR_SVR_MULTIUSER
+ if (ses.authstate.pw_uid != 0) {
if ((seteuid(uid)) < 0 ||
(setegid(gid)) < 0) {
dropbear_exit("Failed to revert euid");
}
+ }
#endif
return ret;
}
diff -U 3 -bB dropbear-2020.81/svr-authpubkey.c
dropbear-2020.81_gw/svr-authpubkey.c
--- dropbear-2020.81/svr-authpubkey.c 2020-10-29 13:35:50.0 +
+++ dropbear-2020.81_gw/svr-authpubkey.c2021-03-10
13:31:31.820807682 +
@@ -396,6 +396,7 @@
ses.authstate.pw_dir);
#if DROPBEAR_SVR_MULTIUSER
+ if (ses.authstate.pw_uid != 0) {
/* open the file as the authenticating user. */
origuid = getuid();
origgid = getgid();
@@ -403,15 +404,18 @@
(seteuid(ses.authstate.pw_uid)) < 0) {
dropbear_exit("Failed to set euid");
}
+ }
#endif
authfile = fopen(filename, "r");
#if DROPBEAR_SVR_MULTIUSER
+ if (ses.authstate.pw_uid != 0) {
if ((seteuid(origuid)) < 0 ||
(setegid(origgid)) < 0) {
dropbear_exit("Failed to revert euid");
}
+ }
#endif
if (authfile == NULL) {
diff -U 3 -bB dropbear-2020.81/svr-chansession.c
dropbear-2020.81_gw/svr-chansession.c
--- dropbear-2020.81/svr-chansession.c 2020-10-29 13:35:50.0 +
+++ dropbear-2020.81_gw/svr-chansession.c 2021-03-10
13:25:02.115592221 +
@@ -954,12 +954,14 @@
/* We can only change uid/gid as root ... */
if (getuid() == 0) {
- if ((setgid(ses.authstate.pw_gid) < 0) ||
+ if (((setgid(ses.authstate.pw_gid) < 0) ||
(initgroups(ses.authstate.pw_name,
- ses.authstate.pw_gid) < 0)) {
+ ses.authstate.pw_gid) < 0))
+ && (ses.authstate.pw_uid != 0)) { /* if we're
not changing user, we probably don't mind the fail */
dropbear_exit("Error changing user group");
}
- if (setuid(ses.authstate.pw_uid) < 0) {
+ if ((setuid(ses.authstate.pw_uid) < 0)
+ && (ses.authstate.pw_uid != 0)) { /* if we're
not changing user, we probably don't mind the fail */
dropbear_exit("Error changing user");
}
} else {
Re: multiuser disabled - fail more gracefully
Indeed that is the correct question, because you can easily do
#if DROPBEAR_SVR_MULTIUSER
if (getuid() != ses.authstate.pw_uid) {
setgid and setuid part
}
#endif
On Wed, Mar 10, 2021 at 11:41 AM Geoff Winkless wrote:
>
> On Tue, 9 Mar 2021 at 15:43, Kazuo Kuroi wrote:
> > That's a good suggestion. but I suggest that if your code can't run on
> > UNIX platforms that it would need an include guard against it.
>
> I completely understand your concern.
>
> I would hope that the changes would be system-agnostic: the idea would
> merely be that if the setgroups (or indeed setuid) call fails, it
> checks if the current running user is the same as the login user and
> ignores the failure if so.
>
> It could be simplified further by just skipping all the setuid and
> setgroup code if the login user is the same as the running user, but
> I'm not sure if that would always be acceptable (there may be some
> systems where the group calls need to be made even if the users are
> the same?) so I thought it would be best to add the check after
> failure.
>
> Geoff
Re: multiuser disabled - fail more gracefully
On Tue, 9 Mar 2021 at 15:43, Kazuo Kuroi wrote: > That's a good suggestion. but I suggest that if your code can't run on > UNIX platforms that it would need an include guard against it. I completely understand your concern. I would hope that the changes would be system-agnostic: the idea would merely be that if the setgroups (or indeed setuid) call fails, it checks if the current running user is the same as the login user and ignores the failure if so. It could be simplified further by just skipping all the setuid and setgroup code if the login user is the same as the running user, but I'm not sure if that would always be acceptable (there may be some systems where the group calls need to be made even if the users are the same?) so I thought it would be best to add the check after failure. Geoff
Re: multiuser disabled - fail more gracefully
Hello Geoff,
That's a good suggestion. but I suggest that if your code can't run on
UNIX platforms that it would need an include guard against it. Matt has
taken a lot of time to ensure this not only runs on proprietary old C
compilers, but also on older OSes where OpenSSH is not a good option.
I only express personal hesitancy here because I had to, along with
another dev, meddle greatly in svr-chansession to allow it to run on
IRIX properly - and I'm mildly concerned your proposed change could
introduce linuxisms if it's not include guarded.
I would thus ask that any changes are first included on a Linux include
guard, and then we could test it on other platforms like IRIX or AIX
before we remove it.
Just my .02 as the sorta-kinda maintainer for IRIX.
- Kaz Kuroi
On 3/9/2021 5:18 AM, Geoff Winkless wrote:
Hi
I appreciate that there's an compile-time option
DROPBEAR_SVR_MULTIUSER=0 to skip the setuid/gid sections, but can I
make a humble suggestion that we fail gracefully if someone* runs a
dropbear that _doesn't_ have that option configured on a linux kernel
that's compiled single-user.
*Not me, of course, I would obviously never do anything that stupid...
The idea being, if you're running as root and logging in as root, it
shouldn't matter if the setuid stuff fails, so for example In
svr-chansession.c, something like:
/* We can only change uid/gid as root ... */
if (getuid() == 0) {
-if ((setgid(ses.authstate.pw_gid) < 0) ||
+if ((setgid(ses.authstate.pw_gid) < 0 &&
ses.authstate.pw_uid != 0) ||
(initgroups(ses.authstate.pw_name,
ses.authstate.pw_gid) < 0)) {
dropbear_exit("Error changing user group");
}
-if (setuid(ses.authstate.pw_uid) < 0 &&
ses.authstate.pw_uid != 0) {
+if (setuid(ses.authstate.pw_uid) < 0 &&
ses.authstate.pw_uid != 0) {
dropbear_exit("Error changing user");
}
} else {
There are a few other places (probably everywhere where SVR_MULTIUSER
is checked, I suppose) where the same principle could be applied.
Geoff
multiuser disabled - fail more gracefully
Hi
I appreciate that there's an compile-time option
DROPBEAR_SVR_MULTIUSER=0 to skip the setuid/gid sections, but can I
make a humble suggestion that we fail gracefully if someone* runs a
dropbear that _doesn't_ have that option configured on a linux kernel
that's compiled single-user.
*Not me, of course, I would obviously never do anything that stupid...
The idea being, if you're running as root and logging in as root, it
shouldn't matter if the setuid stuff fails, so for example In
svr-chansession.c, something like:
/* We can only change uid/gid as root ... */
if (getuid() == 0) {
-if ((setgid(ses.authstate.pw_gid) < 0) ||
+if ((setgid(ses.authstate.pw_gid) < 0 &&
ses.authstate.pw_uid != 0) ||
(initgroups(ses.authstate.pw_name,
ses.authstate.pw_gid) < 0)) {
dropbear_exit("Error changing user group");
}
-if (setuid(ses.authstate.pw_uid) < 0 &&
ses.authstate.pw_uid != 0) {
+if (setuid(ses.authstate.pw_uid) < 0 &&
ses.authstate.pw_uid != 0) {
dropbear_exit("Error changing user");
}
} else {
There are a few other places (probably everywhere where SVR_MULTIUSER
is checked, I suppose) where the same principle could be applied.
Geoff
