problem with AuthorizedKeysCommand on OpenBSD

[Landry Breuil]

On Tue, Nov 13, 2012 at 10:21:27PM +0100, Landry Breuil wrote:
> On Tue, Nov 13, 2012 at 09:38:58PM +0100, Landry Breuil wrote:
> > On Tue, Nov 13, 2012 at 03:34:28PM -0500, Michael W. Lucas wrote:
> > > Hi,
> > > 
> > > I'm attempting to test the AuthorizedKeysCommand feature with the new
> > > port of ssh-ldap-wrapper to OpenBSD.  I'm running yesterday's
> > > OpenBSD-current i386 snapshot, which includes AuthorizedKeysCommand.
> > > 
> > > The port of ssh-ldap-helper (at
> > > http://old.nabble.com/-new--ssh-ldap-helper-td34667413.html) contains
> > > all the bits I need, and the individual pieces appear to work once
> > > configured:
> > > 
> > > # sudo -u nobody /usr/local/libexec/ssh-ldap-wrapper mwlucas
> > > ssh-rsa AAAAB3NzaC1yc...
> > > 
> > > ssh-rsa AAAAB3NzaC1yc2EA...
> > > 
> > > (Two keys come out, with a blank line between them)
> > > 
> > > My sshd_config has:
> > > 
> > > AuthorizedKeysCommand /usr/local/libexec/ssh-ldap-wrapper
> > > AuthorizedKeysCommandUser nobody
> > > 
> > > (Yes, a user other than nobody will go into production, but I'm just
> > > trying to make the blasted thing work right now.)
> > > 
> > > The keys don't seem to be making it to the SSH server, however. Run
> > > with debugging, I get:
> > > 
> > > ...
> > > debug3: mm_request_receive entering
> > > debug3: monitor_read: checking request 20
> > > debug3: mm_answer_keyallowed entering
> > > debug3: mm_answer_keyallowed: key_from_blob: 0x81973440
> > > debug1: temporarily_use_uid: 32767/32767 (e=0/0)
> > > debug3: Running AuthorizedKeysCommand: "/usr/local/libexec/ssh-ldap-wrapper" as "nobody"
> > > debug1: restore_uid: 0/0
> > > debug1: temporarily_use_uid: 32767/32767 (e=0/0)
> > > debug2: key not found
> > > user_key_command_allowed2: dup2: Bad file descriptor
> > > AuthorizedKeysCommand /usr/local/libexec/ssh-ldap-wrapper returned status 1
> > > debug1: restore_uid: 0/0
> > > debug1: temporarily_use_uid: 1000/1000 (e=0/0)
> > > ...
> > > 
> > > 
> > > Any suggestions, folks?
> > 
> > I was testing it on my shellbox, and i'm stuck at the exact same
> > problem. dup2() on the child's stdout returns EBADF. same if
> > AuthorizedKeysCommandUser is root.
> 
> After examinining the code in user_key_command_allowed2(), i've noted
> a strange thing (which doesnt explain the EBADF on dup2(p[1],
> STDOUT_FILENO)) :
> 
> If AuthorizedKeysCommandUser is say, nobody, then pw->pw_name (3rd arg
> of execl()) is 'nobody' when spawning the helper command (line 550 of
> auth2-pubkey.c), shouldnt it be user_pw->pw_name ? It works by accident
> if AuthorizedKeysCommandUser is %u, in that case user_pw == pw,
> otherwise the wrong username might be passed to the helper command.
> 
> So should that diff apply ?
> diff -u -r1.32 auth2-pubkey.c
> --- auth2-pubkey.c      4 Nov 2012 10:38:43 -0000       1.32
> +++ auth2-pubkey.c      13 Nov 2012 21:16:53 -0000
> @@ -547,7 +547,7 @@
>                 }
>  
>                 execl(options.authorized_keys_command,
> -                   options.authorized_keys_command, pw->pw_name, NULL);
> +                   options.authorized_keys_command, user_pw->pw_name, NULL);
> 
> Also tried with UsePrivilegeSeparation yes to avoid using the sandbox,
> same EBADF issue.

And i think i've found the cause of the issue;

closefrom(STDERR_FILENO + 1) line 523 closes both p[0] and p[1],
probably explaing the dup2(p[1], STDOUT_FILENO) failure. It was moved
before setresgid() call in last commit making AuthorizedKeysCommandUser
mandatory, while before it was after the dup2() block.

Index: auth2-pubkey.c
===================================================================
RCS file: /cvs/src/usr.bin/ssh/auth2-pubkey.c,v
retrieving revision 1.32
diff -u -r1.32 auth2-pubkey.c
--- auth2-pubkey.c      4 Nov 2012 10:38:43 -0000       1.32
+++ auth2-pubkey.c      13 Nov 2012 21:40:44 -0000
@@ -520,7 +520,6 @@
                for (i = 0; i < NSIG; i++)
                        signal(i, SIG_DFL);
 
-               closefrom(STDERR_FILENO + 1);
                /* Don't use permanently_set_uid() here to avoid fatal() */
                if (setresgid(pw->pw_gid, pw->pw_gid, pw->pw_gid) != 0) {
                        error("setresgid %u: %s", (u_int)pw->pw_gid,
@@ -545,9 +544,10 @@
                        error("%s: dup2: %s", __func__, strerror(errno));
                        _exit(1);
                }
+               closefrom(STDERR_FILENO + 1);
 
                execl(options.authorized_keys_command,
-                   options.authorized_keys_command, pw->pw_name, NULL);
+                   options.authorized_keys_command, user_pw->pw_name, NULL);
 
                error("AuthorizedKeysCommand %s exec failed: %s",
                    options.authorized_keys_command, strerror(errno));

fixes both issues for me. Working with %u or nobody in AuthorizedKeysCommandUser.
looking for okays to commit that, if that looks sane.

Landry