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

[Andreas Hasenack]

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.

39e94ee... by Andreas Hasenack on 2020-04-13

changelog

[Bryce Harrington (bryce)]

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

[Andreas Hasenack (ahasenack)]

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

[Andreas Hasenack (ahasenack)]

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.

[Andreas Hasenack (ahasenack)]

This migrated.

diff --git a/debian/changelog b/debian/changelog
index 1e8587a..c16feee 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,12 @@
+apache2 (2.4.41-4ubuntu3) focal; urgency=medium
+
+  [ Timo Aaltonen ]
+  * d/p/buffer-http-request-bodies-for-tlsv13.diff, d/p/tlsv13-add-logno.diff:
+    mod_ssl: Add patches to fix TLS 1.3 client cert authentication for POST requests.
+    Closes: #955348, LP: #1872478
+
+ -- Andreas Hasenack <[e-mail]>  Mon, 13 Apr 2020 14:19:17 -0300
+
 apache2 (2.4.41-4ubuntu2) focal; urgency=medium
 
   * d/p/mod_proxy_ajp-secret-parameter*.patch: add new "secret"
diff --git a/debian/patches/buffer-http-request-bodies-for-tlsv13.diff b/debian/patches/buffer-http-request-bodies-for-tlsv13.diff
new file mode 100644
index 0000000..3f6352c
--- /dev/null
+++ b/debian/patches/buffer-http-request-bodies-for-tlsv13.diff
@@ -0,0 +1,134 @@
+From 3b6181a317bdecac0305c8eb538de4960405e3dd Mon Sep 17 00:00:00 2001
+From: Joe Orton <[e-mail]>
+Date: Thu, 21 Nov 2019 15:51:32 +0000
+Subject: [PATCH] Buffer HTTP request bodies for TLSv1.3 PHA in the same way as
+ for TLSv<1.3 renegotiation.
+
+* modules/ssl/ssl_engine_kernel.c (fill_reneg_buffer): Factor
+  out...
+  (ssl_hook_Access_classic): ... from here.
+  (ssl_hook_Access_modern): Use it here too.
+
+Github: closes #75
+
+
+git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1870095 13f79535-47bb-0310-9956-ffa450edef68
+---
+ modules/ssl/ssl_engine_kernel.c | 87 +++++++++++++++++++++------------
+ 1 file changed, 55 insertions(+), 32 deletions(-)
+
+diff --git a/modules/ssl/ssl_engine_kernel.c b/modules/ssl/ssl_engine_kernel.c
+index 758cc09bb69..516e7082bb0 100644
+--- a/modules/ssl/ssl_engine_kernel.c
++++ b/modules/ssl/ssl_engine_kernel.c
+@@ -114,6 +114,45 @@ static int has_buffered_data(request_rec *r)
+     return result;
+ }
+ 
++/* If a renegotiation is required for the location, and the request
++ * includes a message body (and the client has not requested a "100
++ * Continue" response), then the client will be streaming the request
++ * body over the wire already.  In that case, it is not possible to
++ * stop and perform a new SSL handshake immediately; once the SSL
++ * library moves to the "accept" state, it will reject the SSL packets
++ * which the client is sending for the request body.
++ *
++ * To allow authentication to complete in the hook, the solution used
++ * here is to fill a (bounded) buffer with the request body, and then
++ * to reinject that request body later.
++ *
++ * This function is called to fill the renegotiation buffer for the
++ * location as required, or fail.  Returns zero on success or HTTP_
++ * error code on failure.
++ */
++static int fill_reneg_buffer(request_rec *r, SSLDirConfigRec *dc)
++{
++    int rv;
++    apr_size_t rsize;
++
++    /* ### this is HTTP/1.1 specific, special case for protocol? */
++    if (r->expecting_100 || !ap_request_has_body(r)) {
++        return 0;
++    }
++
++    rsize = dc->nRenegBufferSize == UNSET ? DEFAULT_RENEG_BUFFER_SIZE : dc->nRenegBufferSize;
++    if (rsize > 0) {
++        /* Fill the I/O buffer with the request body if possible. */
++        rv = ssl_io_buffer_fill(r, rsize);
++    }
++    else {
++        /* If the reneg buffer size is set to zero, just fail. */
++        rv = HTTP_REQUEST_ENTITY_TOO_LARGE;
++    }
++
++    return rv;
++}
++
+ #ifdef HAVE_TLSEXT
+ static int ap_array_same_str_set(apr_array_header_t *s1, apr_array_header_t *s2)
+ {
+@@ -814,41 +853,14 @@ static int ssl_hook_Access_classic(request_rec *r, SSLSrvConfigRec *sc, SSLDirCo
+         }
+     }
+ 
+-    /* If a renegotiation is now required for this location, and the
+-     * request includes a message body (and the client has not
+-     * requested a "100 Continue" response), then the client will be
+-     * streaming the request body over the wire already.  In that
+-     * case, it is not possible to stop and perform a new SSL
+-     * handshake immediately; once the SSL library moves to the
+-     * "accept" state, it will reject the SSL packets which the client
+-     * is sending for the request body.
+-     *
+-     * To allow authentication to complete in this auth hook, the
+-     * solution used here is to fill a (bounded) buffer with the
+-     * request body, and then to reinject that request body later.
+-     */
+-    if (renegotiate && !renegotiate_quick
+-        && !r->expecting_100
+-        && ap_request_has_body(r)) {
+-        int rv;
+-        apr_size_t rsize;
+-
+-        rsize = dc->nRenegBufferSize == UNSET ? DEFAULT_RENEG_BUFFER_SIZE :
+-                                                dc->nRenegBufferSize;
+-        if (rsize > 0) {
+-            /* Fill the I/O buffer with the request body if possible. */
+-            rv = ssl_io_buffer_fill(r, rsize);
+-        }
+-        else {
+-            /* If the reneg buffer size is set to zero, just fail. */
+-            rv = HTTP_REQUEST_ENTITY_TOO_LARGE;
+-        }
+-
+-        if (rv) {
++    /* Fill reneg buffer if required. */
++    if (renegotiate && !renegotiate_quick) {
++        rc = fill_reneg_buffer(r, dc);
++        if (rc) {
+             ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02257)
+                           "could not buffer message body to allow "
+                           "SSL renegotiation to proceed");
+-            return rv;
++            return rc;
+         }
+     }
+ 
+@@ -1132,6 +1144,17 @@ static int ssl_hook_Access_modern(request_rec *r, SSLSrvConfigRec *sc, SSLDirCon
+             }
+         }
+ 
++        /* Fill reneg buffer if required. */
++        if (change_vmode) {
++            rc = fill_reneg_buffer(r, dc);
++            if (rc) {
++                ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO()
++                              "could not buffer message body to allow "
++                              "TLS Post-Handshake Authentication to proceed");
++                return rc;
++            }
++        }
++
+         if (change_vmode) {
+             char peekbuf[1];
+ 
diff --git a/debian/patches/series b/debian/patches/series
index 3278e1a..e69e713 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -13,3 +13,5 @@ spelling-errors.patch
 086_svn_cross_compiles
 mod_proxy_ajp-secret-parameter-doc.patch
 mod_proxy_ajp-secret-parameter.patch
+buffer-http-request-bodies-for-tlsv13.diff
+tlsv13-add-logno.diff
diff --git a/debian/patches/tlsv13-add-logno.diff b/debian/patches/tlsv13-add-logno.diff
new file mode 100644
index 0000000..e25c1e9
--- /dev/null
+++ b/debian/patches/tlsv13-add-logno.diff
@@ -0,0 +1,23 @@
+From 55239ec1e23ec02734c0738a57e244b2a0de1060 Mon Sep 17 00:00:00 2001
+From: Joe Orton <[e-mail]>
+Date: Thu, 21 Nov 2019 16:55:14 +0000
+Subject: [PATCH] Add logno.
+
+git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1870097 13f79535-47bb-0310-9956-ffa450edef68
+---
+ modules/ssl/ssl_engine_kernel.c   | 2 +-
+ 2 files changed, 2 insertions(+), 2 deletions(-)
+
+diff --git a/modules/ssl/ssl_engine_kernel.c b/modules/ssl/ssl_engine_kernel.c
+index 516e7082bb0..408ffd90202 100644
+--- a/modules/ssl/ssl_engine_kernel.c
++++ b/modules/ssl/ssl_engine_kernel.c
+@@ -1148,7 +1148,7 @@ static int ssl_hook_Access_modern(request_rec *r, SSLSrvConfigRec *sc, SSLDirCon
+         if (change_vmode) {
+             rc = fill_reneg_buffer(r, dc);
+             if (rc) {
+-                ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO()
++                ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10228)
+                               "could not buffer message body to allow "
+                               "TLS Post-Handshake Authentication to proceed");
+                 return rc;