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)
Nice catch. The same problem exists with AuthorizedPrincipalsCommand too. I've committed your fix for both cases.
Great, thank you!
The fix that was committed causes a new problem: helper commands may now receive SIGPIPE when their output is closed early.
Created attachment 2931 [details] consume entire output of keys/principals on success This eats the remaining output of the keys/principals file when we successfully match.
fix committed - thanks.
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.
(In reply to Jim Knoble from comment #6) > Continuing to consume unneeded output seems like the wrong thing to > do here. Why?
BTW setting a SIGPIPE handler isn't possible as signal handlers are reset on exec
(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....
Close all resolved bugs after release of OpenSSH 7.7.