On Thu, Jun 11, 2020 at 1:22 PM Yann Ylavic <ylavic....@gmail.com> wrote: > > On Thu, Jun 11, 2020 at 9:57 AM Yann Ylavic <ylavic....@gmail.com> wrote: > > > > On Thu, Jun 11, 2020 at 9:50 AM Yann Ylavic <ylavic....@gmail.com> wrote: > > > > > > We need a way to forward non %-decoded URLs upto mod_proxy (reverse) > > > if we want to normalize a second time.. > > > > IOW, this block in ap_process_request_internal(): > [snip] > > Should go _after_ the following: > [snip] > > Or we could introduce a new pre_translate_name hook which would > execute before %-decoding, and be used by mod_proxy when > "ProxyPreTranslation on" is configured, and be a prerequisite for > mapping=servlet. > > I find ProxyPreTranslation also useful for the non-servlet case btw. > > Something like this attached v2 patch.
Here is a v3 with the relevant pre_translate_name hooks only and ap_getparents() preserved when the URI does not start with '/' (which makes the patch read better too). > > Regards; > Yann.
Index: include/http_request.h =================================================================== --- include/http_request.h (revision 1878328) +++ include/http_request.h (working copy) @@ -365,6 +365,16 @@ AP_DECLARE_HOOK(int,create_request,(request_rec *r /** * This hook allow modules an opportunity to translate the URI into an + * actual filename, before URL decoding happens. + * rules will be followed. + * @param r The current request + * @return OK, DECLINED, or HTTP_... + * @ingroup hooks + */ +AP_DECLARE_HOOK(int,pre_translate_name,(request_rec *r)) + +/** + * This hook allow modules an opportunity to translate the URI into an * actual filename. If no modules do anything special, the server's default * rules will be followed. * @param r The current request Index: include/httpd.h =================================================================== --- include/httpd.h (revision 1878328) +++ include/httpd.h (working copy) @@ -1779,8 +1779,21 @@ AP_DECLARE(void) ap_no2slash(char *name) AP_DECLARE(void) ap_no2slash_ex(char *name, int is_fs_path) AP_FN_ATTR_NONNULL_ALL; +#define AP_NORMALIZE_NOT_ABOVE_ROOT (1u << 0) +#define AP_NORMALIZE_MERGE_SLASHES (1u << 1) +#define AP_NORMALIZE_PATH_PARAMETERS (1u << 2) /** + * Remove all ////, /./ and /xx/../ substrings from a path, and more + * depending on passed in flags. + * @param path The path to normalize + * @param flags bitmask of AP_NORMALIZE_* flags + * @return non-zero on success + */ +AP_DECLARE(int) ap_normalize_path(char *path, unsigned int flags) + AP_FN_ATTR_NONNULL((1)); + +/** * Remove all ./ and xx/../ substrings from a file name. Also remove * any leading ../ or /../ substrings. * @param name the file name to parse Index: modules/dav/main/util.c =================================================================== --- modules/dav/main/util.c (revision 1878328) +++ modules/dav/main/util.c (working copy) @@ -664,7 +664,11 @@ static dav_error * dav_process_if_header(request_r /* note that parsed_uri.path is allocated; we can trash it */ /* clean up the URI a bit */ - ap_getparents(parsed_uri.path); + if (!ap_normalize_path(parsed_uri.path, 0)) { + return dav_new_error(r->pool, HTTP_BAD_REQUEST, + DAV_ERR_IF_TAGGED, rv, + "Invalid URI path tagged If-header."); + } /* the resources we will compare to have unencoded paths */ if (ap_unescape_url(parsed_uri.path) != OK) { Index: modules/generators/mod_autoindex.c =================================================================== --- modules/generators/mod_autoindex.c (revision 1878328) +++ modules/generators/mod_autoindex.c (working copy) @@ -1266,8 +1266,7 @@ static struct ent *make_parent_entry(apr_int32_t a if (!(p->name = ap_make_full_path(r->pool, r->uri, "../"))) { return (NULL); } - ap_getparents(p->name); - if (!*p->name) { + if (!ap_normalize_path(p->name, 0)) { return (NULL); } Index: modules/generators/mod_info.c =================================================================== --- modules/generators/mod_info.c (revision 1878328) +++ modules/generators/mod_info.c (working copy) @@ -322,6 +322,7 @@ static const hook_lookup_t request_hooks[] = { {"HTTP Scheme", ap_hook_get_http_scheme}, {"Default Port", ap_hook_get_default_port}, {"Quick Handler", ap_hook_get_quick_handler}, + {"Pre-Translate Name", ap_hook_get_pre_translate_name}, {"Translate Name", ap_hook_get_translate_name}, {"Map to Storage", ap_hook_get_map_to_storage}, {"Check Access", ap_hook_get_access_checker_ex}, Index: modules/proxy/mod_proxy.c =================================================================== --- modules/proxy/mod_proxy.c (revision 1878328) +++ modules/proxy/mod_proxy.c (working copy) @@ -753,6 +753,26 @@ PROXY_DECLARE(int) ap_proxy_trans_match(request_re mismatch = 1; use_uri = r->uri; } + else if (dconf->pre_trans > 0 + && (ent->flags & PROXYPASS_MAPPING_SERVLET)) { + char *uri = apr_pstrdup(r->pool, r->uri); + + /* check against the backend servlet url */ + if (!ap_normalize_path(uri, AP_NORMALIZE_MERGE_SLASHES | + AP_NORMALIZE_PATH_PARAMETERS)) { + ap_log_rerror(APLOG_MARK, APLOG_TRACE1, 0, r, + "servlet normalization failed for '%s'", r->uri); + return HTTP_INTERNAL_SERVER_ERROR; + } + + if (!alias_match(uri, ent->fake)) { + ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO() + "servlet normalization for '%s' mismatch; " + "declining '%s'", r->uri, uri); + return DECLINED; + } + } + found = apr_pstrcat(r->pool, "proxy:", real, use_uri + len, NULL); } } @@ -795,7 +815,7 @@ PROXY_DECLARE(int) ap_proxy_trans_match(request_re return DONE; } -static int proxy_trans(request_rec *r) +static int proxy_trans(request_rec *r, int pre) { int i; struct proxy_alias *ent; @@ -825,6 +845,10 @@ PROXY_DECLARE(int) ap_proxy_trans_match(request_re dconf = ap_get_module_config(r->per_dir_config, &proxy_module); + if ((pre != 0) ^ (dconf->pre_trans > 0)) { + return DECLINED; + } + /* short way - this location is reverse proxied? */ if (dconf->alias) { int rv = ap_proxy_trans_match(r, dconf->alias, dconf); @@ -849,6 +873,15 @@ PROXY_DECLARE(int) ap_proxy_trans_match(request_re return DECLINED; } +static int proxy_pre_translate_name(request_rec *r) +{ + return proxy_trans(r, 1); +} +static int proxy_translate_name(request_rec *r) +{ + return proxy_trans(r, 0); +} + static int proxy_walk(request_rec *r) { proxy_server_conf *sconf = ap_get_module_config(r->server->module_config, @@ -1593,6 +1626,7 @@ static void *create_proxy_dir_config(apr_pool_t *p new->add_forwarded_headers_set = 0; new->forward_100_continue = 1; new->forward_100_continue_set = 0; + new->pre_trans = -1; /* unset */ return (void *) new; } @@ -1651,6 +1685,9 @@ static void *merge_proxy_dir_config(apr_pool_t *p, new->forward_100_continue_set = add->forward_100_continue_set || base->forward_100_continue_set; + new->pre_trans = (add->pre_trans == -1) ? base->pre_trans + : add->pre_trans; + return new; } @@ -1806,6 +1843,9 @@ static const char * else if (!strcasecmp(word,"noquery")) { flags |= PROXYPASS_NOQUERY; } + else if (!strcasecmp(word,"mapping=servlet")) { + flags |= PROXYPASS_MAPPING_SERVLET; + } else { char *val = strchr(word, '='); if (!val) { @@ -2799,6 +2839,9 @@ static const command_rec proxy_cmds[] = AP_INIT_FLAG("Proxy100Continue", forward_100_continue, NULL, RSRC_CONF|ACCESS_CONF, "on if 100-Continue should be forwarded to the origin server, off if the " "proxy should handle it by itself"), + AP_INIT_FLAG("ProxyPreTranslate", ap_set_flag_slot_char, + (void*)APR_OFFSETOF(proxy_dir_conf, pre_trans), + RSRC_CONF|ACCESS_CONF, "Run URI translation with no URL percent decoding"), {NULL} }; @@ -3147,7 +3190,10 @@ static void register_hooks(apr_pool_t *p) /* handler */ ap_hook_handler(proxy_handler, NULL, NULL, APR_HOOK_FIRST); /* filename-to-URI translation */ - ap_hook_translate_name(proxy_trans, aszSucc, NULL, APR_HOOK_FIRST); + ap_hook_pre_translate_name(proxy_pre_translate_name, aszSucc, NULL, + APR_HOOK_MIDDLE); + ap_hook_translate_name(proxy_translate_name, aszSucc, NULL, + APR_HOOK_FIRST); /* walk <Proxy > entries and suppress default TRACE behavior */ ap_hook_map_to_storage(proxy_map_location, NULL,NULL, APR_HOOK_FIRST); /* fixups */ Index: modules/proxy/mod_proxy.h =================================================================== --- modules/proxy/mod_proxy.h (revision 1878328) +++ modules/proxy/mod_proxy.h (working copy) @@ -123,6 +123,7 @@ struct proxy_remote { #define PROXYPASS_NOCANON 0x01 #define PROXYPASS_INTERPOLATE 0x02 #define PROXYPASS_NOQUERY 0x04 +#define PROXYPASS_MAPPING_SERVLET 0x08 struct proxy_alias { const char *real; const char *fake; @@ -243,6 +244,9 @@ typedef struct { unsigned int forward_100_continue_set:1; apr_array_header_t *error_override_codes; + + /** Whether to run translation in pre_translate_name hook */ + int pre_trans; } proxy_dir_conf; /* if we interpolate env vars per-request, we'll need a per-request Index: server/protocol.c =================================================================== --- server/protocol.c (revision 1878328) +++ server/protocol.c (working copy) @@ -627,6 +627,14 @@ AP_CORE_DECLARE(void) ap_parse_uri(request_rec *r, } else { status = apr_uri_parse(r->pool, uri, &r->parsed_uri); + if (status == APR_SUCCESS + && (r->parsed_uri.path != NULL) + && (r->parsed_uri.path[0] != '/') + && (r->method_number != M_OPTIONS + || strcmp(r->parsed_uri.path, "*") != 0)) { + /* Invalid request-target per rfc7230#section-5.3 */ + status = APR_EINVAL; + } } if (status == APR_SUCCESS) { @@ -640,8 +648,15 @@ AP_CORE_DECLARE(void) ap_parse_uri(request_rec *r, } r->args = r->parsed_uri.query; - r->uri = r->parsed_uri.path ? r->parsed_uri.path - : apr_pstrdup(r->pool, "/"); + if (r->parsed_uri.path) { + r->uri = r->parsed_uri.path; + } + else if (r->method_number == M_OPTIONS) { + r->uri = apr_pstrdup(r->pool, "*"); + } + else { + r->uri = apr_pstrdup(r->pool, "/"); + } #if defined(OS2) || defined(WIN32) /* Handle path translations for OS/2 and plug security hole. Index: server/request.c =================================================================== --- server/request.c (revision 1878328) +++ server/request.c (working copy) @@ -59,6 +59,7 @@ #define APLOG_MODULE_INDEX AP_CORE_MODULE_INDEX APR_HOOK_STRUCT( + APR_HOOK_LINK(pre_translate_name) APR_HOOK_LINK(translate_name) APR_HOOK_LINK(map_to_storage) APR_HOOK_LINK(check_user_id) @@ -74,6 +75,8 @@ APR_HOOK_STRUCT( APR_HOOK_LINK(force_authn) ) +AP_IMPLEMENT_HOOK_RUN_FIRST(int,pre_translate_name, + (request_rec *r), (r), DECLINED) AP_IMPLEMENT_HOOK_RUN_FIRST(int,translate_name, (request_rec *r), (r), DECLINED) AP_IMPLEMENT_HOOK_RUN_FIRST(int,map_to_storage, @@ -170,11 +173,35 @@ AP_DECLARE(int) ap_process_request_internal(reques core_server_config *sconf = ap_get_core_module_config(r->server->module_config); - /* Ignore embedded %2F's in path for proxy requests */ - if (!r->proxyreq && r->parsed_uri.path) { + if (r->parsed_uri.path) { + unsigned int normalize_flags = 0; + if (sconf->merge_slashes != AP_CORE_CONFIG_OFF) { + normalize_flags |= AP_NORMALIZE_MERGE_SLASHES; + } + + /* Shrinking transformations */ + if (!ap_normalize_path(r->parsed_uri.path, normalize_flags)) { + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO() + "invalid URI path (%s)", r->uri); + return HTTP_BAD_REQUEST; + } + + /* Apply transformations */ + r->uri = r->parsed_uri.path; + } + + /* Let pre_translate_name hooks own non-decoded URIs */ + access_status = ap_run_pre_translate_name(r); + if (access_status != OK && access_status != DECLINED) { + return access_status; + } + + /* Ignore URL unescaping for translated URIs already */ + if (access_status == DECLINED && r->parsed_uri.path) { d = ap_get_core_module_config(r->per_dir_config); if (d->allow_encoded_slashes) { - access_status = ap_unescape_url_keep2f(r->parsed_uri.path, d->decode_encoded_slashes); + access_status = ap_unescape_url_keep2f(r->parsed_uri.path, + d->decode_encoded_slashes); } else { access_status = ap_unescape_url(r->parsed_uri.path); @@ -185,21 +212,16 @@ AP_DECLARE(int) ap_process_request_internal(reques ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(00026) "found %%2f (encoded '/') in URI " "(decoded='%s'), returning 404", - r->parsed_uri.path); + r->uri); } } return access_status; } + + /* Apply transformations */ + r->uri = r->parsed_uri.path; } - ap_getparents(r->uri); /* OK --- shrinking transformations... */ - if (sconf->merge_slashes != AP_CORE_CONFIG_OFF) { - ap_no2slash(r->uri); - if (r->parsed_uri.path) { - ap_no2slash(r->parsed_uri.path); - } - } - /* All file subrequests are a huge pain... they cannot bubble through the * next several steps. Only file subrequests are allowed an empty uri, * otherwise let translate_name kill the request. Index: server/util.c =================================================================== --- server/util.c (revision 1878328) +++ server/util.c (working copy) @@ -77,7 +77,7 @@ #include "test_char.h" /* Win32/NetWare/OS2 need to check for both forward and back slashes - * in ap_getparents() and ap_escape_url. + * in ap_normalize_path() and ap_escape_url(). */ #ifdef CASE_BLIND_FILESYSTEM #define IS_SLASH(s) ((s == '/') || (s == '\\')) @@ -492,8 +492,108 @@ AP_DECLARE(apr_status_t) ap_pregsub_ex(apr_pool_t return rc; } +/* Forward declare */ +static char x2c(const char *what); + +#define IS_SLASH_OR_NUL(s) (s == '\0' || IS_SLASH(s)) + +static int normalize_path(char *path, unsigned int flags) +{ + apr_size_t l, w; + + /* We assume path[0] == '/' below */ + if (!IS_SLASH(path[0])) { + /* Besides "OPTIONS *" requests, a request-target path should start + * with '/' (RFC-7230 section-5.3), so anything else is invalid. + */ + return (path[0] == '*' && path[1] == '\0'); + } + + for (l = w = 1; path[l] != '\0';) { + if ((flags & AP_NORMALIZE_PATH_PARAMETERS) && path[l] == ';') { + /* Remove path parameters ;foo=bar/ from any path segment */ + do { + l++; + } while (!IS_SLASH_OR_NUL(path[l])); + continue; + } + + /* RFC-3986 section 2.3: + * For consistency, percent-encoded octets in the ranges of + * ALPHA (%41-%5A and %61-%7A), DIGIT (%30-%39), hyphen (%2D), + * period (%2E), underscore (%5F), or tilde (%7E) should [...] + * be decoded to their corresponding unreserved characters by + * URI normalizers. + */ + if (path[l] == '%' && apr_isxdigit(path[l + 1]) + && apr_isxdigit(path[l + 2])) { + const char c = x2c(&path[l + 1]); + if (apr_isalnum(c) || (c && strchr("-._~", c))) { + /* Replace last char and fall through as the current + * read position */ + l += 2; + path[l] = c; + } + } + + if (IS_SLASH(path[w - 1])) { + if ((flags & AP_NORMALIZE_MERGE_SLASHES) && IS_SLASH(path[l])) { + /* Collapse ///// sequences to / */ + do { + l++; + } while (IS_SLASH(path[l])); + continue; + } + + if (path[l] == '.') { + if (IS_SLASH_OR_NUL(path[l + 1])) { + /* Remove /./ segments */ + l++; + if (path[l]) { + l++; + } + continue; + } + + if (path[l + 1] == '.' && IS_SLASH_OR_NUL(path[l + 2])) { + /* Remove /xx/../ segments */ + if (w == 1) { + if (flags & AP_NORMALIZE_NOT_ABOVE_ROOT) { + return 0; + } + l += 2; + continue; + } + + /* Wind w back to remove the previous segment */ + do { + w--; + } while (!IS_SLASH(path[w - 1])); + + /* Move l forward to the next segment */ + l += 2; + if (path[l]) { + l++; + } + continue; + } + } + } + + path[w++] = path[l++]; + } + path[w] = '\0'; + + return 1; +} + +AP_DECLARE(int) ap_normalize_path(char *path, unsigned int flags) +{ + return normalize_path(path, flags); +} + /* - * Parse .. so we don't compromise security + * Normalize .. so we don't compromise security */ AP_DECLARE(void) ap_getparents(char *name) { @@ -500,6 +600,14 @@ AP_DECLARE(void) ap_getparents(char *name) char *next; int l, w, first_dot; + /* If the path starts with '/', ap_normalize_path() is faster. + * Otherthise fallback to legacy four passes. + */ + if (IS_SLASH(name[0])) { + (void)normalize_path(name, 0); + return; + } + /* Four paseses, as per RFC 1808 */ /* a) remove ./ path segments */ for (next = name; *next && (*next != '.'); next++) { @@ -561,6 +669,7 @@ AP_DECLARE(void) ap_getparents(char *name) name[l] = '\0'; } } + AP_DECLARE(void) ap_no2slash_ex(char *name, int is_fs_path) {