On Tue, Feb 2, 2021 at 8:50 PM <rpl...@apache.org> 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.