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

Proposed by Sergio Durigan Junior
Status: Merged
Approved by: Rafael David Tinoco
Approved revision: ab3c0fca6361c92c646cf17ec413a4e2ce939796
Merged at revision: ab3c0fca6361c92c646cf17ec413a4e2ce939796
Proposed branch: ~sergiodj/ubuntu/+source/openssh:bug1877454-hangs-authorizedkeyscommand
Merge into: ubuntu/+source/openssh:ubuntu/xenial-devel
Diff against target: 159 lines (+131/-0)
4 files modified
debian/changelog (+14/-0)
debian/patches/authkeyscommand-deadlock-01.patch (+41/-0)
debian/patches/authkeyscommand-deadlock-02.patch (+74/-0)
debian/patches/series (+2/-0)
Reviewer Review Type Date Requested Status
Rafael David Tinoco (community) Approve
Canonical Server Pending
Canonical Server Core Reviewers Pending
Review via email: mp+383879@code.launchpad.net

Description of the change

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

To post a comment you must log in.
Revision history for this message
Rafael David Tinoco (rafaeldtinoco) wrote :

# 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

Revision history for this message
Rafael David Tinoco (rafaeldtinoco) 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:

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

"""
[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
Revision history for this message
Rafael David Tinoco (rafaeldtinoco) wrote :

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/

Revision history for this message
Rafael David Tinoco (rafaeldtinoco) wrote :

$ 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

Revision history for this message
Sergio Durigan Junior (sergiodj) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/debian/changelog b/debian/changelog
2index 2432f6c..7c39cc6 100644
3--- a/debian/changelog
4+++ b/debian/changelog
5@@ -1,3 +1,17 @@
6+openssh (1:7.2p2-4ubuntu2.10) xenial; urgency=medium
7+
8+ * Fix deadlock when AuthorizedKeysCommand produces a large output.
9+ (LP: #1877454)
10+ - d/p/authkeyscommand-deadlock-01.patch: Make sure to call fclose(2)
11+ and assign NULL to the file handler used to read
12+ "Authorized{Keys,Principal}Command" directives.
13+ - d/p/authkeyscommand-deadlock-02.patch: Consume entire output
14+ generated by the command pointed by
15+ "Authorized{Keys,Principal}Command" in order to avoid sending a
16+ SIGPIPE to the process.
17+
18+ -- Sergio Durigan Junior <sergio.durigan@canonical.com> Wed, 13 May 2020 10:12:28 -0400
19+
20 openssh (1:7.2p2-4ubuntu2.9) xenial; urgency=medium
21
22 * Apply upstream patch to stop using 2020 as a future date in regress
23diff --git a/debian/patches/authkeyscommand-deadlock-01.patch b/debian/patches/authkeyscommand-deadlock-01.patch
24new file mode 100644
25index 0000000..b77203c
26--- /dev/null
27+++ b/debian/patches/authkeyscommand-deadlock-01.patch
28@@ -0,0 +1,41 @@
29+From: "djm@openbsd.org" <djm@openbsd.org>
30+Date: Fri, 30 Dec 2016 22:08:02 +0000
31+Subject: fix deadlock when keys/principals command produces a lot of output
32+ and a key is matched early; bz#2655, patch from jboning AT gmail.com
33+
34+Upstream-ID: e19456429bf99087ea994432c16d00a642060afe
35+
36+Author: jboning@gmail.com
37+Origin: upstream, https://github.com/openssh/openssh-portable/commit/ddd3d34e5c7979ca6f4a3a98a7d219a4ed3d98c2
38+Bug: https://bugzilla.mindrot.org/show_bug.cgi?id=2655
39+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/openssh/+bug/1877454
40+Reviewed-By: Sergio Durigan Junior <sergio.durigan@canonical.com>
41+Last-Update: 2020-05-08
42+---
43+ auth2-pubkey.c | 6 ++++++
44+ 1 file changed, 6 insertions(+)
45+
46+diff --git a/auth2-pubkey.c b/auth2-pubkey.c
47+index 1a3beeb..c8bbce3 100644
48+--- a/auth2-pubkey.c
49++++ b/auth2-pubkey.c
50+@@ -704,6 +704,9 @@ match_principals_command(struct passwd *user_pw, struct sshkey_cert *cert)
51+
52+ ok = process_principals(f, NULL, pw, cert);
53+
54++ fclose(f);
55++ f = NULL;
56++
57+ if (exited_cleanly(pid, "AuthorizedPrincipalsCommand", command) != 0)
58+ goto out;
59+
60+@@ -1027,6 +1030,9 @@ user_key_command_allowed2(struct passwd *user_pw, Key *key)
61+
62+ ok = check_authkeys_file(f, options.authorized_keys_command, key, pw);
63+
64++ fclose(f);
65++ f = NULL;
66++
67+ if (exited_cleanly(pid, "AuthorizedKeysCommand", command) != 0)
68+ goto out;
69+
70diff --git a/debian/patches/authkeyscommand-deadlock-02.patch b/debian/patches/authkeyscommand-deadlock-02.patch
71new file mode 100644
72index 0000000..9c06a2e
73--- /dev/null
74+++ b/debian/patches/authkeyscommand-deadlock-02.patch
75@@ -0,0 +1,74 @@
76+From: "djm@openbsd.org" <djm@openbsd.org>
77+Date: Mon, 30 Jan 2017 01:03:00 +0000
78+Subject: revise keys/principals command hang fix (bz#2655) to consume entire
79+ output, avoiding sending SIGPIPE to subprocesses early; ok dtucker@
80+
81+Upstream-ID: 7cb04b31a61f8c78c4e48ceededcd2fd5c4ee1bc
82+
83+Author: Damien Miller <djm@openbsd.org>
84+Origin: upstream, https://github.com/openssh/openssh-portable/commit/52763dd3fe0a4678dafdf7aeb32286e514130afc
85+Bug: https://bugzilla.mindrot.org/show_bug.cgi?id=2655
86+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/openssh/+bug/1877454
87+Reviewed-By: Sergio Durigan Junior <sergio.durigan@canonical.com>
88+Last-Update: 2020-05-12
89+---
90+ auth2-pubkey.c | 19 ++++++++++++++-----
91+ 1 file changed, 14 insertions(+), 5 deletions(-)
92+
93+diff --git a/auth2-pubkey.c b/auth2-pubkey.c
94+index c8bbce3..fb4468e 100644
95+--- a/auth2-pubkey.c
96++++ b/auth2-pubkey.c
97+@@ -571,10 +571,13 @@ process_principals(FILE *f, char *file, struct passwd *pw,
98+ {
99+ char line[SSH_MAX_PUBKEY_BYTES], *cp, *ep, *line_opts;
100+ u_long linenum = 0;
101+- u_int i;
102++ u_int i, found_principal = 0;
103+
104+ auth_start_parse_options();
105+ while (read_keyfile_line(f, file, line, sizeof(line), &linenum) != -1) {
106++ /* Always consume entire input */
107++ if (found_principal)
108++ continue;
109+ /* Skip leading whitespace. */
110+ for (cp = line; *cp == ' ' || *cp == '\t'; cp++)
111+ ;
112+@@ -607,11 +610,12 @@ process_principals(FILE *f, char *file, struct passwd *pw,
113+ if (auth_parse_options(pw, line_opts,
114+ file, linenum) != 1)
115+ continue;
116+- return 1;
117++ found_principal = 1;
118++ continue;
119+ }
120+ }
121+ }
122+- return 0;
123++ return found_principal;
124+ }
125+
126+ static int
127+@@ -744,7 +748,12 @@ check_authkeys_file(FILE *f, char *file, Key* key, struct passwd *pw)
128+ found = NULL;
129+ auth_start_parse_options();
130+ while (read_keyfile_line(f, file, line, sizeof(line), &linenum) != -1) {
131+- char *cp, *key_options = NULL;
132++ char *cp, *key_options = NULL, *fp = NULL;
133++ const char *reason = NULL;
134++
135++ /* Always consume entrire file */
136++ if (found_key)
137++ continue;
138+ if (found != NULL)
139+ key_free(found);
140+ found = key_new(key_is_cert(key) ? KEY_UNSPEC : key->type);
141+@@ -833,7 +842,7 @@ check_authkeys_file(FILE *f, char *file, Key* key, struct passwd *pw)
142+ file, linenum, key_type(found), fp);
143+ free(fp);
144+ found_key = 1;
145+- break;
146++ continue;
147+ }
148+ }
149+ if (found != NULL)
150diff --git a/debian/patches/series b/debian/patches/series
151index a39737f..2b943e1 100644
152--- a/debian/patches/series
153+++ b/debian/patches/series
154@@ -56,3 +56,5 @@ CVE-2019-6109-1.patch
155 CVE-2019-6109-2.patch
156 CVE-2019-6111-2.patch
157 regress-2020.patch
158+authkeyscommand-deadlock-01.patch
159+authkeyscommand-deadlock-02.patch

Subscribers

People subscribed via source and target branches