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
