openssh bugs - Dec 2016 - [Bug 2655] New: AuthorizedKeysCommand with large output can deadlock

https://bugzilla.mindrot.org/show_bug.cgi?id=2655

            Bug ID: 2655
           Summary: AuthorizedKeysCommand with large output can deadlock
           Product: Portable OpenSSH
           Version: 7.2p2
          Hardware: All
                OS: Linux
            Status: NEW
          Severity: normal
          Priority: P5
         Component: sshd
          Assignee: unassigned-bugs at mindrot.org
          Reporter: jboning at gmail.com

If an AuthorizedKeysCommand produces a large amount of output, a
deadlock can result. The relevant code is in auth2-pubkey.c, beginning
at line 1041:

        if ((pid = subprocess("AuthorizedKeysCommand", pw, command,
            ac, av, &f)) == 0)
                goto out;

        uid_swapped = 1;
        temporarily_use_uid(pw);

        ok = check_authkeys_file(f, options.authorized_keys_command,
key, pw);

        if (exited_cleanly(pid, "AuthorizedKeysCommand", command) !=
0)
                goto out;

Upon finding the correct public key in the command's output, we
immediately wait() for the command to exit. However, the command may
continue to generate output; if the subsequent output is large enough
to fill up the pipe's buffer, the command will block on write() and
never exit, resulting in deadlock.

I believe adding "fclose(f); f = NULL;" after the
check_authkeys_file()
call will fix this. (There was indeed an fclose() after the
check_authkeys_file() call prior to v1.50 of auth2-pubkey.c)

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2655

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |djm at mindrot.org
             Blocks|                            |2647

--- Comment #1 from Damien Miller <djm at mindrot.org> ---
Nice catch. The same problem exists with AuthorizedPrincipalsCommand
too.

I've committed your fix for both cases.


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=2647
[Bug 2647] Tracking bug for OpenSSH 7.5 release
-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2655

--- Comment #2 from Josiah Boning <jboning at gmail.com> ---
Great, thank you!

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2655

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|NEW                         |RESOLVED

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2655

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|FIXED                       |---
             Status|RESOLVED                    |REOPENED

--- Comment #3 from Damien Miller <djm at mindrot.org> ---
The fix that was committed causes a new problem: helper commands may
now receive SIGPIPE when their output is closed early.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2655

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dtucker at zip.com.au
           Assignee|unassigned-bugs at mindrot.org |djm at mindrot.org
   Attachment #2931|                            |ok?(dtucker at zip.com.au)
              Flags|                            |

--- Comment #4 from Damien Miller <djm at mindrot.org> ---
Created attachment 2931
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2931&action=edit
consume entire output of keys/principals on success

This eats the remaining output of the keys/principals file when we
successfully match.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2655

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2931|ok?(dtucker at zip.com.au)     |ok+
              Flags|                            |

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2655

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|REOPENED                    |RESOLVED

--- Comment #5 from Damien Miller <djm at mindrot.org> ---
fix committed - thanks.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2655

Jim Knoble <jmknoble at pobox.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jmknoble at pobox.com

--- Comment #6 from Jim Knoble <jmknoble at pobox.com> ---
If an sshd process dies while reading the output from
Authorized{Keys,Pricipals}Command, wouldn't it also die with SIGPIPE?
Wouldn't it be more resilient to require the command to handle SIGPIPE
appropriately ... or even to set up an appropriate handler before
spawning the command?

Continuing to consume unneeded output seems like the wrong thing to do
here.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2655

--- Comment #7 from Damien Miller <djm at mindrot.org> ---
(In reply to Jim Knoble from comment #6)
> Continuing to consume unneeded output seems like the wrong thing to
> do here.
Why?

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2655

--- Comment #8 from Damien Miller <djm at mindrot.org> ---
BTW setting a SIGPIPE handler isn't possible as signal handlers are
reset on exec

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2655

--- Comment #9 from Jim Knoble <jmknoble at pobox.com> ---
(In reply to Damien Miller from comment #7)
> (In reply to Jim Knoble from comment #6)
> 
> > Continuing to consume unneeded output seems like the wrong thing to
> > do here.
> 
> Why?
For large amounts of additional output, it introduces lag in the
processing of authorized keys; which in turn could cause issues in
scalability.

It also seems to "promise" that the output will always be read,
whereas
calling out up front that it may not all be read encourages the writers
of AuthorizedKeysCommand's to write code that handles such failures
resiliently. 


(In reply to Damien Miller from comment #8)
> BTW setting a SIGPIPE handler isn't possible as signal handlers are
> reset on exec
You're right, of course. I was thinking of *ignoring* SIGPIPE when I
wrote this, which, although it would survive execve(2), is clearly not
the right thing to do here. Chalk this one up to "it's been too long
since I've done this". Perhaps I should stop armchair
quarterbacking....

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2655

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #10 from Damien Miller <djm at mindrot.org> ---
Close all resolved bugs after release of OpenSSH 7.7.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.