Re: svn commit: r1777460 - /httpd/httpd/trunk/modules/http/http_filters.c

[Yann Ylavic]

On Fri, Jan 6, 2017 at 6:44 PM, Eric Covener  wrote:
> On Fri, Jan 6, 2017 at 12:06 PM, Jacob Champion  wrote:
>>
>> IOW, the message/http payload body is allowed to be line-length limited, I
>> assume because it's a message/* media type. But that doesn't apply to the
>> HTTP-level headers.
>
> I think you're right.

After re-reading I agree too.
Reverted that part in r1777672, and the associated test in r1777675.

Thanks for the review!

Re: svn commit: r1777460 - /httpd/httpd/trunk/modules/http/http_filters.c

[William A Rowe Jr]

On Fri, Jan 6, 2017 at 11:44 AM, Eric Covener  wrote:
> On Fri, Jan 6, 2017 at 12:06 PM, Jacob Champion  wrote:
>>> Modified:
>>> httpd/httpd/trunk/modules/http/http_filters.c
>>>
>>> Modified: httpd/httpd/trunk/modules/http/http_filters.c
>>> URL:
>>> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http/http_filters.c?rev=1777460=1777459=1777460=diff
>>>
>>> ==
>>> [...]
>>> @@ -683,8 +726,10 @@ static APR_INLINE int check_headers(requ
>>>
>>>  ctx.r = r;
>>>  ctx.strict = (conf->http_conformance != AP_HTTP_CONFORMANCE_UNSAFE);
>>> -return apr_table_do(check_header, , r->headers_out, NULL) &&
>>> -   apr_table_do(check_header, , r->err_headers_out, NULL);
>>> +ctx.unfold = (!r->content_type || strncmp(r->content_type,
>>> +  "message/http", 12));
>>
>>
>> I don't think this unfolding exception should exist, at least not in this
>> part of the code. My reading of 7230 is not that folded headers are allowed
>> when the Content-Type is message/http, but rather that folded headers are
>> allowed *inside* the message/http payload body:
>>
>>This specification deprecates such
>>line folding except within the message/http media type
>>(Section 8.3.1). A sender MUST NOT generate a message that includes
>>line folding (i.e., that has any field-value that contains a match to
>>the obs-fold rule) unless the message is intended for packaging
>>within the message/http media type.
>>
>> IOW, the message/http payload body is allowed to be line-length limited, I
>> assume because it's a message/* media type. But that doesn't apply to the
>> HTTP-level headers.
>
> I think you're right.

This is correct. As a deprecation, it is an absolute prohibition against sending
obs-fold as HTTP protocol bytes. It is -not- a prohibition against receiving an
obs-fold from an earlier generation HTTP server. So this change ensure that
a legacy app server generating obs-folds will not be passed on to a server.

If you wanted to generate message/http media, you would filter the content
on line length to fold it appropriately. The origin server or app's preferences
are irrelevant.

I believe Yann's approach is correct.

Re: svn commit: r1777460 - /httpd/httpd/trunk/modules/http/http_filters.c

[Eric Covener]

On Fri, Jan 6, 2017 at 12:06 PM, Jacob Champion  wrote:
>> Modified:
>> httpd/httpd/trunk/modules/http/http_filters.c
>>
>> Modified: httpd/httpd/trunk/modules/http/http_filters.c
>> URL:
>> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http/http_filters.c?rev=1777460=1777459=1777460=diff
>>
>> ==
>> [...]
>> @@ -683,8 +726,10 @@ static APR_INLINE int check_headers(requ
>>
>>  ctx.r = r;
>>  ctx.strict = (conf->http_conformance != AP_HTTP_CONFORMANCE_UNSAFE);
>> -return apr_table_do(check_header, , r->headers_out, NULL) &&
>> -   apr_table_do(check_header, , r->err_headers_out, NULL);
>> +ctx.unfold = (!r->content_type || strncmp(r->content_type,
>> +  "message/http", 12));
>
>
> I don't think this unfolding exception should exist, at least not in this
> part of the code. My reading of 7230 is not that folded headers are allowed
> when the Content-Type is message/http, but rather that folded headers are
> allowed *inside* the message/http payload body:
>
>This specification deprecates such
>line folding except within the message/http media type
>(Section 8.3.1). A sender MUST NOT generate a message that includes
>line folding (i.e., that has any field-value that contains a match to
>the obs-fold rule) unless the message is intended for packaging
>within the message/http media type.
>
> IOW, the message/http payload body is allowed to be line-length limited, I
> assume because it's a message/* media type. But that doesn't apply to the
> HTTP-level headers.

I think you're right.

-- 
Eric Covener
cove...@gmail.com

Re: svn commit: r1777460 - /httpd/httpd/trunk/modules/http/http_filters.c

[Jacob Champion]

On 01/05/2017 04:31 AM, yla...@apache.org wrote:

Author: ylavic
Date: Thu Jan  5 12:31:48 2017
New Revision: 1777460

URL: http://svn.apache.org/viewvc?rev=1777460=rev
Log:
http: allow folding in check_headers(), still compliant with RFC 7230 (3.2.4).

Modified:
httpd/httpd/trunk/modules/http/http_filters.c

Modified: httpd/httpd/trunk/modules/http/http_filters.c
URL: 
http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http/http_filters.c?rev=1777460=1777459=1777460=diff
==
[...]
@@ -683,8 +726,10 @@ static APR_INLINE int check_headers(requ

 ctx.r = r;
 ctx.strict = (conf->http_conformance != AP_HTTP_CONFORMANCE_UNSAFE);
-return apr_table_do(check_header, , r->headers_out, NULL) &&
-   apr_table_do(check_header, , r->err_headers_out, NULL);
+ctx.unfold = (!r->content_type || strncmp(r->content_type,
+  "message/http", 12));


I don't think this unfolding exception should exist, at least not in 
this part of the code. My reading of 7230 is not that folded headers are 
allowed when the Content-Type is message/http, but rather that folded 
headers are allowed *inside* the message/http payload body:


   This specification deprecates such
   line folding except within the message/http media type
   (Section 8.3.1). A sender MUST NOT generate a message that includes
   line folding (i.e., that has any field-value that contains a match to
   the obs-fold rule) unless the message is intended for packaging
   within the message/http media type.

IOW, the message/http payload body is allowed to be line-length limited, 
I assume because it's a message/* media type. But that doesn't apply to 
the HTTP-level headers.


--Jacob

Re: svn commit: r1777460 - /httpd/httpd/trunk/modules/http/http_filters.c

[William A Rowe Jr]

On Thu, Jan 5, 2017 at 5:14 PM, Yann Ylavic  wrote:
> On Thu, Jan 5, 2017 at 11:49 PM, Yann Ylavic  wrote:
>>
>> But if any of you fears a possible regression for older 2.2.x apps (I
>> see now that Eric included a test, I personnaly tested it this
>> afternoon with a custom integration suite too), I'm happy to propose
>> and vote it.
>
> FWIW, proposed in STATUS for both to 2.2 and 2.4.

Excellent.

Since all other changes had lived on trunk and been accepted by several
more committers towards 2.4 backport, I'm more comfortable working in
the 2.4-stable changes onto 2.2 at this moment than adding new work,
but that's just my 2c. I'll vet and approve the patch after we wrap up 2.2.32
but if others jump on board to review it, I'm not about to object.

Re: svn commit: r1777460 - /httpd/httpd/trunk/modules/http/http_filters.c

[Yann Ylavic]

On Thu, Jan 5, 2017 at 11:42 PM, Yann Ylavic  wrote:
> On Thu, Jan 5, 2017 at 11:08 PM, William A Rowe Jr  
> wrote:
>> On Thu, Jan 5, 2017 at 4:05 PM, Eric Covener  wrote:
>>> Do we want this for the 2.2 release?
>>
>> I don't feel strongly about this.
>>
>> It is such an unusual edge case (I believe Yann pointed out it was a custom
>> module he was working around) that it should rarely be seen in the wild.
>>
>> I'd be fine if we want to accelerate this into 2.2.32, or get 2.2.32
>> out and then
>> backport to have the patch available for the very few who are impacted.
>
> Agreed, I wouldn't push it, and can live with a my own patching.
> Possibly another module/cgi uses that CRLF to LWS trick though, but
> not that I'm aware of so..

But if any of you fears a possible regression for older 2.2.x apps (I
see now that Eric included a test, I personnaly tested it this
afternoon with a custom integration suite too), I'm happy to propose
and vote it.

Re: svn commit: r1777460 - /httpd/httpd/trunk/modules/http/http_filters.c

[Yann Ylavic]

On Thu, Jan 5, 2017 at 11:08 PM, William A Rowe Jr  wrote:
> On Thu, Jan 5, 2017 at 4:05 PM, Eric Covener  wrote:
>> Do we want this for the 2.2 release?
>
> I don't feel strongly about this.
>
> It is such an unusual edge case (I believe Yann pointed out it was a custom
> module he was working around) that it should rarely be seen in the wild.
>
> I'd be fine if we want to accelerate this into 2.2.32, or get 2.2.32
> out and then
> backport to have the patch available for the very few who are impacted.

Agreed, I wouldn't push it, and can live with a my own patching.
Possibly another module/cgi uses that CRLF to LWS trick though, but
not that I'm aware of so..

Re: svn commit: r1777460 - /httpd/httpd/trunk/modules/http/http_filters.c

[William A Rowe Jr]

On Thu, Jan 5, 2017 at 4:05 PM, Eric Covener  wrote:
> Do we want this for the 2.2 release?

I don't feel strongly about this.

It is such an unusual edge case (I believe Yann pointed out it was a custom
module he was working around) that it should rarely be seen in the wild.

I'd be fine if we want to accelerate this into 2.2.32, or get 2.2.32
out and then
backport to have the patch available for the very few who are impacted.

Re: svn commit: r1777460 - /httpd/httpd/trunk/modules/http/http_filters.c

[Eric Covener]

Do we want this for the 2.2 release?

On Thu, Jan 5, 2017 at 7:31 AM,   wrote:
> Author: ylavic
> Date: Thu Jan  5 12:31:48 2017
> New Revision: 1777460
>
> URL: http://svn.apache.org/viewvc?rev=1777460=rev
> Log:
> http: allow folding in check_headers(), still compliant with RFC 7230 (3.2.4).
>
> Modified:
> httpd/httpd/trunk/modules/http/http_filters.c
>
> Modified: httpd/httpd/trunk/modules/http/http_filters.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http/http_filters.c?rev=1777460=1777459=1777460=diff
> ==
> --- httpd/httpd/trunk/modules/http/http_filters.c (original)
> +++ httpd/httpd/trunk/modules/http/http_filters.c Thu Jan  5 12:31:48 2017
> @@ -631,14 +631,16 @@ apr_status_t ap_http_filter(ap_filter_t
>
>  struct check_header_ctx {
>  request_rec *r;
> -int strict;
> +unsigned int strict:1,
> + unfold:1;
>  };
>
>  /* check a single header, to be used with apr_table_do() */
> -static int check_header(void *arg, const char *name, const char *val)
> +static int check_header(struct check_header_ctx *ctx,
> +const char *name, const char **val)
>  {
> -struct check_header_ctx *ctx = arg;
> -const char *test;
> +const char *pos, *end;
> +char *dst = NULL;
>
>  if (name[0] == '\0') {
>  ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, ctx->r, APLOGNO(02428)
> @@ -647,12 +649,12 @@ static int check_header(void *arg, const
>  }
>
>  if (ctx->strict) {
> -test = ap_scan_http_token(name);
> +end = ap_scan_http_token(name);
>  }
>  else {
> -test = ap_scan_vchar_obstext(name);
> +end = ap_scan_vchar_obstext(name);
>  }
> -if (*test) {
> +if (*end) {
>  ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, ctx->r, APLOGNO(02429)
>"Response header name '%s' contains invalid "
>"characters, aborting request",
> @@ -660,13 +662,54 @@ static int check_header(void *arg, const
>  return 0;
>  }
>
> -test = ap_scan_http_field_content(val);
> -if (*test) {
> -ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, ctx->r, APLOGNO(02430)
> -  "Response header '%s' value of '%s' contains invalid "
> -  "characters, aborting request",
> -  name, val);
> -return 0;
> +for (pos = *val; *pos; pos = end) {
> +end = ap_scan_http_field_content(pos);
> +if (*end) {
> +if (end[0] != CR || end[1] != LF || (end[2] != ' ' &&
> + end[2] != '\t')) {
> +ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, ctx->r, 
> APLOGNO(02430)
> +  "Response header '%s' value of '%s' contains "
> +  "invalid characters, aborting request",
> +  name, pos);
> +return 0;
> +}
> +if (!ctx->unfold) {
> +end += 3;
> +}
> +else if (!dst) {
> +*val = dst = apr_palloc(ctx->r->pool, strlen(*val) + 1);
> +}
> +}
> +if (dst) {
> +memcpy(dst, pos, end - pos);
> +dst += end - pos;
> +if (*end) {
> +/* skip folding and replace with a single space */
> +end += 3 + strspn(end + 3, "\t ");
> +*dst++ = ' ';
> +}
> +}
> +}
> +if (dst) {
> +*dst = '\0';
> +}
> +return 1;
> +}
> +
> +static int check_headers_table(apr_table_t *t, struct check_header_ctx *ctx)
> +{
> +const apr_array_header_t *headers = apr_table_elts(t);
> +apr_table_entry_t *header;
> +int i;
> +
> +for (i = 0; i < headers->nelts; ++i) {
> +header = _ARRAY_IDX(headers, i, apr_table_entry_t);
> +if (!header->key) {
> +continue;
> +}
> +if (!check_header(ctx, header->key, (const char **)>val)) {
> +return 0;
> +}
>  }
>  return 1;
>  }
> @@ -683,8 +726,10 @@ static APR_INLINE int check_headers(requ
>
>  ctx.r = r;
>  ctx.strict = (conf->http_conformance != AP_HTTP_CONFORMANCE_UNSAFE);
> -return apr_table_do(check_header, , r->headers_out, NULL) &&
> -   apr_table_do(check_header, , r->err_headers_out, NULL);
> +ctx.unfold = (!r->content_type || strncmp(r->content_type,
> +  "message/http", 12));
> +return check_headers_table(r->headers_out, ) &&
> +   check_headers_table(r->err_headers_out, );
>  }
>
>  static int check_headers_recursion(request_rec *r)
>
>



-- 
Eric Covener
cove...@gmail.com