Merge ~sergiodj/ubuntu/+source/openssh:bug1877454-hangs-authorizedkeyscommand into ubuntu/+source/openssh:ubuntu/xenial-devel

[Sergio Durigan Junior]

When sshd is configured to obtain the list of allowed keys using AuthorizedKeysCommand (or the list of allowed certificate principals using AuthorizedPrincipalsCommand), and if the command pointed by Authorized{Keys,Principals}Command generates a lot of output, sshd will deadlock while reading this same output. In a nutshell, the problem happens when the subprocess generates enough data to fill the pipe's buffer; in this scenario, sshd will wait(2) on the subprocess, which will be blocked trying to write the rest of the output.

There are two upstream fixes that needed to be backported to fix this problem:

https://github.com/openssh/openssh-portable/commit/ddd3d34e5c7979ca6f4a3a98a7d219a4ed3d98c2
https://github.com/openssh/openssh-portable/commit/52763dd3fe0a4678dafdf7aeb32286e514130afc

They make sure that sshd will consume everything that the subprocess generates, and will properly fclose(2) the file handlers used for that.

To reproduce the problem, one can do:

$ lxc launch ubuntu-daily:xenial openssh-server-bug1877454
$ lxc shell openssh-server-bug1877454
# ssh-keygen
(no need to choose a passphrase for the key, just hit ENTER on all prompts)
# cat > authkeyscommand.sh << __EOF__
#!/bin/bash

cat /root/.ssh/id_rsa.pub
echo
head -c 1M < /dev/urandom
__EOF__
# chmod +x authkeyscommand.sh
# cat >> /etc/ssh/sshd_config << __EOF__
AuthorizedKeysCommand /root/authkeyscommand.sh
AuthorizedKeysCommandUser root
__EOF__
# systemctl reload sshd.service
# ssh root@127.0.0.1

You will notice that ssh will stay there waiting for sshd's reply, which won't come.

You can find a PPA with the proposed fix here:

https://launchpad.net/~sergiodj/+archive/ubuntu/openssh-server-bug1877454

autopkgtest is still happy:

autopkgtest [10:53:36]: test regress: -----------------------]
autopkgtest [10:53:36]: test regress: - - - - - - - - - - results - - - - - - - - - -
regress PASS
autopkgtest [10:53:36]: @@@@@@@@@@@@@@@@@@@@ summary
regress PASS

[Rafael David Tinoco (rafaeldtinoco)]

# checklist for fixes
----------------------------
 [.] changelog entry correct:
 [.] targeted to correct codename
 [.] version number is correct
 [.] update-maintainer has been run before
 ----
 [-] changes forwarded upstream/debian (if appropriate)
 [.] patches match what was proposed upstream
 ----
 [.] patches correctly included in debian/patches/series?
 [.] patches have correct DEP3 metadata
     - suggestion: when upstream header is too ugly I usually
       rip things off and rely ONLY on DEP3 (other comment)
 ----
 [.] relying on PPA only for build check ?
 [.] if relying on PPA, did it install correctly ?
 ----
 [.] building it locally ?
 [.] if building locally, was source build good ?
 [.] if building locally, was binary build good ?
 ----
 [-] was autopkgtest tested ?
 ----
 [.] is this a SRU ?
 [.] if a SRU, does the public bug have a template ?
 [-] is this a bundle of fixes ?
 [.] is this a single fix ?
 ----
 [.] if single fix, was testcase provided ?
 [-] if single fix, and testcase provided, could I reproduce it ?
 [-] if single fix, and testcase provided, did it work ?
----------------------------
 [.] = ok
 [x] = not ok
 [?] = question
 [!] = note
 [-] = n/a
----------------------------
# files touched:

$ git log -2 -p | diffstat
 changelog | 14 +++++
 patches/authkeyscommand-deadlock-01.patch | 41 ++++++++++++++++
 patches/authkeyscommand-deadlock-02.patch | 74 ++++++++++++++++++++++++++++++
 patches/series | 2
 4 files changed, 131 insertions(+)

----------------------------
# files check:

----------------------------
# comments:

next comment

[Rafael David Tinoco (rafaeldtinoco)]

All good. You're very careful and detailed with all that you do and that is a very good signal. Anyone reviewing your cases can see you clearly focus in minor details. I particularly like that style and usually read everything written in cases, even if big comments and diccussions.

Nevertheless, for the SRU template, that might not be true for the SRU reviewers. Not that they may dislike reading complete analysis.. but sometimes they lack time. I'll give an example so things are more clear:

----------------------------

"""
[Regression Potential]

Since the affected code deals with executing a subprocess, reading its output through a pipe, and then relying on wait(2) to determine whether the subprocess exited correctly; and considering that this code is written in C without the help of features like RAII and with the use of goto statements, we are not able to disconsider the chances of making a mistake and forgetting to free a resource or to properly read/write from/to pipes, for example. This is, after all, the reason the bug happened in the first place.

Having said that, openssh contains extensive tests and the code is well organized and relatively easy to follow. Upon close inspection, there doesn't seem to be an evident problem with the backported fixes.

As usual when dealing with a somewhat older distribution, there is always the possibility of encountering problems because we will be recompiling openssh using the most recent versions of its build dependencies.
"""

I usually use the bullets (from the template) and list one by one:

----

[Regression Potential]

* code forks and wait(2)s for clean exit, could lead to fd or mem leaks

* upstream code has tests, ran testcase couldn't find leaks

* ran valgrind before/after my fix, no leaks (to mitigate the potential)

----

3 bullets and SRU team knows what to expect. IF reviewer thinks is worth investigating further, he/she might look into bug details/comments.

----------------------------

Anyway, apart from my suggestions, your work is very solid.

+1 from me for this merge.

review: Approve

[Rafael David Tinoco (rafaeldtinoco)]

So, like said before, if patch header is too ugly from upstream, I usually do the full (hopefully) DEP3 format:

----

Description: original patch description

 Minor or no description (if upstream lacks it)

Author:
Origin: <upstream|backport|vendor|other>, URL
Bug:
Bug-<Vendor>:
Forwarded:
Applied-Upstream:
Reviewed-by:
Last-Update:

----

And I remove MIME and/or mail headers from format-patch etc.

BUT this is MY preference, only mandatory thing is to follow guidelines (https://dep-team.pages.debian.net/deps/dep3/) mandatory things.

I also usually do:

debian/patches/lpXXXX-description.patch
debian/patches/lpXXXX-description.patch
debian/patches/lpXXXX-description.patch

for SRUs. If they all come from the same bug I would know.

Just showing options for you to consider (or not).

Cheers o/

[Rafael David Tinoco (rafaeldtinoco)]

$ git describe
upload/1%7.2p2-4ubuntu2.10

----

$ git push pkg upload/1%7.2p2-4ubuntu2.10
Counting objects: 12, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (12/12), done.
Writing objects: 100% (12/12), 3.56 KiB | 0 bytes/s, done.
Total 12 (delta 7), reused 0 (delta 0)
To ssh://<email address hidden>/ubuntu/+source/openssh
 * [new tag] upload/1%7.2p2-4ubuntu2.10 -> upload/1%7.2p2-4ubuntu2.10

----

openssh (1:7.2p2-4ubuntu2.10) xenial; urgency=medium

  * Fix deadlock when AuthorizedKeysCommand produces a large output.
    (LP: #1877454)
    - d/p/authkeyscommand-deadlock-01.patch: Make sure to call fclose(2)
      and assign NULL to the file handler used to read
      "Authorized{Keys,Principal}Command" directives.
    - d/p/authkeyscommand-deadlock-02.patch: Consume entire output
      generated by the command pointed by
      "Authorized{Keys,Principal}Command" in order to avoid sending a
      SIGPIPE to the process.

Date: Wed, 13 May 2020 10:12:28 -0400
Changed-By: Sergio Durigan Junior <email address hidden>
Maintainer: Ubuntu Developers <email address hidden>
Signed-By: Rafael David Tinoco <email address hidden>
https://launchpad.net/ubuntu/+source/openssh/1:7.2p2-4ubuntu2.10

==

 OK: openssh_7.2p2.orig.tar.gz
 OK: openssh_7.2p2-4ubuntu2.10.debian.tar.xz
 OK: openssh_7.2p2-4ubuntu2.10.dsc
     -> Component: main Section: net

Upload Warnings:
Redirecting ubuntu xenial to ubuntu xenial-proposed.
This upload awaits approval by a distro manager

[Sergio Durigan Junior (sergiodj)]

On Thursday, May 14 2020, Rafael David Tinoco wrote:

> All good. You're very careful and detailed with all that you do and
> that is a very good signal. Anyone reviewing your cases can see you
> clearly focus in minor details. I particularly like that style and
> usually read everything written in cases, even if big comments and
> diccussions.
>
> Nevertheless, for the SRU template, that might not be true for the SRU
> reviewers. Not that they may dislike reading complete analysis.. but
> sometimes they lack time. I'll give an example so things are more
> clear:

Thank you very much for the review and the comments, Rafael.

> ----------------------------
>
> """
> [Regression Potential]
>
> Since the affected code deals with executing a subprocess, reading its
> output through a pipe, and then relying on wait(2) to determine
> whether the subprocess exited correctly; and considering that this
> code is written in C without the help of features like RAII and with
> the use of goto statements, we are not able to disconsider the chances
> of making a mistake and forgetting to free a resource or to properly
> read/write from/to pipes, for example. This is, after all, the reason
> the bug happened in the first place.
>
> Having said that, openssh contains extensive tests and the code is
> well organized and relatively easy to follow. Upon close inspection,
> there doesn't seem to be an evident problem with the backported fixes.
>
> As usual when dealing with a somewhat older distribution, there is
> always the possibility of encountering problems because we will be
> recompiling openssh using the most recent versions of its build
> dependencies.
> """
>
> I usually use the bullets (from the template) and list one by one:
>
> ----
>
> [Regression Potential]
>
> * code forks and wait(2)s for clean exit, could lead to fd or mem leaks
>
> * upstream code has tests, ran testcase couldn't find leaks
>
> * ran valgrind before/after my fix, no leaks (to mitigate the potential)
>
> ----
>
> 3 bullets and SRU team knows what to expect. IF reviewer thinks is worth investigating further, he/she might look into bug details/comments.

I know we've discussed this already, and I apologize for being too
verbose again. I will make sure to be more succint next time :-).

> ----------------------------
>
> Anyway, apart from my suggestions, your work is very solid.
>
> +1 from me for this merge.

Thanks!

--
Sergio
GPG key ID: E92F D0B3 6B14 F1F4 D8E0 EB2F 106D A1C8 C3CB BF14

diff --git a/debian/changelog b/debian/changelog
index 2432f6c..7c39cc6 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,17 @@
+openssh (1:7.2p2-4ubuntu2.10) xenial; urgency=medium
+
+  * Fix deadlock when AuthorizedKeysCommand produces a large output.
+    (LP: #1877454)
+    - d/p/authkeyscommand-deadlock-01.patch: Make sure to call fclose(2)
+      and assign NULL to the file handler used to read
+      "Authorized{Keys,Principal}Command" directives.
+    - d/p/authkeyscommand-deadlock-02.patch: Consume entire output
+      generated by the command pointed by
+      "Authorized{Keys,Principal}Command" in order to avoid sending a
+      SIGPIPE to the process.
+
+ -- Sergio Durigan Junior <[e-mail]>  Wed, 13 May 2020 10:12:28 -0400
+
 openssh (1:7.2p2-4ubuntu2.9) xenial; urgency=medium
 
   * Apply upstream patch to stop using 2020 as a future date in regress
diff --git a/debian/patches/authkeyscommand-deadlock-01.patch b/debian/patches/authkeyscommand-deadlock-01.patch
 new file mode 100644
index 0000000..b77203c
--- /dev/null
+++ b/debian/patches/authkeyscommand-deadlock-01.patch
@@ -0,0 +1,41 @@
+From: "[e-mail]" <[e-mail]>
+Date: Fri, 30 Dec 2016 22:08:02 +0000
+Subject: fix deadlock when keys/principals command produces a lot of output
+ and a key is matched early; bz#2655, patch from jboning AT gmail.com
+
+Upstream-ID: e19456429bf99087ea994432c16d00a642060afe
+
+Author: [e-mail]
+Origin: upstream, https://github.com/openssh/openssh-portable/commit/ddd3d34e5c7979ca6f4a3a98a7d219a4ed3d98c2
+Bug: https://bugzilla.mindrot.org/show_bug.cgi?id=2655
+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/openssh/+bug/1877454
+Reviewed-By: Sergio Durigan Junior <[e-mail]>
+Last-Update: 2020-05-08
+---
+ auth2-pubkey.c | 6 ++++++
+ 1 file changed, 6 insertions(+)
+
+diff --git a/auth2-pubkey.c b/auth2-pubkey.c
+index 1a3beeb..c8bbce3 100644
+--- a/auth2-pubkey.c
++++ b/auth2-pubkey.c
+@@ -704,6 +704,9 @@ match_principals_command(struct passwd *user_pw, struct sshkey_cert *cert)
+ 
+ 	ok = process_principals(f, NULL, pw, cert);
+ 
++	fclose(f);
++	f = NULL;
++
+ 	if (exited_cleanly(pid, "AuthorizedPrincipalsCommand", command) != 0)
+ 		goto out;
+ 
+@@ -1027,6 +1030,9 @@ user_key_command_allowed2(struct passwd *user_pw, Key *key)
+ 
+ 	ok = check_authkeys_file(f, options.authorized_keys_command, key, pw);
+ 
++	fclose(f);
++	f = NULL;
++
+ 	if (exited_cleanly(pid, "AuthorizedKeysCommand", command) != 0)
+ 		goto out;
+ 
diff --git a/debian/patches/authkeyscommand-deadlock-02.patch b/debian/patches/authkeyscommand-deadlock-02.patch
 new file mode 100644
index 0000000..9c06a2e
--- /dev/null
+++ b/debian/patches/authkeyscommand-deadlock-02.patch
@@ -0,0 +1,74 @@
+From: "[e-mail]" <[e-mail]>
+Date: Mon, 30 Jan 2017 01:03:00 +0000
+Subject: revise keys/principals command hang fix (bz#2655) to consume entire
+ output, avoiding sending SIGPIPE to subprocesses early; ok dtucker@
+
+Upstream-ID: 7cb04b31a61f8c78c4e48ceededcd2fd5c4ee1bc
+
+Author: Damien Miller <[e-mail]>
+Origin: upstream, https://github.com/openssh/openssh-portable/commit/52763dd3fe0a4678dafdf7aeb32286e514130afc
+Bug: https://bugzilla.mindrot.org/show_bug.cgi?id=2655
+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/openssh/+bug/1877454
+Reviewed-By: Sergio Durigan Junior <[e-mail]>
+Last-Update: 2020-05-12
+---
+ auth2-pubkey.c | 19 ++++++++++++++-----
+ 1 file changed, 14 insertions(+), 5 deletions(-)
+
+diff --git a/auth2-pubkey.c b/auth2-pubkey.c
+index c8bbce3..fb4468e 100644
+--- a/auth2-pubkey.c
++++ b/auth2-pubkey.c
+@@ -571,10 +571,13 @@ process_principals(FILE *f, char *file, struct passwd *pw,
+ {
+ 	char line[SSH_MAX_PUBKEY_BYTES], *cp, *ep, *line_opts;
+ 	u_long linenum = 0;
+-	u_int i;
++	u_int i, found_principal = 0;
+ 
+ 	auth_start_parse_options();
+ 	while (read_keyfile_line(f, file, line, sizeof(line), &linenum) != -1) {
++		/* Always consume entire input */
++		if (found_principal)
++			continue;
+ 		/* Skip leading whitespace. */
+ 		for (cp = line; *cp == ' ' || *cp == '\t'; cp++)
+ 			;
+@@ -607,11 +610,12 @@ process_principals(FILE *f, char *file, struct passwd *pw,
+ 				if (auth_parse_options(pw, line_opts,
+ 				    file, linenum) != 1)
+ 					continue;
+-				return 1;
++				found_principal = 1;
++				continue;
+ 			}
+ 		}
+ 	}
+-	return 0;
++	return found_principal;
+ }
+ 
+ static int
+@@ -744,7 +748,12 @@ check_authkeys_file(FILE *f, char *file, Key* key, struct passwd *pw)
+ 	found = NULL;
+ 	auth_start_parse_options();
+ 	while (read_keyfile_line(f, file, line, sizeof(line), &linenum) != -1) {
+-		char *cp, *key_options = NULL;
++		char *cp, *key_options = NULL, *fp = NULL;
++		const char *reason = NULL;
++
++		/* Always consume entrire file */
++		if (found_key)
++			continue;
+ 		if (found != NULL)
+ 			key_free(found);
+ 		found = key_new(key_is_cert(key) ? KEY_UNSPEC : key->type);
+@@ -833,7 +842,7 @@ check_authkeys_file(FILE *f, char *file, Key* key, struct passwd *pw)
+ 			    file, linenum, key_type(found), fp);
+ 			free(fp);
+ 			found_key = 1;
+-			break;
++			continue;
+ 		}
+ 	}
+ 	if (found != NULL)
diff --git a/debian/patches/series b/debian/patches/series
index a39737f..2b943e1 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -56,3 +56,5 @@ CVE-2019-6109-1.patch
 CVE-2019-6109-2.patch
 CVE-2019-6111-2.patch
 regress-2020.patch
+authkeyscommand-deadlock-01.patch
+authkeyscommand-deadlock-02.patch