Bug#858373: help needed to complete regression fix for apache2 Bug#858373

[Antoine Beaupré]

And then, obviously, I forget the patch.

Sorry for the noise.

-- 
The secret of life is to have no fear; it's the only way to function.
- Stokely Carmichael
diff -Nru apache2-2.2.22/debian/changelog apache2-2.2.22/debian/changelog
--- apache2-2.2.22/debian/changelog	2017-07-17 03:50:16.0 -0400
+++ apache2-2.2.22/debian/changelog	2017-07-19 14:12:44.0 -0400
@@ -1,3 +1,12 @@
+apache2 (2.2.22-13+deb7u11) UNRELEASED; urgency=high
+
+  * Non-maintainer upload by the LTS Security Team.
+  * fix regression introduced in 2.2.22-13+deb7u8 that re-introduced
+something like CVE-2015-0253 when fixing CVE-2016-8743 (Closes:
+#858373)
+
+ -- Antoine Beaupré   Wed, 19 Jul 2017 14:12:44 -0400
+
 apache2 (2.2.22-13+deb7u10) wheezy-security; urgency=high
 
   * CVE-2017-9788: The value placeholder in [Proxy-]Authorization headers of
diff -Nru apache2-2.2.22/debian/patches/CVE-2016-8743-regression.patch apache2-2.2.22/debian/patches/CVE-2016-8743-regression.patch
--- apache2-2.2.22/debian/patches/CVE-2016-8743-regression.patch	1969-12-31 19:00:00.0 -0500
+++ apache2-2.2.22/debian/patches/CVE-2016-8743-regression.patch	2017-07-19 14:12:44.0 -0400
@@ -0,0 +1,23 @@
+Description: fix regression introduced in CVE-2016-8743
+ The messy CVE-2016-8743 patchset introduced an error in protocol
+ initialization in some error cases. This makes sure that invalid
+ requests doesn't segfault apache.
+ .
+ This is similar, but not directly related to CVE-2015-0253.
+Origin: https://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/server/protocol.c?r1=1642403=1668879=1668879=patch
+Bug-Debian: 858373
+Forwarded: not-needed
+Author: Antoine Beaupré
+Last-update: 2017-07-19
+
+--- a/server/protocol.c
 b/server/protocol.c
+@@ -637,6 +637,8 @@ static int read_request_line(request_rec
+ else if (APR_STATUS_IS_EINVAL(rv)) {
+ r->status = HTTP_BAD_REQUEST;
+ }
++r->proto_num = HTTP_VERSION(1,0);
++r->protocol  = apr_pstrdup(r->pool, "HTTP/1.0");
+ return 0;
+ }
+ } while ((len <= 0) && (++num_blank_lines < max_blank_lines));
diff -Nru apache2-2.2.22/debian/patches/series apache2-2.2.22/debian/patches/series
--- apache2-2.2.22/debian/patches/series	2017-07-17 03:50:33.0 -0400
+++ apache2-2.2.22/debian/patches/series	2017-07-19 14:12:44.0 -0400
@@ -61,3 +61,4 @@
 CVE-2017-7668.patch
 CVE-2017-7669.patch
 CVE-2017-9788.patch
+CVE-2016-8743-regression.patch

help needed to complete regression fix for apache2 Bug#858373

[Antoine Beaupre]

Hi,

(Sorry for the large CC list, but I am hoping to get a broad approval of
the next changes for this in order to avoid previous mistakes. ;) In
particular, I'd be very grateful for some input by Stefan considering
his knowledge of the Apache codebase and how ... exotic this problems
is.)

As I mentioned in the #858373 bug report, I started looking at fixing
the regression introduced by the 2.2.22-13+deb7u8 upload, part of
DLA-841-1. The problem occurs when a CGI(d) ErrorDocument is configured
to handle 400 error messages that can be triggered with a simple "GET /
HTTP/1.0\n\n". Such a request segfaults Apache in Wheezy right now.

I have been able to confirm that there is an unitialized variable that
gets carried around. This issue was introduced as part of
CVE-2016-8743-aux.patch in the original upload, although I fail to
remember now why this hunk is there exactly. It seems to be related to a
patch I somewhat blindly and incorrectly merged (see
87r33tqvqs@curie.anarc.at for details).

Unfortunately, re-introducing the protocol initialization code isn't
sufficient: it does fix the segfaults, but the ErrorDocument handling is
not quite working yet. Instead of seeing the output of the
ErrorDocument, after 10 seconds, I get the raw 400 message, doubled with
a 500 error document warning:

$ echo -ne "GET /foo HTTP/1.0\n\n" | nc localhost 80
HTTP/1.1 400 Bad Request
Date: Wed, 19 Jul 2017 19:11:13 GMT
Server: Apache/2.2.22 (Debian)
Vary: Accept-Encoding
Content-Length: 433
Connection: close
Content-Type: text/html; charset=iso-8859-1



400 Bad Request

Bad Request
Your browser sent a request that this server could not understand.

Additionally, a 500 Internal Server Error
error was encountered while trying to use an ErrorDocument to handle the 
request.

Apache/2.2.22 (Debian) Server at wheezy.raw Port 80


In the error log, I see:

[Wed Jul 19 19:11:23 2017] [error] [client 127.0.0.1] (70007)The timeout
specified has expired: Error reading request entity data

The first part of the error is mod_reqtimeout kicking in as the request
parser stalls on the CGI script. The second part is mod_cgi(d) failing
to read the request from the CGI script, obviously.

My theory is that there is *still* something wrong with the request
parser, even after fixing the r->protocol initialization flaw. I base
this theory on the fact that a 404 ErrorDocument works without problem.

$ echo -ne "GET /foo HTTP/1.0\r\n\r\n" | nc localhost 80
HTTP/1.1 404 Not Found
Date: Wed, 19 Jul 2017 19:13:44 GMT
Server: Apache/2.2.22 (Debian)
Vary: Accept-Encoding
Connection: close
Content-Type: text/html

Hello, World.

Note that I have also tried to see if sending "\r\n" instead of just
"\n" in my "hello world" example would work around the issue: it
doesn't, unfortunately.

I am at a loss as where to go from here, to be honest. The patch
(attached) at least fixes the segfault, which resolves the primary issue
at hand here (DoS by crashing processes!) but it would be nice to
actually fix the ErrorDocument as well..

Any ideas?

Thanks in advance,

A.


signature.asc
Description: PGP signature

Bug#858373: apache2: segfaults upon recieving bad request when using worker/event mpm and cgid errordoc

[Antoine Beaupre]

Hi!

First, thank you very much for the detailed bug report, very useful!
Responses inline.

On Tue, Mar 21, 2017 at 11:56:40AM -0500, Brian Kroth wrote:
> Package: apache2.2-common
> Version: 2.2.22-13+deb7u8
> Severity: normal
> Tags: security
> 
> Dear Maintainer,
> 
> We have some websites running on Debian Wheezy, so still using Apache
> 2.2.22, that are configured either in Worker or Event MPM (so are using
> mod_cgid in what follows), and have a custom "ErrorDocument 400" directive
> that points at a perl script for providing custom ModSecurity error.

Interesting. It would have been useful to see actual configuration
examples, as I had to go through a little bit of digging to reproduce
the issue. Just using cgid or worker is not sufficient - you actually
*do* need a ErrorDocument directive as well. So thanks to providing
those details, but snippets would have helped as well! :)
 
> I haven't dug up an older version of the package from snapshots to confirm
> this, but I think that since the recently backported HttpProtocolOptions
> directive to that version (BTW, where was that announced - I had to run
> strings on the binary to find it), I've been seeing a lot of
> segfault/coredumps registered in the Apache error logs.

I'm sorry to hear that. The change was announced in DLA-841-1 back in
February:

https://lists.debian.org/debian-lts-announce/2017/02/msg00031.html

If you haven't seen that announcement, you may not be subscribed to the
debian-lts-announce mailing list, in which case I strongly suggest you
subscribe:

https://lists.debian.org/debian-lts-announce/

There was also a lengthy discussion regarding the patchset on the
debian-lts mailing list:

https://lists.debian.org/87fukh7hcq@curie.anarc.at

Participation to the debian-lts mailing list is not mandatory for LTS
users, however...

> After some analysis, I've found that I can reproduce the error with a fairly
> trivial shell command:
> 
> # echo -ne "GET / HTTP/1.0\n" | nc $some_website 80

This definitely looks like a heading parsing regression caused by
DLA-841-1. I cannot reproduce with a vanilla Apache install.

To reproduce, on wheezy:

 1. apt-get install apache2
 2. create a dummy perl script in /usr/lib/cgi-bin/hello.pl

#!/usr/bin/perl
print "Content-type: text/html\n\n";
print "Hello, World.";

 3. make it executable
 4. add this directive, say in /etc/apache2/conf.d/hello

ErrorDocument 400 /cgi-bin/hello.pl

 5. reload apache (apache2ctl graceful or whatever)
 6. issue the killer request:

echo -ne "GET / HTTP/1.0\n" | nc localhost 80

> > From the coredump, I was able to find that this line (1371) in the
> cgid_handler() code in the modules/generators/mod_cgid.c source file has a
> null pointer issue on the r->protocol field:
> 
>   is_included = !strcmp(r->protocol, "INCLUDED");
> 
> Seems like a bit of a security issue to me.

Definifely.

[...]

On Tue, Apr 11, 2017 at 12:08:11PM +0930, Doran Moppert wrote:
> This looks like a form of CVE-2015-0253, which affected upstream apache
> 2.4.11, was introduced by the backport.  The fix is to ensure
> r->protocol is always populated:
> 
> https://svn.apache.org/viewvc?view=revision=1668879

Good findings. The fun part is that the above patch doesn't apply
because the protocol initialization was just missing. So this is
possibly broader than just 400 errors.

Note that I have tested this in jessie, and it doesn't seem affected, so
the backport for CVE-2016-8743 was done correctly there.

I'll prepare a package to fix this shortly.

A.

-- 
Cyberspace. A consensual hallucination experienced daily by billions
of legitimate operators, in every nation, by children being taught
mathematical concepts...
   - William Gibson


signature.asc
Description: PGP signature

Re: Wheezy update of apache2?

[Antoine Beaupré]

On 2017-07-18 20:53:35, Stefan Fritsch wrote:
> On Monday, 17 July 2017 16:57:00 CEST Roberto C. Sánchez wrote:
>> I did the deb7u9 update of apache2 and I was not aware of the regression
>> either.  I wonder if it makes sense for bugs above a certain severity
>> affecting versions of a package which are security uploads to show up in
>> the security tracker.  Or would there be some other sensible way, aside
>> from having to go to the BTS directly?
>
> Sorry that I haven't forwarded that to you in a timely manner. I think I have 
> mentioned it before the previous upload, but it may have gotten lost 
> somewhere.
>
> I don't know how a reasonable automatic notification could look like. 
> Probably 
> it has to be up to the maintainer to forward such bug reports.

I would agree as well - we can't possibly watch all of the BTS for such
reports. :)

Honestly, I was surprised there wasn't more pushback on DLA-841-1: it
was a major change with significant impact. The patch was a mess to
backport, and basically rewrote the request parser in Apache (!). It was
bound to introduce more issues.

I'll try to tackle this one, naturally, since I'm the one who issued the
DLA in the end!

sorry about the trouble.

a.

-- 
A genius is someone who discovers that the stone that falls and the
moon that doesn't fall represent one and the same phenomenon.
 - Ernesto Sabato

Re: Wheezy update of apache2?

[Jonas Meurer]

Hi there,

Am 17.07.2017 um 22:50 schrieb Chris Lamb:
> Hi Stefan,
> 
>> Note that a previous DLSA introduced a regression. It would be nice if 
>> you could take a look at that, too:
>>
>> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=858373
> 
> Unfortunately I uploaded this morning before I saw your note about
> this regression.
> 
> I've added anarcat and mejo to CC as they are mentioned in the
> apache2 2.2.22-13+deb7u8 upload; could one of you take care of it?

Unfortunately I'm on holidays with bad internet connectivity until
August 5th. Will not find time to look into the regression earlier.
Also, I just did further debugging and a final fix to the deb7u8 upload.
I remember that backporting the CVE-2016-8743 fix to 2.2.22 very
intrusive and complex.

Kind regards,
 jonas






signature.asc
Description: OpenPGP digital signature

Processed: found 868861 in 2.4.10-10

[Debian Bug Tracking System]

Processing commands for cont...@bugs.debian.org:

> found 868861 2.4.10-10
Bug #868861 [apache2] apache2: Package upgrade does not play well with multiple 
instances and restarting them
Marked as found in versions apache2/2.4.10-10.
> thanks
Stopping processing here.

Please contact me if you need assistance.
-- 
868861: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=868861
Debian Bug Tracking System
Contact ow...@bugs.debian.org with problems

Bug#868861: apache2: Package upgrade does not play well with multiple instances and restarting them

[Matt Hoskins]

Package: apache2
Version: 2.4.10-10+deb8u10
Severity: normal

Dear Maintainer,

I use multiple apache2 instances facility on several of my servers using the
debian example script to set them up - i.e. they're set up using
/usr/share/doc/apache2/examples/setup-instance

When doing security updates via apt-get upgrade then the postinst
script doesn't restart anything other than the default instance.
This obviously isn't much of a surprise (although it would be nice
if there was a way of registering them for restart on upgrade).
However it strikes me that this behaviour might be something worth
noting in the README.multiple-instances file though as it means the
admin will need to notice when apache2 is upgraded and manually restart
the other instances.

That's not the main reason for this report though - the main reason is
that after the postinst script has restarted the main instance if you
then try manually restart the other instances via any mechanism which
uses /etc/init.d/apache2 and if the /usr/sbin/apache2 binary has been
updated by the upgrade then you get:

[FAIL] Restarting web server: apache2 failed!
[] There are processes named 'apache2' running which do not match your pid 
file which are left untouched in the name of safety,|Please review the 
situation by hand. |
The reason for this appears to be down to the behaviour of pidof which
the /etc/init.d/apache2 script uses to perform this safety check.

It appears that debian's pidof will first try find process matches by 
device/inode
pair (i.e. where the process executable's device/inode pair matches the 
device/inode
pair of the binary, if it's located on a local filesystem). Only if it finds no
matches that way does it then try to find matches based on full path name 
instead.

Straight after upgrading apache the binary /usr/sbin/apache2
has a different device/inode pair to that of all the running /usr/sbin/apache
processes (as they were run from the prior binary version which is now deleted).

So when the postinst script for apache triggers a restart of the main instance
then pidof in the safety check will list pids of the all instances (because it 
finds none
matching the exact device/incode pair, so falls back on finding ones matching
the path). Thus that restart will pass the check and proceed ok.

After that restart the main apache2 instance is running from the new binary
and so its device/inode pair matches /usr/sbin/apache. Running pidof
/usr/sbin/apache2 will now thus only list the pids of the main apache instance
processes because it finds at least one matching the device/inode pair in
a running process and so it doesn't list those processes from the other 
instances
(because they're still running off the old now-deleted binary with a different
device/inode pair). Hence when you try use the init script for the instance to
stop/restart apache you now get the error about not matching the pidfile.
E.g. these will error while the system is in that state:

service apache2-instancename restart
/etc/init.d/apache2-instancename restart

The apache2ctl command has no such check on the contents of the pidfile I 
believe
and so will still work, so e.g. this will work:
/usr/local/sbin/apache2ctl-instancename stop
/usr/local/sbin/apache2ctl-instancename start

And also you can manually kill the old process using the pid in the pidfile and 
then
use service start or the init.d script for the instance.

Once the other instances running off the previous binary have been restarted 
using either
of these interventions then pidof will start listing them again.

I've no idea how you might resolve the issue though other than maybe weakening
the pidof check in the init script to just check for "apache2" instead
of the full path of "/usr/sbin/apache2" or alternatively just document
this issue in the README so at least people using multiple-instances know that 
upgrading the
apache2 package need to watch out for this (and perhaps just stop all
instances prior to doing the package upgrade so they don't have any
running off the prior binary).

I've had a quick look at the apache2 source in experimental and it seems to have
the same check in the apache2 init script, so this report should be valid 
against
the latest version.

Regards,
Matt