Merge ~ahasenack/ubuntu/+source/apache2:focal-apache-pha-with-post-tlsv13 into ubuntu/+source/apache2:ubuntu/devel

Proposed by Andreas Hasenack
Status: Merged
Approved by: Andreas Hasenack
Approved revision: 39e94eeb52120796a4be5639df774840a2339500
Merge reported by: Andreas Hasenack
Merged at revision: 39e94eeb52120796a4be5639df774840a2339500
Proposed branch: ~ahasenack/ubuntu/+source/apache2:focal-apache-pha-with-post-tlsv13
Merge into: ubuntu/+source/apache2:ubuntu/devel
Diff against target: 196 lines (+168/-0)
4 files modified
debian/changelog (+9/-0)
debian/patches/buffer-http-request-bodies-for-tlsv13.diff (+134/-0)
debian/patches/series (+2/-0)
debian/patches/tlsv13-add-logno.diff (+23/-0)
Reviewer Review Type Date Requested Status
Bryce Harrington Approve
Canonical Server Core Reviewers Pending
Timo Aaltonen Pending
Review via email: mp+382127@code.launchpad.net

Description of the change

Fix POST with client cert auth and TLSv1.3. Details and testing steps are in the linked bug, which I prepared as if this were an SRU.

This is a straight cherry-pick from debian's apache2 package.

To post a comment you must log in.
39e94ee... by Andreas Hasenack

changelog

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

Thanks for the detailed test case, was easy to follow.

There wasn't a PPA identified in this MP or the LP bug, but I found https://launchpad.net/~ahasenack/+archive/ubuntu/apache-tlsv13-post-pha and assume it is the one to test against; or at least, after installing it I got the documented test result successes.

* Changelog:
  - [√] changelog entry correct version and targeted codename
  - [√] changelog entries correct
  - [√] update-maintainer has been run

* Actual changes:
  - [-] no upstream changes to consider
  - [-] no further upstream version to consider
  - [√] debian changes look safe

* Old Delta:
  - [-] dropped changes are ok to be dropped
  - [-] nothing else to drop
  - [√] changes forwarded upstream/debian (if appropriate)

* New Delta:
  - [-] no new patches added
  - [√] patches match what was proposed upstream
  - [√] patches correctly included in debian/patches/series
  - [√] patches have correct DEP3 metadata

* Build/Test:
  - [-] build is ok
  - [√] verified PPA package installs/uninstalls
  - [√] autopkgtest against the PPA package passes
  - [√] sanity checks test fine

I verified the patches match upstream, and did a cursory code review of them. I ran the test cases in LXC and verified I got the same output in error.log, access.log, and stdout as documented in the test case. I also ran the autopkgtests in another lxc container, and verified they pass (several SKIP due to testbed not providing revert-full-system).

I did not rebuild the apache2 package myself, and am presuming the PPA build is sufficient.

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

Thanks, that was the right PPA indeed, sorry for not mentioning it anywhere.

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

Tagging and uploading 39e94eeb52120796a4be5639df774840a2339500

$ git push pkg upload/2.4.41-4ubuntu3
Enumerating objects: 23, done.
Counting objects: 100% (23/23), done.
Delta compression using up to 4 threads
Compressing objects: 100% (14/14), done.
Writing objects: 100% (15/15), 4.06 KiB | 64.00 KiB/s, done.
Total 15 (delta 10), reused 3 (delta 1)
To ssh://git.launchpad.net/~usd-import-team/ubuntu/+source/apache2
 * [new tag] upload/2.4.41-4ubuntu3 -> upload/2.4.41-4ubuntu3

$ dput ubuntu ../apache2_2.4.41-4ubuntu3_source.changes
Checking signature on .changes
gpg: ../apache2_2.4.41-4ubuntu3_source.changes: Valid signature from AC983EB5BF6BCBA9
Checking signature on .dsc
gpg: ../apache2_2.4.41-4ubuntu3.dsc: Valid signature from AC983EB5BF6BCBA9
Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading apache2_2.4.41-4ubuntu3.dsc: done.
  Uploading apache2_2.4.41-4ubuntu3.debian.tar.xz: done.
  Uploading apache2_2.4.41-4ubuntu3_source.buildinfo: done.
  Uploading apache2_2.4.41-4ubuntu3_source.changes: done.
Successfully uploaded packages.

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

This migrated.

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 1e8587a..c16feee 100644
3--- a/debian/changelog
4+++ b/debian/changelog
5@@ -1,3 +1,12 @@
6+apache2 (2.4.41-4ubuntu3) focal; urgency=medium
7+
8+ [ Timo Aaltonen ]
9+ * d/p/buffer-http-request-bodies-for-tlsv13.diff, d/p/tlsv13-add-logno.diff:
10+ mod_ssl: Add patches to fix TLS 1.3 client cert authentication for POST requests.
11+ Closes: #955348, LP: #1872478
12+
13+ -- Andreas Hasenack <andreas@canonical.com> Mon, 13 Apr 2020 14:19:17 -0300
14+
15 apache2 (2.4.41-4ubuntu2) focal; urgency=medium
16
17 * d/p/mod_proxy_ajp-secret-parameter*.patch: add new "secret"
18diff --git a/debian/patches/buffer-http-request-bodies-for-tlsv13.diff b/debian/patches/buffer-http-request-bodies-for-tlsv13.diff
19new file mode 100644
20index 0000000..3f6352c
21--- /dev/null
22+++ b/debian/patches/buffer-http-request-bodies-for-tlsv13.diff
23@@ -0,0 +1,134 @@
24+From 3b6181a317bdecac0305c8eb538de4960405e3dd Mon Sep 17 00:00:00 2001
25+From: Joe Orton <jorton@apache.org>
26+Date: Thu, 21 Nov 2019 15:51:32 +0000
27+Subject: [PATCH] Buffer HTTP request bodies for TLSv1.3 PHA in the same way as
28+ for TLSv<1.3 renegotiation.
29+
30+* modules/ssl/ssl_engine_kernel.c (fill_reneg_buffer): Factor
31+ out...
32+ (ssl_hook_Access_classic): ... from here.
33+ (ssl_hook_Access_modern): Use it here too.
34+
35+Github: closes #75
36+
37+
38+git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1870095 13f79535-47bb-0310-9956-ffa450edef68
39+---
40+ modules/ssl/ssl_engine_kernel.c | 87 +++++++++++++++++++++------------
41+ 1 file changed, 55 insertions(+), 32 deletions(-)
42+
43+diff --git a/modules/ssl/ssl_engine_kernel.c b/modules/ssl/ssl_engine_kernel.c
44+index 758cc09bb69..516e7082bb0 100644
45+--- a/modules/ssl/ssl_engine_kernel.c
46++++ b/modules/ssl/ssl_engine_kernel.c
47+@@ -114,6 +114,45 @@ static int has_buffered_data(request_rec *r)
48+ return result;
49+ }
50+
51++/* If a renegotiation is required for the location, and the request
52++ * includes a message body (and the client has not requested a "100
53++ * Continue" response), then the client will be streaming the request
54++ * body over the wire already. In that case, it is not possible to
55++ * stop and perform a new SSL handshake immediately; once the SSL
56++ * library moves to the "accept" state, it will reject the SSL packets
57++ * which the client is sending for the request body.
58++ *
59++ * To allow authentication to complete in the hook, the solution used
60++ * here is to fill a (bounded) buffer with the request body, and then
61++ * to reinject that request body later.
62++ *
63++ * This function is called to fill the renegotiation buffer for the
64++ * location as required, or fail. Returns zero on success or HTTP_
65++ * error code on failure.
66++ */
67++static int fill_reneg_buffer(request_rec *r, SSLDirConfigRec *dc)
68++{
69++ int rv;
70++ apr_size_t rsize;
71++
72++ /* ### this is HTTP/1.1 specific, special case for protocol? */
73++ if (r->expecting_100 || !ap_request_has_body(r)) {
74++ return 0;
75++ }
76++
77++ rsize = dc->nRenegBufferSize == UNSET ? DEFAULT_RENEG_BUFFER_SIZE : dc->nRenegBufferSize;
78++ if (rsize > 0) {
79++ /* Fill the I/O buffer with the request body if possible. */
80++ rv = ssl_io_buffer_fill(r, rsize);
81++ }
82++ else {
83++ /* If the reneg buffer size is set to zero, just fail. */
84++ rv = HTTP_REQUEST_ENTITY_TOO_LARGE;
85++ }
86++
87++ return rv;
88++}
89++
90+ #ifdef HAVE_TLSEXT
91+ static int ap_array_same_str_set(apr_array_header_t *s1, apr_array_header_t *s2)
92+ {
93+@@ -814,41 +853,14 @@ static int ssl_hook_Access_classic(request_rec *r, SSLSrvConfigRec *sc, SSLDirCo
94+ }
95+ }
96+
97+- /* If a renegotiation is now required for this location, and the
98+- * request includes a message body (and the client has not
99+- * requested a "100 Continue" response), then the client will be
100+- * streaming the request body over the wire already. In that
101+- * case, it is not possible to stop and perform a new SSL
102+- * handshake immediately; once the SSL library moves to the
103+- * "accept" state, it will reject the SSL packets which the client
104+- * is sending for the request body.
105+- *
106+- * To allow authentication to complete in this auth hook, the
107+- * solution used here is to fill a (bounded) buffer with the
108+- * request body, and then to reinject that request body later.
109+- */
110+- if (renegotiate && !renegotiate_quick
111+- && !r->expecting_100
112+- && ap_request_has_body(r)) {
113+- int rv;
114+- apr_size_t rsize;
115+-
116+- rsize = dc->nRenegBufferSize == UNSET ? DEFAULT_RENEG_BUFFER_SIZE :
117+- dc->nRenegBufferSize;
118+- if (rsize > 0) {
119+- /* Fill the I/O buffer with the request body if possible. */
120+- rv = ssl_io_buffer_fill(r, rsize);
121+- }
122+- else {
123+- /* If the reneg buffer size is set to zero, just fail. */
124+- rv = HTTP_REQUEST_ENTITY_TOO_LARGE;
125+- }
126+-
127+- if (rv) {
128++ /* Fill reneg buffer if required. */
129++ if (renegotiate && !renegotiate_quick) {
130++ rc = fill_reneg_buffer(r, dc);
131++ if (rc) {
132+ ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02257)
133+ "could not buffer message body to allow "
134+ "SSL renegotiation to proceed");
135+- return rv;
136++ return rc;
137+ }
138+ }
139+
140+@@ -1132,6 +1144,17 @@ static int ssl_hook_Access_modern(request_rec *r, SSLSrvConfigRec *sc, SSLDirCon
141+ }
142+ }
143+
144++ /* Fill reneg buffer if required. */
145++ if (change_vmode) {
146++ rc = fill_reneg_buffer(r, dc);
147++ if (rc) {
148++ ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO()
149++ "could not buffer message body to allow "
150++ "TLS Post-Handshake Authentication to proceed");
151++ return rc;
152++ }
153++ }
154++
155+ if (change_vmode) {
156+ char peekbuf[1];
157+
158diff --git a/debian/patches/series b/debian/patches/series
159index 3278e1a..e69e713 100644
160--- a/debian/patches/series
161+++ b/debian/patches/series
162@@ -13,3 +13,5 @@ spelling-errors.patch
163 086_svn_cross_compiles
164 mod_proxy_ajp-secret-parameter-doc.patch
165 mod_proxy_ajp-secret-parameter.patch
166+buffer-http-request-bodies-for-tlsv13.diff
167+tlsv13-add-logno.diff
168diff --git a/debian/patches/tlsv13-add-logno.diff b/debian/patches/tlsv13-add-logno.diff
169new file mode 100644
170index 0000000..e25c1e9
171--- /dev/null
172+++ b/debian/patches/tlsv13-add-logno.diff
173@@ -0,0 +1,23 @@
174+From 55239ec1e23ec02734c0738a57e244b2a0de1060 Mon Sep 17 00:00:00 2001
175+From: Joe Orton <jorton@apache.org>
176+Date: Thu, 21 Nov 2019 16:55:14 +0000
177+Subject: [PATCH] Add logno.
178+
179+git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1870097 13f79535-47bb-0310-9956-ffa450edef68
180+---
181+ modules/ssl/ssl_engine_kernel.c | 2 +-
182+ 2 files changed, 2 insertions(+), 2 deletions(-)
183+
184+diff --git a/modules/ssl/ssl_engine_kernel.c b/modules/ssl/ssl_engine_kernel.c
185+index 516e7082bb0..408ffd90202 100644
186+--- a/modules/ssl/ssl_engine_kernel.c
187++++ b/modules/ssl/ssl_engine_kernel.c
188+@@ -1148,7 +1148,7 @@ static int ssl_hook_Access_modern(request_rec *r, SSLSrvConfigRec *sc, SSLDirCon
189+ if (change_vmode) {
190+ rc = fill_reneg_buffer(r, dc);
191+ if (rc) {
192+- ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO()
193++ ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10228)
194+ "could not buffer message body to allow "
195+ "TLS Post-Handshake Authentication to proceed");
196+ return rc;

Subscribers

People subscribed via source and target branches