Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Subject: Re: svn commit: r1799375 - /httpd/httpd/trunk/server/util.c
To: dev@httpd.apache.org
References: <20170620230820.92CF93A25B1@svn01-us-west.apache.org>
 <CACsi252iUnj4K_CBzDcLhTP2tWO4T1-9LxjesNMKFrqk6_=TEw@mail.gmail.com>
From: Jacob Champion <champion.p@gmail.com>
Message-ID: <40f8643c-e5b7-0e88-4e62-f6bb49ff5841@gmail.com>
Date: Wed, 21 Jun 2017 10:09:34 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.1.1
MIME-Version: 1.0
In-Reply-To: <CACsi252iUnj4K_CBzDcLhTP2tWO4T1-9LxjesNMKFrqk6_=TEw@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
archived-at: Wed, 21 Jun 2017 17:09:41 -0000

Reverted while the veto is in effect.

On 06/20/2017 11:08 PM, William A Rowe Jr wrote:
> Sorry but I reraise my objection and veto worthless cpu cycles.

Yep, but it's public now, which was my goal.

Background for the uninitiated: CVE-2017-7668 is a buffer overrun caused 
by Bill's patch in the Strict-HTTP backport, which inverted 
test_char_table's classification of the NULL character from "is an HTTP 
token stop" to "is NOT an HTTP token stop". A loop that relied on the 
previous behavior to stop at the end of strings was broken, causing a 
vulnerability.

My patch introduces redundant checks to ensure that even if NULL's 
classification changes in the future, all loops using test_char_table 
are provably correct without code changes, since we've already screwed 
this up once.

(Please understand that my goal here is not to point fingers at you, 
Bill, for the bug. I'm very thankful that you spent as much time as you 
did on the HTTP-strict backport. My goal is to scrutinize how we follow 
up on our mistakes after they're made, to try to make sure they don't 
happen again, and to figure out why a logically correct patch is being 
*vetoed* by you.)

> The correct fix to your concern is to document all expected behavior of 
> the null but in gen_test_char.c - and in such tests a /* !c && */ 
> notation is fine.

That's a personal judgment call, and it deserves the opinion of others 
on the list. You've used a veto to ensure that my preferred solution 
can't even reach trunk and get votes for backport. That is my major 
concern here.

If my patch goes to trunk and gets votes despite your concerns, it seems 
like that should be good enough. If it can't, and your preferred patch 
does get votes, great! At least it was all done fairly.

> Due to the way we assemble the code, I'm not convinced it that any 
> compiler can optimize away these infrequent cases.

I was under the impression that it was on you, as the one exercising the 
veto, to prove your case technically.

But that's really neither here nor there, because in the end, I'm 
comfortable trading a handful of nanoseconds for provable correctness 
and improved auditability. And I think others should be able to express 
their opinion on the matter *without* the shadow of a veto over the 
alternative that you dislike. Why would anyone else here spend time 
arguing for a patch that's already procedurally DOA?

> But there were only two questionable values for \0, and in this case the 
> answer is obvious. Invert the rule to a TOKEN char from the rather 
> dubious TOKEN_STOP definition. Solved.

Patches welcome.

--Jacob