Re: svn commit: r1886141 - /httpd/httpd/trunk/modules/proxy/mod_proxy_http.c

2021-02-03 Thread Yann Ylavic 
On Wed, Feb 3, 2021 at 9:48 AM Ruediger Pluem  wrote:
>
> On 2/3/21 1:23 AM, Yann Ylavic wrote:
> >
> > I think we can remove this test/block completely now that
>
> Agreed. I think we should just decline and leave it to further possible 
> protocol handlers to deal
> with these URL's. And if all decline we just hit the catch all that there is 
> no handler for this URL.
> So +1 to just remove the block. Will you do?

r1886151.

Regards;
Yann.

Re: svn commit: r1886141 - /httpd/httpd/trunk/modules/proxy/mod_proxy_http.c

2021-02-03 Thread Ruediger Pluem 



On 2/3/21 1:23 AM, Yann Ylavic wrote:
> On Tue, Feb 2, 2021 at 8:50 PM  wrote:
>>
>> Author: rpluem
>> Date: Tue Feb  2 19:50:14 2021
>> New Revision: 1886141
>>
> []
>>  if (!scheme || u[0] != '/' || u[1] != '/' || u[2] == '\0') {
>> -if (!scheme && (u = strchr(url, ':')) && (u - url) > 14) {
>> -ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10262)
>> -  "overlong proxy URL scheme in %s", url);
>> -return HTTP_BAD_REQUEST;
>> -}
>>  ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01113)
>>"HTTP: declining URL %s", url);
>>  return DECLINED; /* only interested in HTTP, WS or FTP via proxy */
>>  }
>> +if (!scheme && (u = strchr(url, ':')) && (u - url) > 14) {
>> +ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10262)
>> +  "overlong proxy URL scheme in %s", url);
>> +return HTTP_BAD_REQUEST;
>> +}
> 
> Hmm, actually this !scheme here is unreachable now.

Good catch.

> I think we can remove this test/block completely now that

Agreed. I think we should just decline and leave it to further possible 
protocol handlers to deal
with these URL's. And if all decline we just hit the catch all that there is no 
handler for this URL.
So +1 to just remove the block. Will you do?

Regards

Rüdiger

Re: svn commit: r1886141 - /httpd/httpd/trunk/modules/proxy/mod_proxy_http.c

2021-02-02 Thread Yann Ylavic 
On Tue, Feb 2, 2021 at 8:50 PM  wrote:
>
> Author: rpluem
> Date: Tue Feb  2 19:50:14 2021
> New Revision: 1886141
>
[]
>  if (!scheme || u[0] != '/' || u[1] != '/' || u[2] == '\0') {
> -if (!scheme && (u = strchr(url, ':')) && (u - url) > 14) {
> -ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10262)
> -  "overlong proxy URL scheme in %s", url);
> -return HTTP_BAD_REQUEST;
> -}
>  ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01113)
>"HTTP: declining URL %s", url);
>  return DECLINED; /* only interested in HTTP, WS or FTP via proxy */
>  }
> +if (!scheme && (u = strchr(url, ':')) && (u - url) > 14) {
> +ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10262)
> +  "overlong proxy URL scheme in %s", url);
> +return HTTP_BAD_REQUEST;
> +}

Hmm, actually this !scheme here is unreachable now.
I think we can remove this test/block completely now that
get_url_scheme() checks the exact scheme.
The old code was like this (removed hunk from r1885239):
-if ((u - url) > 14)
-return HTTP_BAD_REQUEST;
-scheme = apr_pstrmemdup(p, url, u - url);
and probably meant to avoid allocating an unreasonable scheme, but
thanks to get_url_scheme() we do not allocate anymore, so this check
is probably useless now.

It's quite hard to reject overlong schemes in mod_proxy_http anyway,
because it'll really take the !scheme branch above, and since a scheme
is [[:alnum:].+-] it may well be a hostname too and match a CONNECT
URI.
So should we check for:
if (!scheme || u[0] != '/' || u[1] != '/' || u[2] == '\0') {
   if (!scheme && (u = strchr(url, ':')) && (u - url) > 14 &&
!apr_isdigit(u[0])) {
   ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10262)
 "overlong proxy URL scheme in %s", url);
   return HTTP_BAD_REQUEST;
   }
   return DECLINED; /* only interested in HTTP, WS or FTP via proxy */
}
or something?

Regards;
Yann.