Re: svn commit: r1768079 - in /httpd/httpd/branches/2.4.x: ./ CHANGES STATUS docs/manual/ modules/http/http_filters.c

[William A Rowe Jr]

On Fri, Nov 4, 2016 at 9:14 PM, William A Rowe Jr 
wrote:

> On Fri, Nov 4, 2016 at 5:17 PM, Yann Ylavic  wrote:
>
>> On Fri, Nov 4, 2016 at 10:15 PM, William A Rowe Jr 
>> wrote:
>> > I'm really not clear how ap_map_http_request_error() arrived without the
>> > patch patch below, and unclear whether it does what the backport
>> proposal
>> > intended with this work omitted.
>> >
>> > 1482522 minfrin
>> > core: Stop the HTTP_IN filter from attempting to write error buckets
>> > to the output filters, which is bogus in the proxy case. Create a
>> > clean mapping from APR codes to HTTP status codes, and use it where
>> > needed.
>>
>> The above was backported in 2.4.13 ([1]+[2]) and 2.2.30 ([3]), though
>> not exactly the same way as implemented in r1482522 (yet the intent is
>> there, and the issue fixed ;)
>>
>
> Ahhh, that's cool.
>
>
>> What differed is that http_filters' bail_out_on_error() was preserved
>> (so that [third-party-]modules don't notice/have to handle the
>> response themselves)
>
>
Which would be a brilliant solution if

 -static apr_status_t bail_out_on_error(http_ctx_t *ctx,
-  ap_filter_t *f,
-  int http_error)

Wasn't a private, unexported entry point on every sane C compiler. Sigh...

Re: svn commit: r1768079 - in /httpd/httpd/branches/2.4.x: ./ CHANGES STATUS docs/manual/ modules/http/http_filters.c

[William A Rowe Jr]

On Fri, Nov 4, 2016 at 5:17 PM, Yann Ylavic  wrote:

> On Fri, Nov 4, 2016 at 10:15 PM, William A Rowe Jr 
> wrote:
> > I'm really not clear how ap_map_http_request_error() arrived without the
> > patch patch below, and unclear whether it does what the backport proposal
> > intended with this work omitted.
> >
> > 1482522 minfrin
> > core: Stop the HTTP_IN filter from attempting to write error buckets
> > to the output filters, which is bogus in the proxy case. Create a
> > clean mapping from APR codes to HTTP status codes, and use it where
> > needed.
>
> The above was backported in 2.4.13 ([1]+[2]) and 2.2.30 ([3]), though
> not exactly the same way as implemented in r1482522 (yet the intent is
> there, and the issue fixed ;)
>

Ahhh, that's cool.


> What differed is that http_filters' bail_out_on_error() was preserved
> (so that [third-party-]modules don't notice/have to handle the
> response themselves)
>

Re: svn commit: r1768079 - in /httpd/httpd/branches/2.4.x: ./ CHANGES STATUS docs/manual/ modules/http/http_filters.c

[Yann Ylavic]

On Fri, Nov 4, 2016 at 10:15 PM, William A Rowe Jr  wrote:
> I'm really not clear how ap_map_http_request_error() arrived without the
> patch patch below, and unclear whether it does what the backport proposal
> intended with this work omitted.
>
> 1482522 minfrin
> core: Stop the HTTP_IN filter from attempting to write error buckets
> to the output filters, which is bogus in the proxy case. Create a
> clean mapping from APR codes to HTTP status codes, and use it where
> needed.

The above was backported in 2.4.13 ([1]+[2]) and 2.2.30 ([3]), though
not exactly the same way as implemented in r1482522 (yet the intent is
there, and the issue fixed ;)

What differed is that http_filters' bail_out_on_error() was preserved
(so that [third-party-]modules don't notice/have to handle the
response themselves), and "generic" proxy 502 error status was not
turned into 504 (disputed change IIRC).

Otherwise the potential double responses on error were finally handled
by [4] and follow ups (correct raising/handling of AP_FILTER_ERROR),
supposed to be compatible with both module behaviours wrt filters
error handling.

>
> I'll simply ignore this in the backport/normalization but figured it's worth
> pointing out such a discrepancy.

Not sure there is one...

Regarding the related changes in trunk only (now), we should probably
decide what to do about the 502=>504, but the modules having to
handle/raise the error looks reasonable to me, and even the correct
behaviour for 2.6/3.0.


Regards,
Yann.

[1] http://svn.apache.org/r1681114
[2] http://svn.apache.org/r1682544
[3] http://svn.apache.org/r1683808
[4] http://svn.apache.org/r1657897

Re: svn commit: r1768036 - in /httpd/httpd/branches/2.4.x-merge-http-strict: ./ CHANGES include/ap_mmn.h include/http_core.h include/httpd.h modules/http/http_filters.c server/core.c server/protocol.c

[William A Rowe Jr]

On Fri, Nov 4, 2016 at 12:46 PM, Jacob Champion 
wrote:

> [spec discussion]
>
> On 11/04/2016 09:40 AM, William A Rowe Jr wrote:
>
>> On Fri, Nov 4, 2016 at 9:47 AM, Eric Covener > > wrote:
>>
>>> There is even an example with no scheme:
>>>
>>> Location: /People.html#tim
>>>
>>
>> Not valid as a request (fragment not allowed)
>>
>
> ...but what does that have to do with the Location header? This is a valid
> and useful Location header value. (Clients won't send the fragment during
> the redirect; they'll save it to use during later presentation of the
> resource.)


Yup, this is an interesting point/question. Location header value !=
Request line URI value.

Re: svn commit: r1768036 - in /httpd/httpd/branches/2.4.x-merge-http-strict: ./ CHANGES include/ap_mmn.h include/http_core.h include/httpd.h modules/http/http_filters.c server/core.c server/protocol.c

[Jacob Champion]

[spec discussion]

On 11/04/2016 09:40 AM, William A Rowe Jr wrote:

On Fri, Nov 4, 2016 at 9:47 AM, Eric Covener > wrote:

There is even an example with no scheme:

Location: /People.html#tim


Not valid as a request (fragment not allowed)


...but what does that have to do with the Location header? This is a 
valid and useful Location header value. (Clients won't send the fragment 
during the redirect; they'll save it to use during later presentation of 
the resource.)


--Jacob

Re: ap_get_status_line(int)

[William A Rowe Jr]

Yes, you can find bugzilla entries on this topic.


On Fri, Nov 4, 2016 at 11:23 AM, Yann Ylavic  wrote:

> On Fri, Nov 4, 2016 at 2:09 PM, William A Rowe Jr 
> wrote:
> >
> > What would a non-three-digit, >599 response look like? Perhaps we have
> > a good case for a NULL response or a generic 500.
>
> AFAICT, an empty reason is allowed (the space after the status is
> required though).
> So maybe something like the attached patch?
>

Re: ap_get_status_line(int)

[Stefan Eissing]

> Am 04.11.2016 um 17:23 schrieb Yann Ylavic :
> 
> On Fri, Nov 4, 2016 at 2:09 PM, William A Rowe Jr  wrote:
>> 
>> What would a non-three-digit, >599 response look like? Perhaps we have
>> a good case for a NULL response or a generic 500.
> 
> AFAICT, an empty reason is allowed (the space after the status is
> required though).
> So maybe something like the attached patch?
> 

The patch looks fine to me. I do just feel a tad bit uncomfortable with the 
empty description. How about 
+return apr_psprintf(p, "%i No Description", status);

There are probably tons of client implementations out there that will fail when 
nothing comes after the space.

-Stefan

Re: svn commit: r1768036 - in /httpd/httpd/branches/2.4.x-merge-http-strict: ./ CHANGES include/ap_mmn.h include/http_core.h include/httpd.h modules/http/http_filters.c server/core.c server/protocol.c

[William A Rowe Jr]

On Fri, Nov 4, 2016 at 11:40 AM, William A Rowe Jr 
wrote:

>
> Give me about 24 hours to complete all this work, end of day today
> is my most optimistic timetable. Then we can discuss the resulting
> delta as a single unit/backport vote.  Because of a huge number of
> intervening commits to the relevant source files, mixing modern code
> changes based on nearly-4-year-old work in trunk is something of
> a nightmare (as we observed just syncing 2.2 to the state of the
> fixes already in 2.4.) I'll be starting a 2.2.current -> 2.4.current branch
> next, so the combination of that plus this trunk branch should get us
> to a parsing/strict observation shortly.
>

(If you do spot something amiss, you can quickly check against /trunk/
to see if the defect has been addressed already, then it will likely be
caught in my exhaustive backport list.)

Re: svn commit: r1768036 - in /httpd/httpd/branches/2.4.x-merge-http-strict: ./ CHANGES include/ap_mmn.h include/http_core.h include/httpd.h modules/http/http_filters.c server/core.c server/protocol.c

[William A Rowe Jr]

On Fri, Nov 4, 2016 at 9:47 AM, Eric Covener  wrote:

> On Fri, Nov 4, 2016 at 10:20 AM,   wrote:
> >  * that the Location response header (if present) has a valid scheme and
> is
> >absolute
>
> Too strict?
>
> https://tools.ietf.org/html/rfc7231#section-7.1.2
>
>The "Location" header field is used in some responses to refer to a
>specific resource in relation to the response.  The type of
>relationship is defined by the combination of request method and
>status code semantics.
>
>  Location = URI-reference
>
>The field value consists of a single URI-reference.  When it has the
>form of a relative reference ([RFC3986], Section 4.2), the final
>value is computed by resolving it against the effective request URI
>([RFC3986], Section 5).
>
> There is even an example with no scheme:
>
>  Location: /People.html#tim
>

Not valid as a request (fragment not allowed)

Beyond this, you may want to pend questions since there is a *very*
long list of backports to get this fork in sync with httpd-2.x trunk.
Much of the initial code has been re-thought and will take me the
better part of the day to sequentially merge. For small groups of
simple patches without much issue, I'm collapsing them (with credits
and multiple rev no's) - especially where the 'next patch' undoes some
of the changes in the prior patch and momentary flaw can be ignored.

I still have some lingering questions, but this will get you the full
picture
of where the change came from when reviewing the overall delta between
2.4.now and 2.4.next.

Give me about 24 hours to complete all this work, end of day today
is my most optimistic timetable. Then we can discuss the resulting
delta as a single unit/backport vote.  Because of a huge number of
intervening commits to the relevant source files, mixing modern code
changes based on nearly-4-year-old work in trunk is something of
a nightmare (as we observed just syncing 2.2 to the state of the
fixes already in 2.4.) I'll be starting a 2.2.current -> 2.4.current branch
next, so the combination of that plus this trunk branch should get us
to a parsing/strict observation shortly.

Re: ap_get_status_line(int)

[Yann Ylavic]

On Fri, Nov 4, 2016 at 2:09 PM, William A Rowe Jr  wrote:
>
> What would a non-three-digit, >599 response look like? Perhaps we have
> a good case for a NULL response or a generic 500.

AFAICT, an empty reason is allowed (the space after the status is
required though).
So maybe something like the attached patch?
Index: modules/http/http_protocol.c
===
--- modules/http/http_protocol.c	(revision 1766905)
+++ modules/http/http_protocol.c	(working copy)
@@ -801,15 +801,18 @@ AP_DECLARE(const char *) ap_method_name_of(apr_poo
  * from status_lines[shortcut[i]] to status_lines[shortcut[i+1]-1];
  * or use NULL to fill the gaps.
  */
-AP_DECLARE(int) ap_index_of_response(int status)
+static int index_of_response(int status)
 {
-static int shortcut[6] = {0, LEVEL_200, LEVEL_300, LEVEL_400,
-LEVEL_500, RESPONSE_CODES};
+static int shortcut[6] = {0, LEVEL_200, LEVEL_300, LEVEL_400, LEVEL_500,
+ RESPONSE_CODES};
 int i, pos;
 
-if (status < 100) {   /* Below 100 is illegal for HTTP status */
-return LEVEL_500;
+if (status < 100) { /* Below 100 is illegal for HTTP status */
+return -1;
 }
+if (status > 999) { /* Above 999 is also illegal for HTTP status */
+return -1;
+}
 
 for (i = 0; i < 5; i++) {
 status -= 100;
@@ -819,13 +822,33 @@ AP_DECLARE(const char *) ap_method_name_of(apr_poo
 return pos;
 }
 else {
-return LEVEL_500;/* status unknown (falls in gap) */
+break;
 }
 }
 }
-return LEVEL_500; /* 600 or above is also illegal */
+return 0;   /* Status unknown (falls in gap) or above 600 */
 }
 
+AP_DECLARE(int) ap_index_of_response(int status)
+{
+int index = index_of_response(status);
+return (index <= 0) ? LEVEL_500 : index;
+}
+
+AP_DECLARE(const char *) ap_get_status_line_ex(apr_pool_t *p, int status)
+{
+int index = index_of_response(status);
+if (index < 0) {
+return status_lines[LEVEL_500];
+}
+else if (index == 0) {
+return apr_psprintf(p, "%i ", status);
+}
+else {
+return status_lines[index];
+}
+}
+
 AP_DECLARE(const char *) ap_get_status_line(int status)
 {
 return status_lines[ap_index_of_response(status)];

Re: svn commit: r1768036 - in /httpd/httpd/branches/2.4.x-merge-http-strict: ./ CHANGES include/ap_mmn.h include/http_core.h include/httpd.h modules/http/http_filters.c server/core.c server/protocol.c

[Nick Kew]

On Fri, 2016-11-04 at 10:47 -0400, Eric Covener wrote:

> Too strict?

Be conservative in what you send.  An Absolute URL
is never going to be the wrong thing to send.

> https://tools.ietf.org/html/rfc7231#section-7.1.2

Another change from the HTTP RFCs we learned, where
Location MUST be absolute.

Though in practice, there was a lot of confusion,
with the CGI spec - and hence serverside apps -
permitting the relative semantics.  Seems recent
HTTP came into line with CGI on this one.

-- 
Nick Kew

Multiple SessionCryptoPassphrase keys lead to segfault when decrypting session

[Ewald Dieterich]

mod_session_crypto supports multiple SessionCryptoPassphrase keys. The 
idea is that when decrypting a session you try one key after the other 
until the decryption succeeds, assuming that the successful key is the 
key that was used when the session was encrypted.


But this assumption obviously doesn't hold. I have a case where a wrong 
(old) key successfully decrypts a session that was encrypted with a 
different (new) key. This leads to a segfault in mod_session.c, 
session_identity_decode() because the tokenization assumes valid data 
when in this case it's just binary rubbish (one of the apr_strtok() 
calls segfaults).


I "fixed" this by adding an additional sanity check to 
mod_session_crypto.c, decrypt_string(), see the attached patch. Of 
course this fix only works for cases where the seemingly successfully 
decrypted binary rubbish contains 0x00 somewhere in the decrypted data.


Any ideas for a proper fix?

Sorry that I can't provide you with the actual session data since it 
contains sensitive information (username and password).
--- mod_session_crypto.c.orig	2016-11-04 15:34:46.740015054 +0100
+++ mod_session_crypto.c	2016-11-04 15:36:02.407403627 +0100
@@ -321,6 +321,12 @@
 decryptedlen += tlen;
 decrypted[decryptedlen] = 0;
 
+if (strlen(decrypted) != decryptedlen) {
+ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
+  "decryption sanity check failed");
+continue;
+}
+
 break;
 }
 

Re: svn commit: r1768036 - in /httpd/httpd/branches/2.4.x-merge-http-strict: ./ CHANGES include/ap_mmn.h include/http_core.h include/httpd.h modules/http/http_filters.c server/core.c server/protocol.c

[Eric Covener]

On Fri, Nov 4, 2016 at 10:20 AM,   wrote:
>  * that the Location response header (if present) has a valid scheme and is
>absolute

Too strict?

https://tools.ietf.org/html/rfc7231#section-7.1.2

   The "Location" header field is used in some responses to refer to a
   specific resource in relation to the response.  The type of
   relationship is defined by the combination of request method and
   status code semantics.

 Location = URI-reference

   The field value consists of a single URI-reference.  When it has the
   form of a relative reference ([RFC3986], Section 4.2), the final
   value is computed by resolving it against the effective request URI
   ([RFC3986], Section 5).


There is even an example with no scheme:


 Location: /People.html#tim

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

Re: ap_get_status_line(int)

[William A Rowe Jr]

On Thu, Nov 3, 2016 at 12:51 PM, Stefan Eissing <
stefan.eiss...@greenbytes.de> wrote:

> Like to get your opinion:
>
> ap_get_status_line(int) converts any status code it does not know into a
> 500. This is fine for the use cases we had so far, although it can be
> discussed if it is a good idea, too strict or whatnot. But I do not want to
> go down that rathole...
>
> My question is how httpd should handle unknown codes coming from an
> upstream server. Currently:
> - mod_proxy_http sends it on (tested for intermediate responses)
> - mod_proxy_http2 sends a 500 server error
>
> The reason for this is that mod_proxy_http2 generates a status line using
> ap_get_status_line() while mod_proxy_http can parse that line from the
> upstream response.
>
> So, we have different handling of status code acceptance in proxies
> responses for HTTP/1.x and HTTP/2. This should not be. Question is, do we
> need another function which returns NULL for such cases, so the caller can
> decide to give up or work around it?
>

We really shouldn't be losing the metadata. A 500 error means something
very specific, and although it can represent unrecognized 5xx codes,
it isn't applicable to unrecognized, newer 1xx-4xx codes.

What would a non-three-digit, >599 response look like? Perhaps we have
a good case for a NULL response or a generic 500.

The actually callers to this function are fairly limited;

./server/core.c:r->status_line =
ap_get_status_line(r->status);
./modules/http2/h2_proxy_session.c:r->status_line =
ap_get_status_line(r->status);
./modules/dav/main/mod_dav.c:r->status_line =
ap_get_status_line(status);
./modules/dav/main/mod_dav.c:
 ap_get_status_line(response->status),
./modules/arch/win32/mod_isapi.c:cid->r->status_line =
ap_get_status_line(cid->r->status);
./modules/arch/win32/mod_isapi.c:cid->r->status_line =
ap_get_status_line(cid->r->status);
./modules/arch/win32/mod_isapi.c:cid->r->status_line =
ap_get_status_line(cid->r->status);
./modules/arch/win32/mod_isapi.c:cid->r->status_line =
ap_get_status_line(cid->r->status);
./modules/arch/win32/mod_isapi.c:cid->r->status_line =
ap_get_status_line(cid->r->status);
./modules/http/http_filters.c:
 ap_get_status_line(HTTP_CONTINUE), CRLF CRLF, NULL);
./modules/http/http_filters.c:r->status_line =
ap_get_status_line(r->status);
./modules/http/http_filters.c:const char *tmp =
ap_get_status_line(r->status);

Replacing these with a more robust function that doesn't throw away
newer response codes would be a good thing, but note that they expect
to have some valid result, a NULL result will probably be surprising (and
this goes for 3rd party modules as well.)