Re: svn commit: r1886141 - /httpd/httpd/trunk/modules/proxy/mod_proxy_http.c
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
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
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.