Hi, I'm in the position of having to support a fix for a bad interaction between sshd and winbind/Active Directory. It's solved by a small patch against openssh, but it would be nice to have the solution generally available. The problem has previously been described on this list by Andreas Schneider, see: https://lists.mindrot.org/pipermail/openssh-unix-dev/2019-February/037556.html That's the last mention of this I could find in the archives. Was a final decision reached on whether that patch (or something similar) would be accepted? -- Hans Petter -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 488 bytes Desc: This is a digitally signed message part URL: <http://lists.mindrot.org/pipermail/openssh-unix-dev/attachments/20200526/8f0922bb/attachment.asc>
On 27/5/20 12:41 am, Hans Petter Jansson wrote:> Hi, I'm in the position of having to support a fix for a bad > interaction between sshd and winbind/Active Directory. It's solved by a > small patch against openssh, but it would be nice to have the solution > generally available. > > The problem has previously been described on this list by Andreas > Schneider, see: > > https://lists.mindrot.org/pipermail/openssh-unix-dev/2019-February/037556.htmlI have two comments: First, in the patch, I think it's insufficient to free(s->pw) as s->pw probably has copies of strings.? See end of pwcopy() in misc.c. Second, might userauth_finish() in auth2.c be a better place to reload the struct passwd? It does seem like something which deserves to be fixed.? Don't let it drop.
On Tue, 2020-05-26 at 17:11 +0200, Hans Petter Jansson wrote:> Hi, I'm in the position of having to support a fix for a bad > interaction between sshd and winbind/Active Directory. It's solved by > a > small patch against openssh, but it would be nice to have the > solution > generally available. > > The problem has previously been described on this list by Andreas > Schneider, see: > > https://lists.mindrot.org/pipermail/openssh-unix-dev/2019-February/037556.html > > That's the last mention of this I could find in the archives. Was a > final decision reached on whether that patch (or something similar) > would be accepted?Did you try that patch and it solved the issue for you? We tried and we were not able to verify it fixes the described issue. Moreover this original patch is broken in systems where two users have same UID. I tried to tweak it a bit (see the attached patch) to avoid these issues, but still we were not able to verify it fixes the described issue so we do not ship it. I did not look into this much, but if I am right, the group information is cached in uidswap.c too so it might need some more work to be working. Whether it will be accepted here, is other question. Hope it helps, -- Jakub Jelen Senior Software Engineer Security Technologies Red Hat, Inc. -------------- next part -------------- commit 3899be76a3fdbf366ee7143ce38f53a1546b65ae Author: Jakub Jelen <jjelen at redhat.com> Date: Fri May 31 13:24:34 2019 +0200 Update cached pw structure after successful authetnication through PAM diff --git a/session.c b/session.c index f2c3abde..e25f1e82 100644 --- a/session.c +++ b/session.c @@ -1515,9 +1515,21 @@ do_child(struct ssh *ssh, Session *s, const char *command) extern char **environ; char **env, *argv[ARGV_MAX], remote_id[512]; const char *shell, *shell0; - struct passwd *pw = s->pw; + struct passwd *pw = NULL; int r = 0; + /* Update the users passwd structure after successful login */ + pw = pwcopy(getpwnam(s->pw->pw_name)); + if (pw != NULL) { + s->pw = pw; + /* Fix also the original location where we copied + * the pw structure from, to be sure. */ + free(s->authctxt->pw); + s->authctxt->pw = pw; + } else { + pw = s->pw; + } + sshpkt_fmt_connection_id(ssh, remote_id, sizeof(remote_id));
On Wed, 2020-05-27 at 09:27 +0200, Jakub Jelen wrote:> On Tue, 2020-05-26 at 17:11 +0200, Hans Petter Jansson wrote:> > https://lists.mindrot.org/pipermail/openssh-unix-dev/2019-February/037556.html > > > > That's the last mention of this I could find in the archives. Was a > > final decision reached on whether that patch (or something similar) > > would be accepted? > > Did you try that patch and it solved the issue for you? We tried and > we > were not able to verify it fixes the described issue. > > Moreover this original patch is broken in systems where two users > have > same UID. > > I tried to tweak it a bit (see the attached patch) to avoid these > issues, but still we were not able to verify it fixes the described > issue so we do not ship it. > > I did not look into this much, but if I am right, the group > information > is cached in uidswap.c too so it might need some more work to be > working. Whether it will be accepted here, is other question. > > Hope it helps,Thanks, it does. I haven't tried the patch from Andreas, but I got positive feedback on one I wrote. That one may be bad for other reasons, though :) I'm attaching it anyway. Reportedly this only works with nscd disabled. Otherwise it will also cache the bad GID. -- Hans Petter -------------- next part -------------- A non-text attachment was scrubbed... Name: 0001-Refresh-primary-group-ID-after-successful-authentica.patch Type: text/x-patch Size: 1515 bytes Desc: not available URL: <http://lists.mindrot.org/pipermail/openssh-unix-dev/attachments/20200527/fcf9857c/attachment.bin> -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 488 bytes Desc: This is a digitally signed message part URL: <http://lists.mindrot.org/pipermail/openssh-unix-dev/attachments/20200527/fcf9857c/attachment.asc>
On Wed, 2020-05-27 at 16:27 +0930, David Newall wrote:> On 27/5/20 12:41 am, Hans Petter Jansson wrote:> > https://lists.mindrot.org/pipermail/openssh-unix-dev/2019-February/037556.html > > I have two comments: > First, in the patch, I think it's insufficient to free(s->pw) as s- > >pw probably has copies of strings. See end of pwcopy() in misc.c. > Second, might userauth_finish() in auth2.c be a better place to > reload the struct passwd? > It does seem like something which deserves to be fixed. Don't let it > drop.auth2.c:userauth_finish() does seem like a good place. I tried that first, but privsep complicates it somewhat. I wasn't able to figure out a way to do it without adding monitor code for getpwnam(); as far as I can tell, getpwnamallow() is only meant to be called once, and it also does quite a bit of extra work (config parsing etc) that we don't need the second time around. -- Hans Petter -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 488 bytes Desc: This is a digitally signed message part URL: <http://lists.mindrot.org/pipermail/openssh-unix-dev/attachments/20200527/d9b4434f/attachment.asc>
Apparently Analagous Threads
- [Bug 559] PAM fixes
- No subject
- [patch] ssh.c load_public_identity_files calls getpwuid twice without copy
- Re: [PATCH v3 1/4] mltools: Rename Yajl module as JSON_parser and move to common/mltools.
- [Bug 702] dont call userauth_finish after auth2_challenge_stop