You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Yann Ylavic <yl...@gmail.com> on 2017/01/04 12:57:14 UTC
Could/Shouldn't check_header() allow folding?
I'm using a (third-party/closed) module which replaces newlines in
header values (like base64 encoded PEMs) with obs-fold.
That's probably obsolete, but not forbidden per se...
How about something like:
Index: modules/http/http_filters.c
===================================================================
--- modules/http/http_filters.c (revision 1776920)
+++ modules/http/http_filters.c (working copy)
@@ -701,19 +701,26 @@ static int check_header(void *arg, const char *nam
return 0;
}
- if (ctx->strict) {
- test = ap_scan_http_token(name);
- }
- else {
- test = ap_scan_vchar_obstext(name);
- }
- if (*test) {
- ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, ctx->r, APLOGNO(02429)
- "Response header name '%s' contains invalid "
- "characters, aborting request",
- name);
- return 0;
- }
+ test = name;
+ do {
+ if (ctx->strict) {
+ test = ap_scan_http_token(test);
+ }
+ else {
+ test = ap_scan_vchar_obstext(test);
+ }
+ if (*test) {
+ if (test[0] != CR || test[1] != LF || (test[2] != ' ' &&
+ test[2] != '\t')) {
+ ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, ctx->r, APLOGNO(02429)
+ "Response header name '%s' contains invalid "
+ "characters, aborting request",
+ name);
+ return 0;
+ }
+ test += 3;
+ }
+ } while (*test);
test = ap_scan_http_field_content(val);
if (*test) {
?
Re: Could/Shouldn't check_header() allow folding?
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Wed, Jan 4, 2017 at 7:21 AM, William A Rowe Jr <wr...@rowe-clan.net> wrote:
> On Wed, Jan 4, 2017 at 6:57 AM, Yann Ylavic <yl...@gmail.com> wrote:
>> I'm using a (third-party/closed) module which replaces newlines in
>> header values (like base64 encoded PEMs) with obs-fold.
>
> If we accept obs-fold from CGI, or internally within the
> headers_out, we must
> replace them with a single SP and conform to the spec on the wire.
What about either an optional filter, or simply plug in a module that
only implements a fixup hook, which can be configured on potentially
offending requests to scan and correct the headers_out members?
Such a module could also do something to work around other CTLs,
and invalid header token names, to avoid 500 error conditions and
attempt to still forward some response.
Re: Could/Shouldn't check_header() allow folding?
Posted by Yann Ylavic <yl...@gmail.com>.
On Wed, Jan 4, 2017 at 6:22 PM, William A Rowe Jr <wr...@rowe-clan.net> wrote:
> On Wed, Jan 4, 2017 at 11:12 AM, Yann Ylavic <yl...@gmail.com> wrote:
>>
>> This would work for me (on the proxy side), too.
>> The patch (attached) is a bit longer, but still reasonable IMHO.
>> WDYT?
>
> Not understanding if (!header->key) { continue; } - why success if there is
> a dead ': UnnamedValue' entry in the output headers table?
That's quite due to apr_table internals, it seems that
apr_table_entry_t->key can be set to NULL there to remove an entry.
Every apr_table_elts() iterator I've seen or worked with does this
check, and so does apr_table_[v]do BTW.
> Unclear why
> apr_table_do needed to be re-implemented.
Because we can't change the value in an apr_table_do callback.
(Note this patch won't change the value in place, but will make it
point to another buffer).
>
> Does it make sense to smash surrounding whitespace here? A single SP
> is going to have the same semantic value as multiple SP/TAB characters.
Makes sense, will do it on commit ("end += 3 + strspn(end + 3, "\t
");") if no one is against the whole change.
Re: Could/Shouldn't check_header() allow folding?
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Wed, Jan 4, 2017 at 11:12 AM, Yann Ylavic <yl...@gmail.com> wrote:
>
> This would work for me (on the proxy side), too.
> The patch (attached) is a bit longer, but still reasonable IMHO.
> WDYT?
Not understanding if (!header->key) { continue; } - why success if there is
a dead ': UnnamedValue' entry in the output headers table? Unclear why
apr_table_do needed to be re-implemented.
Does it make sense to smash surrounding whitespace here? A single SP
is going to have the same semantic value as multiple SP/TAB characters.
Re: Could/Shouldn't check_header() allow folding?
Posted by Yann Ylavic <yl...@gmail.com>.
On Wed, Jan 4, 2017 at 2:21 PM, William A Rowe Jr <wr...@rowe-clan.net> wrote:
> On Wed, Jan 4, 2017 at 6:57 AM, Yann Ylavic <yl...@gmail.com> wrote:
>> I'm using a (third-party/closed) module which replaces newlines in
>> header values (like base64 encoded PEMs) with obs-fold.
>> That's probably obsolete, but not forbidden per se...
>
> Actually, it is, c.f. 3.2.4 of RFC 7230
>
> [...] 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.
OK, pretty clear...
>
>> How about something like:
>
> -1. If we accept obs-fold from CGI, or internally within the
> headers_out, we must
> replace them with a single SP and conform to the spec on the wire.
This would work for me (on the proxy side), too.
The patch (attached) is a bit longer, but still reasonable IMHO.
WDYT?
Re: Could/Shouldn't check_header() allow folding?
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Wed, Jan 4, 2017 at 6:57 AM, Yann Ylavic <yl...@gmail.com> wrote:
> I'm using a (third-party/closed) module which replaces newlines in
> header values (like base64 encoded PEMs) with obs-fold.
> That's probably obsolete, but not forbidden per se...
Actually, it is, c.f. 3.2.4 of RFC 7230
[...] 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.
> How about something like:
>
> Index: modules/http/http_filters.c
> ===================================================================
> --- modules/http/http_filters.c (revision 1776920)
> +++ modules/http/http_filters.c (working copy)
> @@ -701,19 +701,26 @@ static int check_header(void *arg, const char *nam
> return 0;
> }
>
> - if (ctx->strict) {
> - test = ap_scan_http_token(name);
> - }
> - else {
> - test = ap_scan_vchar_obstext(name);
> - }
> - if (*test) {
> - ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, ctx->r, APLOGNO(02429)
> - "Response header name '%s' contains invalid "
> - "characters, aborting request",
> - name);
> - return 0;
> - }
> + test = name;
> + do {
> + if (ctx->strict) {
> + test = ap_scan_http_token(test);
> + }
> + else {
> + test = ap_scan_vchar_obstext(test);
> + }
> + if (*test) {
> + if (test[0] != CR || test[1] != LF || (test[2] != ' ' &&
> + test[2] != '\t')) {
> + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, ctx->r, APLOGNO(02429)
> + "Response header name '%s' contains invalid "
> + "characters, aborting request",
> + name);
> + return 0;
> + }
> + test += 3;
> + }
> + } while (*test);
>
> test = ap_scan_http_field_content(val);
> if (*test) {
> ?
-1. If we accept obs-fold from CGI, or internally within the
headers_out, we must
replace them with a single SP and conform to the spec on the wire.