Merge ~ahasenack/ubuntu/+source/libapache2-mod-perl2:xenial-modperl2-fix-dep8-1779400 into ubuntu/+source/libapache2-mod-perl2:ubuntu/xenial-devel

Proposed by Andreas Hasenack on 2018-06-29
Status: Merged
Approved by: Robie Basak on 2018-07-12
Approved revision: afe63fa602ff94b2a8645ca156d7d44253112b2e
Merge reported by: Christian Ehrhardt 
Merged at revision: afe63fa602ff94b2a8645ca156d7d44253112b2e
Proposed branch: ~ahasenack/ubuntu/+source/libapache2-mod-perl2:xenial-modperl2-fix-dep8-1779400
Merge into: ubuntu/+source/libapache2-mod-perl2:ubuntu/xenial-devel
Diff against target: 121 lines (+93/-0)
4 files modified
debian/changelog (+11/-0)
debian/patches/370_http_syntax.patch (+34/-0)
debian/patches/380_inject_header_line_terminators.patch (+46/-0)
debian/patches/series (+2/-0)
Reviewer Review Type Date Requested Status
Christian Ehrhardt  2018-06-29 Approve on 2018-07-07
Canonical Server Team 2018-06-29 Pending
Review via email: mp+348780@code.launchpad.net

Description of the change

PPA with test packages: https://launchpad.net/~ahasenack/+archive/ubuntu/libapache2-mod-perl2-dep8/+packages (ppa:ahasenack/libapache2-mod-perl2-dep8)

This branch brings to xenial the Debian DEP8 fixes from libapache2-mod-perl2-2.0.10-2: https://git.launchpad.net/ubuntu/+source/libapache2-mod-perl2/commit/?h=debian/buster&id=1a0e6c046960f816f0089d8d164f7fa9b077719f

I slightly augmented the two patches with a reference to the Ubuntu bug, but that's it.

The two test failures we are fixing here are:
t/apache/read.t (Wstat: 0 Tests: 1 Failed: 1)
  Failed test: 1
t/filter/in_bbs_inject_header.t (Wstat: 512 Tests: 0 Failed: 0)
  Non-zero exit status: 2
  Parse errors: Bad plan. You planned 36 tests but ran 0.

Here is the xenial/amd64 test history: http://autopkgtest.ubuntu.com/packages/liba/libapache2-mod-perl2/xenial/amd64

Run this to verify the fix and run the dep8 tests locally (adjust the path to the xenial autopkgtest image):

autopkgtest -o dep8-xenial-ppa -U -s --apt-pocket=proposed --setup-commands="sudo add-apt-repository -s -y -u ppa:ahasenack/libapache2-mod-perl2-dep8" -B libapache2-mod-perl2 -- qemu /var/lib/adt-images/autopkgtest-xenial-amd64.img

Here is an output run with the package from the test ppa: http://people.ubuntu.com/~ahasenack/deb8-modperl2-xenial-ppa/

To post a comment you must log in.
Christian Ehrhardt  (paelzer) wrote :

Thanks for adding the Bug refs, from a packaging POV this LGTM.

It leaves open the convincing of the SRU Team to push an update for it, but I think it might be ok.
Worst case they will ask you to do a force-badtest hint, but then the coverage of the tests would be lost and since you have the fix ready, why not.

Two minor comments that you might include if you want (maybe only in the bug for the second).
- this is a chain of thanks, you mention Niko, and in the Debian change he mentions Stefan Fritsch.
  Not sure, but if you want to update you might want to mention the original Author instead of the
  Debian packager?
- The Bug and Comments mention this is a compat needed for 2.4.24 but Xenial has apache2/2.4.18-2ubuntu3.9 it seems - I assume that some security update might have brought the changes in, it might be worth to find what exactly and mention it in the changelog/patch-headers

review: Approve
Andreas Hasenack (ahasenack) wrote :

I think it's less confusing to drop the thanks from d/changelog. That was put in there by the dep3changelog script. The patch files have the authorship explicitly stated.

2.4.24 was unreleased, and our apache was affected by this test failure due to a change there that became a security fix we backported ("HTTP strict parsing changes"):

Changes with Apache 2.4.26

...

  *) SECURITY: CVE-2017-7668 (cve.mitre.org)
     The HTTP strict parsing changes added in 2.2.32 and 2.4.24 introduced a
     bug in token list parsing, which allows ap_find_token() to search past
     the end of its input string. By maliciously crafting a sequence of
     request headers, an attacker may be able to cause a segmentation fault,
     or to force ap_find_token() to return an incorrect value.
     [Jacob Champion]

Andreas Hasenack (ahasenack) wrote :

I believe it was this change in 2.4.18-2ubuntu3.2:

  * SECURITY UPDATE: response splitting and cache pollution issue via
    incomplete RFC7230 HTTP request grammar enforcing
    - debian/patches/CVE-2016-8743.patch: enfore stricter parsing in
      include/http_core.h, include/http_protocol.h, include/httpd.h,
      modules/http/http_filters.c, server/core.c, server/gen_test_char.c,
      server/protocol.c, server/util.c, server/vhost.c.
    - debian/patches/hostnames_with_underscores.diff: relax hostname
      restrictions in server/vhost.c.
    - CVE-2016-8743
  * WARNING: The fix for CVE-2016-8743 introduces a behavioural change and
    may introduce compatibility issues with clients that do not strictly
    follow specifications. A new configuration directive,
    "HttpProtocolOptions Unsafe" can be used to re-enable some of the less
    strict parsing restrictions, at the expense of security.

Andreas Hasenack (ahasenack) wrote :

I updated d/changelog with this information, instead of the patches. Let me know if you would rather have this info in the patches.

Christian Ehrhardt  (paelzer) wrote :

changelog is fine - any place is ok for that IMHO, thanks for adding it - I think you are good to go now.

Andreas Hasenack (ahasenack) wrote :

Getting bored during your PTO? :)

Thanks, please set the MP to approved and push the upload tag. Or just change the MP status, Robie can push the tag tomorrow. I can then dput the package.

Robie Basak (racb) wrote :

upload/2.0.9-4ubuntu1.1 pushed as afe63fa

Christian Ehrhardt  (paelzer) wrote :

Setting to merged as it was called uploaded.
It is there https://launchpadlibrarian.net/378238062/libapache2-mod-perl2_2.0.9-4ubuntu1.1_source.changes

While this needs SRU team handling we can take it out of the review queue for now.

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 1f1d278..f449fe1 100644
3--- a/debian/changelog
4+++ b/debian/changelog
5@@ -1,3 +1,14 @@
6+libapache2-mod-perl2 (2.0.9-4ubuntu1.1) xenial; urgency=medium
7+
8+ * Pull fixes from Debian's package 2.0.10-2 for the DEP8 tests that started
9+ failing after the CVE-2016-8743 fix for apache2 (LP: #1779400):
10+ - debian/patches/370_http_syntax.patch: [PATCH 1/2] Fix
11+ t/apache/read.t HTTP syntax for Apache 2.4.24.
12+ - debian/patches/380_inject_header_line_terminators.patch: [PATCH 2/2]
13+ Fix in_bbs_inject_header line terminators for Apache.
14+
15+ -- Andreas Hasenack <andreas@canonical.com> Fri, 29 Jun 2018 15:48:39 -0300
16+
17 libapache2-mod-perl2 (2.0.9-4ubuntu1) xenial; urgency=low
18
19 * Resynchronise with Debian. Remaining changes:
20diff --git a/debian/patches/370_http_syntax.patch b/debian/patches/370_http_syntax.patch
21new file mode 100644
22index 0000000..a173da3
23--- /dev/null
24+++ b/debian/patches/370_http_syntax.patch
25@@ -0,0 +1,34 @@
26+From 4a803fdb4c9eae8538293fe31c9222eecb6465be Mon Sep 17 00:00:00 2001
27+From: Niko Tyni <ntyni@debian.org>
28+Date: Fri, 23 Dec 2016 18:27:23 +0200
29+Subject: [PATCH 1/2] Fix t/apache/read.t HTTP syntax for Apache 2.4.24
30+ compatibility
31+
32+HTTP/1.1 RFC 7230, section 2.6. "Protocol Versioning" says the HTTP name
33+is case sensitive. Starting with Apache 2.4.24, using lower case will
34+make the server issue a 400 Bad request response, causing a test failure.
35+
36+https://tools.ietf.org/html/rfc7230#section-2.6
37+
38+Bug-Debian: https://bugs.debian.org/849082
39+Bug-Ubuntu: https://launchpad.net/bugs/1779400
40+---
41+ t/apache/read.t | 2 +-
42+ 1 file changed, 1 insertion(+), 1 deletion(-)
43+
44+diff --git a/t/apache/read.t b/t/apache/read.t
45+index 83670c9..9f7f504 100644
46+--- a/t/apache/read.t
47++++ b/t/apache/read.t
48+@@ -24,7 +24,7 @@ close $fh;
49+
50+ my $size = length $data;
51+
52+-for my $string ("POST $location http/1.0",
53++for my $string ("POST $location HTTP/1.0",
54+ "Content-length: $size",
55+ "") {
56+ my $line = "$string\r\n";
57+--
58+2.11.0
59+
60diff --git a/debian/patches/380_inject_header_line_terminators.patch b/debian/patches/380_inject_header_line_terminators.patch
61new file mode 100644
62index 0000000..286d7be
63--- /dev/null
64+++ b/debian/patches/380_inject_header_line_terminators.patch
65@@ -0,0 +1,46 @@
66+From d59229cf4f5b91ed58e25e27977e76f59096b72d Mon Sep 17 00:00:00 2001
67+From: Niko Tyni <ntyni@debian.org>
68+Date: Sat, 24 Dec 2016 23:07:28 +0200
69+Subject: [PATCH 2/2] Fix in_bbs_inject_header line terminators for Apache
70+ 2.4.24 compatibility
71+
72+rfc7230 3.5 says:
73+
74+ Although the line terminator for the start-line and header fields is
75+ the sequence CRLF, a recipient MAY recognize a single LF as a line
76+ terminator and ignore any preceding CR.
77+
78+Apache with strict enabled chooses not to implement the MAY.
79+
80+Author: Stefan Fritsch <sf@sfritsch.de>
81+Bug-Debian: https://bugs.debian.org/849082
82+Bug-Ubuntu: https://launchpad.net/bugs/1779400
83+---
84+ t/filter/TestFilter/in_bbs_inject_header.pm | 4 ++--
85+ 1 file changed, 2 insertions(+), 2 deletions(-)
86+
87+diff --git a/t/filter/TestFilter/in_bbs_inject_header.pm b/t/filter/TestFilter/in_bbs_inject_header.pm
88+index b09d6f9..5380c65 100644
89+--- a/t/filter/TestFilter/in_bbs_inject_header.pm
90++++ b/t/filter/TestFilter/in_bbs_inject_header.pm
91+@@ -181,7 +181,7 @@ sub handler : FilterConnectionHandler {
92+
93+ if ($data and $data =~ /^POST/) {
94+ # demonstrate how to add a header while processing other headers
95+- my $header = "$header1_key: $header1_val\n";
96++ my $header = "$header1_key: $header1_val\r\n";
97+ push @{ $ctx->{buckets} }, APR::Bucket->new($c->bucket_alloc, $header);
98+ debug "queued header [$header]";
99+ }
100+@@ -199,7 +199,7 @@ sub handler : FilterConnectionHandler {
101+ # we hit the headers and body separator, which is a good
102+ # time to add extra headers:
103+ for my $key (keys %headers) {
104+- my $header = "$key: $headers{$key}\n";
105++ my $header = "$key: $headers{$key}\r\n";
106+ push @{ $ctx->{buckets} }, APR::Bucket->new($c->bucket_alloc, $header);
107+ debug "queued header [$header]";
108+ }
109+--
110+2.11.0
111+
112diff --git a/debian/patches/series b/debian/patches/series
113index 9d123a8..ab36e55 100644
114--- a/debian/patches/series
115+++ b/debian/patches/series
116@@ -14,3 +14,5 @@ avoid-db-linkage.patch
117 0001-Fix-t-api-uri.t-t-apr-uri.t-and-t-apr-ext-uri.t-unde.patch
118 0002-Fix-t-modperl-print.t-t-modperl-printf.t-and-t-direc.patch
119 svn-r1717474-perl-5.22.patch
120+370_http_syntax.patch
121+380_inject_header_line_terminators.patch

Subscribers

People subscribed via source and target branches