Re: svn commit: r1799375 - /httpd/httpd/trunk/server/util.c
On Mon, Jun 26, 2017 at 3:14 PM, Jacob Champion wrote: > On 06/20/2017 11:08 PM, William A Rowe Jr wrote: >> >> Sorry but I reraise my objection and veto worthless cpu cycles. > > For posterity, can I get a succinct description of your technical > justification for this veto? I have several. * Wasting CPU cycles for a no-op *which the compiler cannot recover from is unacceptable. httpd is coded in C for a reason, with all the associated side effects. Any deoptimization as some matter of style or as a matter of security (e.g. stack -> alloc) is discussed on list and the costs weighed by the group. * The test_char.h table is a private-use entity with 256 well-defined octets (not utf-8, not char-wise, these are raw bytes). Sometimes, and often for some platform, one is wrong, it is simply fixed, and the sole consumer, util.c, is corrected. * The test_char.h table is private-use and is not exported, not installed to httpd target include/, etc. util.c was its sole consumer. * mod_log_forensic changed this and started consuming an include which was buried under server/ for its own devices. Questionable, but still a core module (changes to the core table less likely to be caught when reviewing the module, but the toggle is clearly labeled FORENSIC, so there's that. Note the module duplicates the entire table, but it's forensic/diagnostic so I don't care what such code and const static footprints look like.) * T_HTTP_TOKEN_STOP declared NUL did not stop a token. That is an idiotic assertion. I inverted it. Cursory examination of the code, I did not spend enough time studying side-effects of the end-of-string behaviors, nor did the many other reviewers, I looked at the immediate affected logic. All use of this array and it's value of element NUL are exercised in util.c, this is not like it was some obscure external citation. * I agree with you the entire thing is unclear because there is no entity called a TOKEN_STOP, although there are recognized separators, and non-token garbage entities, but the functions implicated never bothered to distinguish between any of this. T_HTTP_TOKEN is clearer and less ambiguous (and NUL does not belong in that set.) I'm about to propose that change and will VOTE DOWN any call to backport that change to 2.4, as... * This change is mutually agreed to be a no-op, and you've made clear the intent was to backport to a stable, maintenance branch which should see NO deltas which have no effect. For example, whitespace correction is absolutely prohibited on a release branch because the svn blame ... comprehension of the changes to the code is destroyed by a stupid endeavor to make things look nice. When whitespace changes are introduced, it is in order to port the minimal, identical changes previously applied to trunk, so that a functional patch can be applied cleanly from an svn merge. So to convince me my veto is unwarranted, you need to basically convince me that the array elts are too volatile to trust, including the elt[0], or that we have a long tradition of getting this wrong (I suggest we don't - this was an exception), that we are unable to maintain the pairing util.c <> test_char.h. OR that we are about to export test_char.h to a much less attentive audience as a public export, etc.
Re: svn commit: r1799375 - /httpd/httpd/trunk/server/util.c
On 06/20/2017 11:08 PM, William A Rowe Jr wrote: Sorry but I reraise my objection and veto worthless cpu cycles. Hi Bill, For posterity, can I get a succinct description of your technical justification for this veto? --Jacob
Re: svn commit: r1799375 - /httpd/httpd/trunk/server/util.c
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
Re: svn commit: r1799375 - /httpd/httpd/trunk/server/util.c
On Wed, Jun 21, 2017 at 1:08 AM, William A Rowe Jr wrote:
>
> 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.
... for trunk, IMO. I don't suggest we touch this code again on
2.4 branch. For trunk, when I encounter the concept of a STOP
I envision there is a set of defined delimiters (e.g. punctuation/ctrls)
which serve as terminators. What HTTP defined was the set of
token characters in the bnf. T_HTTP_NOT_TOKEN would have
been a better name for the existing rule. T_HTTP_TOKEN is
simply a better understood rule.
On to whether element [NUL] tests aught to be trustable or
unused; My research (unsure if this was shared earlier) starts
with tests of isfn('\0');
isalnum false
isalpha false
iscntrl true
isdigit false
isgraph false
islower false
isprint false
ispunct false
isspace false
isupper false
isxdigit false
isascii true
isblank false
In each case above, the isfn('\0') results are plainly obvious, right
down to iscntrl(NUL)'s and isascii(NUL)'s result. Our resulting
test_char table values include element [NUL] as well, and it needs
to be sensible for the many purposes...
Our declarations;
T_ESCAPE_SHELL_CMD false (correct, \0 is EOS, not a char)
T_ESCAPE_PATH_SEGMENT false (correct, \0 is EOS, not a char)
T_OS_ESCAPE_PATH false (correct, \0 is EOS, not a char)
T_HTTP_TOKEN_STOP true (correct, \0 is a non-token byte)
T_ESCAPE_LOGITEM false (correct, \0 is EOS not a char)
T_ESCAPE_FORENSIC true (interesting/debatable)
T_ESCAPE_URLENCODEDfalse (correct, \0 is EOS not a char)
T_HTTP_CTRLS true (correct, \0 is NUL, non VCHAR)
T_VCHAR_OBSTEXTfalse (correct, \0 is NUL < 0x80)
In the _ESCAPE's we are escaping a string, and NUL's terminate
a string and are not escaped. Any NUL byte traveling the wire
needs to be evaluated by the appropriate TOKEN, CTRLS or
OBSTEXT rule by HTTP RFC.
The T_ESCAPE_FORENSIC case inverts the general rule for
command lines and paths we are constructing, but as a symbol
over the wire, I can agree with the idea of logging an escaped \0
in the case of a NUL input byte. This differs from logging in that
our logging API has no counted-string semantic (if it did, I guess
we would encrypt \0's.)
Re: svn commit: r1799375 - /httpd/httpd/trunk/server/util.c
Sorry but I reraise my objection and veto worthless cpu cycles.
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.
Due to the way we assemble the code, I'm not convinced it that any compiler
can optimize away these infrequent cases.
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.
On Jun 20, 2017 18:08, wrote:
> Author: jchampion
> Date: Tue Jun 20 23:08:19 2017
> New Revision: 1799375
>
> URL: http://svn.apache.org/viewvc?rev=1799375&view=rev
> Log:
> util.c: ensure all TEST_CHAR loops stop at the null terminator
>
> In the aftermath of CVE-2017-7668, decouple the business logic ("is NULL
> a T_HTTP_CTRL") from the postcondition ("must not go past the end of the
> string"). The NULL-byte classification in the TEST_CHAR table may change
> in the future.
>
> Modified:
> httpd/httpd/trunk/server/util.c
>
> Modified: httpd/httpd/trunk/server/util.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/util.
> c?rev=1799375&r1=1799374&r2=1799375&view=diff
>
> ==
> --- httpd/httpd/trunk/server/util.c (original)
> +++ httpd/httpd/trunk/server/util.c Tue Jun 20 23:08:19 2017
> @@ -1526,7 +1526,7 @@ AP_DECLARE(const char *) ap_parse_token_
> while (!string_end) {
> const unsigned char c = (unsigned char)*cur;
>
> -if (!TEST_CHAR(c, T_HTTP_TOKEN_STOP)) {
> +if (c && !TEST_CHAR(c, T_HTTP_TOKEN_STOP)) {
> /* Non-separator character; we are finished with leading
> * whitespace. We must never have encountered any trailing
> * whitespace before the delimiter (comma) */
> @@ -1600,7 +1600,7 @@ AP_DECLARE(const char *) ap_parse_token_
> */
> AP_DECLARE(const char *) ap_scan_http_field_content(const char *ptr)
> {
> -for ( ; !TEST_CHAR(*ptr, T_HTTP_CTRLS); ++ptr) ;
> +for ( ; *ptr && !TEST_CHAR(*ptr, T_HTTP_CTRLS); ++ptr) ;
>
> return ptr;
> }
> @@ -1610,7 +1610,7 @@ AP_DECLARE(const char *) ap_scan_http_fi
> */
> AP_DECLARE(const char *) ap_scan_http_token(const char *ptr)
> {
> -for ( ; !TEST_CHAR(*ptr, T_HTTP_TOKEN_STOP); ++ptr) ;
> +for ( ; *ptr && !TEST_CHAR(*ptr, T_HTTP_TOKEN_STOP); ++ptr) ;
>
> return ptr;
> }
> @@ -1620,7 +1620,7 @@ AP_DECLARE(const char *) ap_scan_http_to
> */
> AP_DECLARE(const char *) ap_scan_vchar_obstext(const char *ptr)
> {
> -for ( ; TEST_CHAR(*ptr, T_VCHAR_OBSTEXT); ++ptr) ;
> +for ( ; *ptr && TEST_CHAR(*ptr, T_VCHAR_OBSTEXT); ++ptr) ;
>
> return ptr;
> }
>
>
>
