Ubuntu
apache2 package

Merge ~ahasenack/ubuntu/+source/apache2:bionic-apache-ssl-regression-1836329 into ubuntu/+source/apache2:ubuntu/bionic-devel

  1. Git
  2. lp:~ahasenack/ubuntu/+source/apache2
  3. bionic-apache-ssl-regression-1836329
  4. Merge into ubuntu/bionic-devel
Proposed by Andreas Hasenack
Status: Merged
Approved by: Andreas Hasenack
Approved revision: 22efcf97fc5b252565bc0aa6cd12d4da93a7be64
Merge reported by: Andreas Hasenack
Merged at revision: 22efcf97fc5b252565bc0aa6cd12d4da93a7be64
Proposed branch: ~ahasenack/ubuntu/+source/apache2:bionic-apache-ssl-regression-1836329
Merge into: ubuntu/+source/apache2:ubuntu/bionic-devel
Diff against target: 216 lines (+188/-0)
4 files modified
debian/changelog (+9/-0)
debian/patches/clear-retry-flags-before-abort.patch (+67/-0)
debian/patches/series (+2/-0)
debian/patches/ssl-read-rc-value-openssl-1.1.1.patch (+110/-0)
Reviewer Review Type Date Requested Status
Bryce Harrington (community) Approve
Canonical Server packageset reviewers Pending
Review via email: mp+370217@code.launchpad.net

Description of the change

PPA: https://launchpad.net/~ahasenack/+archive/ubuntu/apache-regression-1836329/
sudo add-apt-repository ppa:ahasenack/apache-regression-1836329 -y

DEP8: http://people.ubuntu.com/~ahasenack/dep8-apache2-1836329-bionic/
(...)
autopkgtest [17:15:07]: test chroot: - - - - - - - - - - results - - - - - - - - - -
chroot PASS
autopkgtest [17:15:07]: @@@@@@@@@@@@@@@@@@@@ summary
run-test-suite PASS
duplicate-module-load PASS
htcacheclean PASS
default-mods PASS
ssl-passphrase PASS
check-http2 PASS
chroot PASS

There are two patches in this update. Once fixes the exact issue reported in the linked bug, while the other one was found during the investigation and deemed appropriate as it fixes another potential openssl-1.1.1-introduced regression (HTTP/2 SSL downloads). I couldn't reproduce the HTTP/2 errors, though. I asked other developers and they were in agreement that it should be included, but the final word will be with the SRU team.

The bug has a test case, which unfortunately needs a publicly available IP with a corresponding A DNS record so that the ssllabs website can run its tests.

A cosmic branch is still being prepared.

To post a comment you must log in.
Revision history for this message
Andreas Hasenack (ahasenack) wrote :

I just found out that the run-test-suite dep8 test isn't actually running, because the scrit runs as root and apache refuses that, and it still exits 0:
make[1]: Leaving directory '/tmp/autopkgtest.wAqieG/autopkgtest_tmp/perl-framework/c-modules/eat_post'
[ error] Apache cannot spawn child processes as root, therefore the test suite must be run as a non-privileged user.
+ rm apache2.conf.debian

From debian's 2.4.37-1 upload:
  * Fix test suite to actually run by creating a test user. It turns out
    the test suite refuses to run as root but returns true even in that
    case. It seems this has been broken since 2.4.27-4, where the test suite
    had been updated and the debci test duration dropped from 15min to
    3min. Also, don't rely on the exit status anymore but parse the test
    output.

I might give that a try tomorrow.

Revision history for this message
Andreas Hasenack (ahasenack) wrote :

Similar to my cosmic comment, in bionic this failed, for the same reason:
# Test 2 got: "68" (t/apache/mmn.t at line 42)
# Expected: "75"
t/apache/mmn.t ......................
Failed 1/2 subtests

Revision history for this message
Bryce Harrington (bryce) wrote :

0. Created lxc container 'review-bionic'
1. Configured container per bug test case:
    - apt update
    - apt install apache2
    - a2enmod ssl
    - a2ensite default-ssl.conf
    - service apache2 restart
2. echo "<html><body><p>Hello LP: #1836329!</p></body></html>" > /var/www/html/index.html
3. In my home router, configured port forwarding for 443 to point at internal host
4. On host, added 443 port map for the lxc container
    lxc config device add review-bionic 443 proxy listen=tcp:0.0.0.0:443 connect=tcp:localhost:443
5. On host, verified lxd is listening to http/https:
    $ sudo lsof -i -n | grep http | grep ^lxd
    lxd 12757 165536 4u IPv6 13665650 0t0 TCP *:https (LISTEN)
    lxd 12757 165536 6u IPv6 13665650 0t0 TCP *:https (LISTEN)
6. From remote system, verified (with lynx) that I could access the homepage from step #2
7. Loaded https://www.ssllabs.com and performed test, accepting/bypassing cert warnings
8. When test finished, verified 100% cpu spin in top:

top - 00:33:22 up 20:17, 0 users, load average: 1.66, 1.59, 1.61
Tasks: 26 total, 1 running, 25 sleeping, 0 stopped, 0 zombie
%Cpu(s): 26.5 us, 1.8 sy, 0.0 ni, 71.5 id, 0.0 wa, 0.0 hi, 0.2 si, 0.0 st
KiB Mem : 32852756 total, 32044440 free, 110200 used, 698116 buff/cache
KiB Swap: 2097148 total, 2095100 free, 2048 used. 32742556 avail Mem

  PID USER PR NI VIRT RES SHR S %CPU %MEM TIME+ COMMAND
19927 www-data 20 0 2088504 8340 6028 S 100.0 0.0 4:49.00 apache2

9. Restarted apache, and verified in top cpu load quieted down

10. Installed fixed apache2 from the linked ppa
  $ sudo add-apt-repository -y ppa:ahasenack/apache-regression-1836329
  $ apt-cache policy apache2 | grep Candidate:
  Candidate: 2.4.29-1ubuntu4.8~ppa3
  $ sudo apt-get install apache2
  (...)
  $ sudo service apache2 restart

11. Reran testsuite as before, then checked top. This time no non-idle tasks above 0.3%.

12. lxc config device remove review-bionic 443

Also:
 - Verified LP numbers and descriptions
 - Verified patch matches to what was proposed upstream, except for CHANGES/STATUS chunks as mentioned
 - Verified update-maintainer is already set
 - Verified patches are correctly listed in debian/patches/series
 - Verified DEP3 bug links in both patches
 - Verified debdiff targets have correct release codename
 - Verified installation / de-installation / purge / upgrade

review: Approve
Revision history for this message
Andreas Hasenack (ahasenack) wrote :

Thanks for the review, Bryce!

Tagged and uploaded:
$ git push pkg upload/2.4.29-1ubuntu4.8
Enumerating objects: 28, done.
Counting objects: 100% (28/28), done.
Delta compression using up to 2 threads
Compressing objects: 100% (20/20), done.
Writing objects: 100% (20/20), 4.59 KiB | 70.00 KiB/s, done.
Total 20 (delta 14), reused 0 (delta 0)
To ssh://git.launchpad.net/~usd-import-team/ubuntu/+source/apache2
 * [new tag] upload/2.4.29-1ubuntu4.8 -> upload/2.4.29-1ubuntu4.8

$ dput ubuntu ../apache2_2.4.29-1ubuntu4.8_source.changes
Checking signature on .changes
gpg: ../apache2_2.4.29-1ubuntu4.8_source.changes: Valid signature from AC983EB5BF6BCBA9
Checking signature on .dsc
gpg: ../apache2_2.4.29-1ubuntu4.8.dsc: Valid signature from AC983EB5BF6BCBA9
Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading apache2_2.4.29-1ubuntu4.8.dsc: done.
  Uploading apache2_2.4.29-1ubuntu4.8.debian.tar.xz: done.
  Uploading apache2_2.4.29-1ubuntu4.8_source.buildinfo: done.
  Uploading apache2_2.4.29-1ubuntu4.8_source.changes: done.
Successfully uploaded packages.

Revision history for this message
Andreas Hasenack (ahasenack) wrote :

This is fix released, and thus, merged.

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 522f32f..edf6aba 100644
3--- a/debian/changelog
4+++ b/debian/changelog
5@@ -1,3 +1,12 @@
6+apache2 (2.4.29-1ubuntu4.8) bionic; urgency=medium
7+
8+ * d/p/ssl-read-rc-value-openssl-1.1.1.patch: Handle SSL_read() return code 0
9+ similarly to <0 with openssl 1.1.1
10+ * d/p/clear-retry-flags-before-abort.patch: clear retry flags before
11+ aborting on client-initiated reneg (LP: #1836329)
12+
13+ -- Andreas Hasenack <andreas@canonical.com> Tue, 16 Jul 2019 15:14:45 -0300
14+
15 apache2 (2.4.29-1ubuntu4.7) bionic; urgency=medium
16
17 * d/p/disable-ssl-1.1.1-auto-retry.patch: fix client certificate
18diff --git a/debian/patches/clear-retry-flags-before-abort.patch b/debian/patches/clear-retry-flags-before-abort.patch
19new file mode 100644
20index 0000000..c1a8434
21--- /dev/null
22+++ b/debian/patches/clear-retry-flags-before-abort.patch
23@@ -0,0 +1,67 @@
24+From c0b11e0ba834665b87eaa59ff544a3552d5f46e8 Mon Sep 17 00:00:00 2001
25+From: "William A. Rowe Jr" <wrowe@apache.org>
26+Date: Wed, 16 Jan 2019 17:06:07 +0000
27+Subject: [PATCH] mod_ssl (ssl_engine_io.c: bio_filter_out_write,
28+ bio_filter_in_read) Clear retry flags before aborting on client-initiated
29+ reneg.
30+
31+PR: 63052
32+Backports: r1850946
33+Submitted by: Joe Orton
34+Reviewed by: wrowe, jorton, rpluem
35+
36+
37+git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/2.4.x@1851471 13f79535-47bb-0310-9956-ffa450edef68
38+---
39+ CHANGES | 3 +++
40+ STATUS | 6 ------
41+ modules/ssl/ssl_engine_io.c | 12 ++++--------
42+ 3 files changed, 7 insertions(+), 14 deletions(-)
43+ .
44+ Ubuntu note: removed the CHANGES and STATUS hunks from the upstrem patch
45+Bug: https://bz.apache.org/bugzilla/show_bug.cgi?id=63052
46+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/apache2/+bug/1836329
47+Origin: upstream, https://github.com/apache/httpd/commit/c0b11e0ba834665b87eaa59ff544a3552d5f46e8
48+Last-Update: 2019-07-16
49+diff --git a/modules/ssl/ssl_engine_io.c b/modules/ssl/ssl_engine_io.c
50+index b398363b1ca..6da8f10118f 100644
51+--- a/modules/ssl/ssl_engine_io.c
52++++ b/modules/ssl/ssl_engine_io.c
53+@@ -200,18 +200,14 @@ static int bio_filter_out_write(BIO *bio, const char *in, int inl)
54+ apr_bucket *e;
55+ int need_flush;
56+
57++ BIO_clear_retry_flags(bio);
58++
59+ /* Abort early if the client has initiated a renegotiation. */
60+ if (outctx->filter_ctx->config->reneg_state == RENEG_ABORT) {
61+ outctx->rc = APR_ECONNABORTED;
62+ return -1;
63+ }
64+
65+- /* when handshaking we'll have a small number of bytes.
66+- * max size SSL will pass us here is about 16k.
67+- * (16413 bytes to be exact)
68+- */
69+- BIO_clear_retry_flags(bio);
70+-
71+ /* Use a transient bucket for the output data - any downstream
72+ * filter must setaside if necessary. */
73+ e = apr_bucket_transient_create(in, inl, outctx->bb->bucket_alloc);
74+@@ -458,14 +454,14 @@ static int bio_filter_in_read(BIO *bio, char *in, int inlen)
75+ if (!in)
76+ return 0;
77+
78++ BIO_clear_retry_flags(bio);
79++
80+ /* Abort early if the client has initiated a renegotiation. */
81+ if (inctx->filter_ctx->config->reneg_state == RENEG_ABORT) {
82+ inctx->rc = APR_ECONNABORTED;
83+ return -1;
84+ }
85+
86+- BIO_clear_retry_flags(bio);
87+-
88+ if (!inctx->bb) {
89+ inctx->rc = APR_EOF;
90+ return -1;
91diff --git a/debian/patches/series b/debian/patches/series
92index 0a4cf1a..50b5da6 100644
93--- a/debian/patches/series
94+++ b/debian/patches/series
95@@ -37,3 +37,5 @@ CVE-2019-0220-1.patch
96 CVE-2019-0220-2.patch
97 CVE-2019-0220-3.patch
98 disable-ssl-1.1.1-auto-retry.patch
99+ssl-read-rc-value-openssl-1.1.1.patch
100+clear-retry-flags-before-abort.patch
101diff --git a/debian/patches/ssl-read-rc-value-openssl-1.1.1.patch b/debian/patches/ssl-read-rc-value-openssl-1.1.1.patch
102new file mode 100644
103index 0000000..5b905f9
104--- /dev/null
105+++ b/debian/patches/ssl-read-rc-value-openssl-1.1.1.patch
106@@ -0,0 +1,110 @@
107+From 644cff9977efa322fe6c0ae3357a5b8cb1eeec11 Mon Sep 17 00:00:00 2001
108+From: Daniel Ruggeri <druggeri@apache.org>
109+Date: Tue, 16 Oct 2018 20:24:23 +0000
110+Subject: [PATCH] *) mod_ssl: Handle SSL_read() return code 0 similarly to
111+ <0. It is needed when using OpenSSL 1.1.1 and should not harm
112+ for versions before 1.1.1. Without the patch for
113+ 1.1.1 a 0 byte read no longer results in EAGAIN but instead in
114+ APR_EOF which leads to HTTP/2 failures. For the changelog: Fix
115+ HTTP/2 failures when using OpenSSL 1.1.1. trunk patch:
116+ http://svn.apache.org/r1843954 2.4.x patch: svn merge -c 1843954
117+ ^/httpd/httpd/trunk . +1: rjung, druggeri, rpluem
118+
119+git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/2.4.x@1844047 13f79535-47bb-0310-9956-ffa450edef68
120+---
121+ STATUS | 12 --------
122+ modules/ssl/ssl_engine_io.c | 59 +++++++++++++++++++++----------------
123+ 2 files changed, 33 insertions(+), 38 deletions(-)
124+ .
125+ Ubuntu note: removed STATUS hunk from the upstream patch
126+Origin: upstream, https://github.com/apache/httpd/commit/644cff9977efa322fe6c0ae3357a5b8cb1eeec11
127+Last-Update: 2019-07-16
128+diff --git a/modules/ssl/ssl_engine_io.c b/modules/ssl/ssl_engine_io.c
129+index d52d5e30caa..03aa0cec994 100644
130+--- a/modules/ssl/ssl_engine_io.c
131++++ b/modules/ssl/ssl_engine_io.c
132+@@ -680,37 +680,36 @@ static apr_status_t ssl_io_input_read(bio_filter_in_ctx_t *inctx,
133+ }
134+ return inctx->rc;
135+ }
136+- else if (rc == 0) {
137+- /* If EAGAIN, we will loop given a blocking read,
138+- * otherwise consider ourselves at EOF.
139+- */
140+- if (APR_STATUS_IS_EAGAIN(inctx->rc)
141+- || APR_STATUS_IS_EINTR(inctx->rc)) {
142+- /* Already read something, return APR_SUCCESS instead.
143+- * On win32 in particular, but perhaps on other kernels,
144+- * a blocking call isn't 'always' blocking.
145++ else /* (rc <= 0) */ {
146++ int ssl_err;
147++ conn_rec *c;
148++ if (rc == 0) {
149++ /* If EAGAIN, we will loop given a blocking read,
150++ * otherwise consider ourselves at EOF.
151+ */
152+- if (*len > 0) {
153+- inctx->rc = APR_SUCCESS;
154+- break;
155+- }
156+- if (inctx->block == APR_NONBLOCK_READ) {
157+- break;
158+- }
159+- }
160+- else {
161+- if (*len > 0) {
162+- inctx->rc = APR_SUCCESS;
163++ if (APR_STATUS_IS_EAGAIN(inctx->rc)
164++ || APR_STATUS_IS_EINTR(inctx->rc)) {
165++ /* Already read something, return APR_SUCCESS instead.
166++ * On win32 in particular, but perhaps on other kernels,
167++ * a blocking call isn't 'always' blocking.
168++ */
169++ if (*len > 0) {
170++ inctx->rc = APR_SUCCESS;
171++ break;
172++ }
173++ if (inctx->block == APR_NONBLOCK_READ) {
174++ break;
175++ }
176+ }
177+ else {
178+- inctx->rc = APR_EOF;
179++ if (*len > 0) {
180++ inctx->rc = APR_SUCCESS;
181++ break;
182++ }
183+ }
184+- break;
185+ }
186+- }
187+- else /* (rc < 0) */ {
188+- int ssl_err = SSL_get_error(inctx->filter_ctx->pssl, rc);
189+- conn_rec *c = (conn_rec*)SSL_get_app_data(inctx->filter_ctx->pssl);
190++ ssl_err = SSL_get_error(inctx->filter_ctx->pssl, rc);
191++ c = (conn_rec*)SSL_get_app_data(inctx->filter_ctx->pssl);
192+
193+ if (ssl_err == SSL_ERROR_WANT_READ) {
194+ /*
195+@@ -754,6 +753,10 @@ static apr_status_t ssl_io_input_read(bio_filter_in_ctx_t *inctx,
196+ "SSL input filter read failed.");
197+ }
198+ }
199++ else if (rc == 0 && ssl_err == SSL_ERROR_ZERO_RETURN) {
200++ inctx->rc = APR_EOF;
201++ break;
202++ }
203+ else /* if (ssl_err == SSL_ERROR_SSL) */ {
204+ /*
205+ * Log SSL errors and any unexpected conditions.
206+@@ -763,6 +766,10 @@ static apr_status_t ssl_io_input_read(bio_filter_in_ctx_t *inctx,
207+ ssl_log_ssl_error(SSLLOG_MARK, APLOG_INFO, mySrvFromConn(c));
208+
209+ }
210++ if (rc == 0) {
211++ inctx->rc = APR_EOF;
212++ break;
213++ }
214+ if (inctx->rc == APR_SUCCESS) {
215+ inctx->rc = APR_EGENERAL;
216+ }

Subscribers

People subscribed via source and target branches