Re: svn commit: r1908827 - in /httpd/httpd/trunk/modules: http2/mod_proxy_http2.c proxy/mod_proxy_ajp.c proxy/mod_proxy_balancer.c proxy/mod_proxy_fcgi.c proxy/mod_proxy_http.c proxy/mod_proxy_uwsgi.c
On Fri, Mar 31, 2023 at 11:23 AM Ruediger Pluem wrote: > > On 3/31/23 10:25 AM, Yann Ylavic wrote: > > On Fri, Mar 31, 2023 at 10:15 AM Ruediger Pluem wrote: > >> > >> On 3/31/23 9:56 AM, Yann Ylavic wrote: > >>> On Fri, Mar 31, 2023 at 8:28 AM Ruediger Pluem wrote: > > On 3/31/23 2:11 AM, yla...@apache.org wrote: > > Author: ylavic > > Date: Fri Mar 31 00:11:02 2023 > > New Revision: 1908827 > > > > URL: http://svn.apache.org/viewvc?rev=1908827=rev > > Log: > > mod_proxy: Check for space/ctrls in nocanon path/urls before forwarding. > > Can this really happen? Wouldn't this path be rejected during request > line parsing? > >>> > >>> This is on the suspenders plus belt side (check what we send), but I > >>> suppose that an unfortunate NE[,P] rule could cause it. > >> > >> Don't get me wrong I like better safe than sorry, but can we have nocanon > >> with RewriteRules? > > > > NE,P forces nocanon in mod_rewrite for instance. > > Good catch. I missed this. I was tempted to not do it for "noencode" (i.e. mapping=encoded) because it is the (most-)fast path for proxying, likely w/o RewriteRules or the users really knowing what they are doing here, but finally went with common code. I could be convinced though :) > > > > >> And if we think that this is a possibility shouldn't we check > >> > >> *ap_scan_vchar_obstext(r->filename) > >> > >> just before returning, to be really sure we missed no edge case? > > > > We could do that, but the path and search parts of the constructed > > r->filename seem to be the only ones we haven't "parsed" in > > canon_handler/ap_proxy_canon_netloc already. > > Ok, fair enough. Do you care to propose for backport such that we get it in > before the tag? Done (STATUS + backport PR #354). Thanks; Yann.
Re: svn commit: r1908827 - in /httpd/httpd/trunk/modules: http2/mod_proxy_http2.c proxy/mod_proxy_ajp.c proxy/mod_proxy_balancer.c proxy/mod_proxy_fcgi.c proxy/mod_proxy_http.c proxy/mod_proxy_uwsgi.c
On 3/31/23 10:25 AM, Yann Ylavic wrote: > On Fri, Mar 31, 2023 at 10:15 AM Ruediger Pluem wrote: >> >> On 3/31/23 9:56 AM, Yann Ylavic wrote: >>> On Fri, Mar 31, 2023 at 8:28 AM Ruediger Pluem wrote: On 3/31/23 2:11 AM, yla...@apache.org wrote: > Author: ylavic > Date: Fri Mar 31 00:11:02 2023 > New Revision: 1908827 > > URL: http://svn.apache.org/viewvc?rev=1908827=rev > Log: > mod_proxy: Check for space/ctrls in nocanon path/urls before forwarding. Can this really happen? Wouldn't this path be rejected during request line parsing? >>> >>> This is on the suspenders plus belt side (check what we send), but I >>> suppose that an unfortunate NE[,P] rule could cause it. >> >> Don't get me wrong I like better safe than sorry, but can we have nocanon >> with RewriteRules? > > NE,P forces nocanon in mod_rewrite for instance. Good catch. I missed this. > >> And if we think that this is a possibility shouldn't we check >> >> *ap_scan_vchar_obstext(r->filename) >> >> just before returning, to be really sure we missed no edge case? > > We could do that, but the path and search parts of the constructed > r->filename seem to be the only ones we haven't "parsed" in > canon_handler/ap_proxy_canon_netloc already. Ok, fair enough. Do you care to propose for backport such that we get it in before the tag? Regards Rüdiger
Re: svn commit: r1908827 - in /httpd/httpd/trunk/modules: http2/mod_proxy_http2.c proxy/mod_proxy_ajp.c proxy/mod_proxy_balancer.c proxy/mod_proxy_fcgi.c proxy/mod_proxy_http.c proxy/mod_proxy_uwsgi.c
On Fri, Mar 31, 2023 at 10:15 AM Ruediger Pluem wrote: > > On 3/31/23 9:56 AM, Yann Ylavic wrote: > > On Fri, Mar 31, 2023 at 8:28 AM Ruediger Pluem wrote: > >> > >> On 3/31/23 2:11 AM, yla...@apache.org wrote: > >>> Author: ylavic > >>> Date: Fri Mar 31 00:11:02 2023 > >>> New Revision: 1908827 > >>> > >>> URL: http://svn.apache.org/viewvc?rev=1908827=rev > >>> Log: > >>> mod_proxy: Check for space/ctrls in nocanon path/urls before forwarding. > >> > >> Can this really happen? Wouldn't this path be rejected during request line > >> parsing? > > > > This is on the suspenders plus belt side (check what we send), but I > > suppose that an unfortunate NE[,P] rule could cause it. > > Don't get me wrong I like better safe than sorry, but can we have nocanon > with RewriteRules? NE,P forces nocanon in mod_rewrite for instance. > And if we think that this is a possibility shouldn't we check > > *ap_scan_vchar_obstext(r->filename) > > just before returning, to be really sure we missed no edge case? We could do that, but the path and search parts of the constructed r->filename seem to be the only ones we haven't "parsed" in canon_handler/ap_proxy_canon_netloc already. Regards; Yann.
Re: svn commit: r1908827 - in /httpd/httpd/trunk/modules: http2/mod_proxy_http2.c proxy/mod_proxy_ajp.c proxy/mod_proxy_balancer.c proxy/mod_proxy_fcgi.c proxy/mod_proxy_http.c proxy/mod_proxy_uwsgi.c
On 3/31/23 9:56 AM, Yann Ylavic wrote: > On Fri, Mar 31, 2023 at 8:28 AM Ruediger Pluem wrote: >> >> On 3/31/23 2:11 AM, yla...@apache.org wrote: >>> Author: ylavic >>> Date: Fri Mar 31 00:11:02 2023 >>> New Revision: 1908827 >>> >>> URL: http://svn.apache.org/viewvc?rev=1908827=rev >>> Log: >>> mod_proxy: Check for space/ctrls in nocanon path/urls before forwarding. >> >> Can this really happen? Wouldn't this path be rejected during request line >> parsing? > > This is on the suspenders plus belt side (check what we send), but I > suppose that an unfortunate NE[,P] rule could cause it. Don't get me wrong I like better safe than sorry, but can we have nocanon with RewriteRules? And if we think that this is a possibility shouldn't we check *ap_scan_vchar_obstext(r->filename) just before returning, to be really sure we missed no edge case? Regards Rüdiger
Re: svn commit: r1908827 - in /httpd/httpd/trunk/modules: http2/mod_proxy_http2.c proxy/mod_proxy_ajp.c proxy/mod_proxy_balancer.c proxy/mod_proxy_fcgi.c proxy/mod_proxy_http.c proxy/mod_proxy_uwsgi.c
On Fri, Mar 31, 2023 at 8:28 AM Ruediger Pluem wrote: > > On 3/31/23 2:11 AM, yla...@apache.org wrote: > > Author: ylavic > > Date: Fri Mar 31 00:11:02 2023 > > New Revision: 1908827 > > > > URL: http://svn.apache.org/viewvc?rev=1908827=rev > > Log: > > mod_proxy: Check for space/ctrls in nocanon path/urls before forwarding. > > Can this really happen? Wouldn't this path be rejected during request line > parsing? This is on the suspenders plus belt side (check what we send), but I suppose that an unfortunate NE[,P] rule could cause it. Regards; Yann.
Re: svn commit: r1908827 - in /httpd/httpd/trunk/modules: http2/mod_proxy_http2.c proxy/mod_proxy_ajp.c proxy/mod_proxy_balancer.c proxy/mod_proxy_fcgi.c proxy/mod_proxy_http.c proxy/mod_proxy_uwsgi.c
On 3/31/23 2:11 AM, yla...@apache.org wrote: > Author: ylavic > Date: Fri Mar 31 00:11:02 2023 > New Revision: 1908827 > > URL: http://svn.apache.org/viewvc?rev=1908827=rev > Log: > mod_proxy: Check for space/ctrls in nocanon path/urls before forwarding. Can this really happen? Wouldn't this path be rejected during request line parsing? Regards Rüdiger
Re: svn commit: r1907980 - in /httpd/httpd/trunk: changes-entries/proxy_uwsgi_response_validation.txt modules/proxy/mod_proxy_uwsgi.c
On Thu, Mar 2, 2023 at 8:22 PM Ruediger Pluem wrote:
>
> On 3/2/23 7:21 PM, Christophe JAILLET wrote:
> > Le 02/03/2023 à 16:10, yla...@apache.org a écrit :
> >> @@ -313,18 +313,16 @@ static int uwsgi_response(request_rec *r
> >> pass_bb = apr_brigade_create(r->pool, c->bucket_alloc);
> >> len = ap_getline(buffer, sizeof(buffer), rp, 1);
> >> -
> >> if (len <= 0) {
> >> -/* oops */
> >> +/* invalid or empty */
> >> return HTTP_INTERNAL_SERVER_ERROR;
> >> }
> >> -
> >> backend->worker->s->read += len;
> >> -
> >> -if (len >= sizeof(buffer) - 1) {
> >> -/* oops */
> >> +if ((apr_size_t)len >= sizeof(buffer)) {
> >
> > Hi Yann,
> >
> > Why removing the -1?
> >
> > My understading is that it is there in case of:
> > uwsgi_response()
> > ap_getline()
> > ap_rgetline()
> > ap_fgetline_core()
> > code around cleanup:
> >
> > In this path, IIUC, sizeof(buffer) - 1 is returned.
> > Can this happen?
>
> I think ap_fgetline_core can only return a len of sizeof(buffer) - 1 when:
>
> 1. The line is really that long.
> 2. The line is longer, but then rv != APR_SUCCESS.
>
> In case 1. all is fine and we should proceed the result.
> In case 2. ap_getline will return sizeof(buffer).
>
> Hence I think the change is correct.
Yes exactly, ap_fgetline_core() returns APR_ENOSPC if the buffer is
truncated, and ap_getline() returns sizeof(buffer).
This change avoids failing unnecessarily for a valid response line of
exactly sizeof(buffer)-1 bytes length.
Regards;
Yann.
>
> Regards
>
> Rüdiger
>
Re: svn commit: r1907980 - in /httpd/httpd/trunk: changes-entries/proxy_uwsgi_response_validation.txt modules/proxy/mod_proxy_uwsgi.c
On 3/2/23 7:21 PM, Christophe JAILLET wrote:
> Le 02/03/2023 à 16:10, yla...@apache.org a écrit :
>> Author: ylavic
>> Date: Thu Mar 2 15:10:30 2023
>> New Revision: 1907980
>>
>> URL: http://svn.apache.org/viewvc?rev=1907980=rev
>> Log:
>> mod_proxy_uwsgi: Stricter backend HTTP response parsing/validation
>>
>>
>> Added:
>> httpd/httpd/trunk/changes-entries/proxy_uwsgi_response_validation.txt
>> Modified:
>> httpd/httpd/trunk/modules/proxy/mod_proxy_uwsgi.c
>>
>> Added: httpd/httpd/trunk/changes-entries/proxy_uwsgi_response_validation.txt
>> URL:
>> http://svn.apache.org/viewvc/httpd/httpd/trunk/changes-entries/proxy_uwsgi_response_validation.txt?rev=1907980=auto
>> ==
>> --- httpd/httpd/trunk/changes-entries/proxy_uwsgi_response_validation.txt
>> (added)
>> +++ httpd/httpd/trunk/changes-entries/proxy_uwsgi_response_validation.txt
>> Thu Mar 2 15:10:30 2023
>> @@ -0,0 +1,2 @@
>> + *) mod_proxy_uwsgi: Stricter backend HTTP response parsing/validation.
>> + [Yann Ylavic]
>>
>> Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_uwsgi.c
>> URL:
>> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_uwsgi.c?rev=1907980=1907979=1907980=diff
>> ======
>> --- httpd/httpd/trunk/modules/proxy/mod_proxy_uwsgi.c (original)
>> +++ httpd/httpd/trunk/modules/proxy/mod_proxy_uwsgi.c Thu Mar 2 15:10:30
>> 2023
>> @@ -313,18 +313,16 @@ static int uwsgi_response(request_rec *r
>> pass_bb = apr_brigade_create(r->pool, c->bucket_alloc);
>> len = ap_getline(buffer, sizeof(buffer), rp, 1);
>> -
>> if (len <= 0) {
>> - /* oops */
>> + /* invalid or empty */
>> return HTTP_INTERNAL_SERVER_ERROR;
>> }
>> -
>> backend->worker->s->read += len;
>> -
>> - if (len >= sizeof(buffer) - 1) {
>> - /* oops */
>> + if ((apr_size_t)len >= sizeof(buffer)) {
>
> Hi Yann,
>
> Why removing the -1?
>
> My understading is that it is there in case of:
> uwsgi_response()
> ap_getline()
> ap_rgetline()
> ap_fgetline_core()
> code around cleanup:
>
> In this path, IIUC, sizeof(buffer) - 1 is returned.
> Can this happen?
I think ap_fgetline_core can only return a len of sizeof(buffer) - 1 when:
1. The line is really that long.
2. The line is longer, but then rv != APR_SUCCESS.
In case 1. all is fine and we should proceed the result.
In case 2. ap_getline will return sizeof(buffer).
Hence I think the change is correct.
Regards
Rüdiger
Re: svn commit: r1907980 - in /httpd/httpd/trunk: changes-entries/proxy_uwsgi_response_validation.txt modules/proxy/mod_proxy_uwsgi.c
Le 02/03/2023 à 16:10, yla...@apache.org a écrit :
Author: ylavic
Date: Thu Mar 2 15:10:30 2023
New Revision: 1907980
URL: http://svn.apache.org/viewvc?rev=1907980=rev
Log:
mod_proxy_uwsgi: Stricter backend HTTP response parsing/validation
Added:
httpd/httpd/trunk/changes-entries/proxy_uwsgi_response_validation.txt
Modified:
httpd/httpd/trunk/modules/proxy/mod_proxy_uwsgi.c
Added: httpd/httpd/trunk/changes-entries/proxy_uwsgi_response_validation.txt
URL:
http://svn.apache.org/viewvc/httpd/httpd/trunk/changes-entries/proxy_uwsgi_response_validation.txt?rev=1907980=auto
==
--- httpd/httpd/trunk/changes-entries/proxy_uwsgi_response_validation.txt
(added)
+++ httpd/httpd/trunk/changes-entries/proxy_uwsgi_response_validation.txt Thu
Mar 2 15:10:30 2023
@@ -0,0 +1,2 @@
+ *) mod_proxy_uwsgi: Stricter backend HTTP response parsing/validation.
+ [Yann Ylavic]
Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_uwsgi.c
URL:
http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_uwsgi.c?rev=1907980=1907979=1907980=diff
==
--- httpd/httpd/trunk/modules/proxy/mod_proxy_uwsgi.c (original)
+++ httpd/httpd/trunk/modules/proxy/mod_proxy_uwsgi.c Thu Mar 2 15:10:30 2023
@@ -313,18 +313,16 @@ static int uwsgi_response(request_rec *r
pass_bb = apr_brigade_create(r->pool, c->bucket_alloc);
len = ap_getline(buffer, sizeof(buffer), rp, 1);
-
if (len <= 0) {
-/* oops */
+/* invalid or empty */
return HTTP_INTERNAL_SERVER_ERROR;
}
-
backend->worker->s->read += len;
-
-if (len >= sizeof(buffer) - 1) {
-/* oops */
+if ((apr_size_t)len >= sizeof(buffer)) {
Hi Yann,
Why removing the -1?
My understading is that it is there in case of:
uwsgi_response()
ap_getline()
ap_rgetline()
ap_fgetline_core()
code around cleanup:
In this path, IIUC, sizeof(buffer) - 1 is returned.
Can this happen?
CJ
+/* too long */
return HTTP_INTERNAL_SERVER_ERROR;
}
+
[...]
mod_proxy_uwsgi.c
My plan is to work on a backport proposal for mod_proxy_uwsgi.c and an updated one for PROXY support...
