On Tue, Feb 2, 2021 at 8:50 PM <[email protected]> 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.
