Re: hardening mod_write and mod_proxy like mod_jk with servletnormalize
On Thu, Jun 11, 2020 at 1:22 PM Yann Ylavic wrote:
>
> On Thu, Jun 11, 2020 at 9:57 AM Yann Ylavic wrote:
> >
> > On Thu, Jun 11, 2020 at 9:50 AM Yann Ylavic 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 =
Re: hardening mod_write and mod_proxy like mod_jk with servletnormalize
On Thu, Jun 11, 2020 at 9:57 AM Yann Ylavic wrote:
>
> On Thu, Jun 11, 2020 at 9:50 AM Yann Ylavic 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.
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/examples/mod_example_hooks.c
===
--- modules/examples/mod_example_hooks.c (revision 1878328)
+++ modules/examples/mod_example_hooks.c (working copy)
@@ -1176,6 +1176,22 @@ static int x_post_read_request(request_rec *r)
/*
* This routine gives our module an opportunity to translate the URI into an
+ * actual filename, before URL decoding happens.
+ *
+ * This is a RUN_FIRST hook.
+ */
+static int x_pre_translate_name(request_rec *r)
+{
+/*
+ * We don't actually *do* anything here, except note the fact that we were
+ * called.
+ */
+trace_request(r, "x_pre_translate_name()");
+return DECLINED;
+}
+
+/*
+ * This routine gives our module an opportunity to translate the URI into an
* actual filename. If we don't do anything special, the server's default
* rules (Alias directives and the like) will continue to be followed.
*
@@ -1467,6 +1483,7 @@ static void x_register_hooks(apr_pool_t *p)
ap_hook_log_transaction(x_log_transaction, NULL, NULL, APR_HOOK_MIDDLE);
ap_hook_http_scheme(x_http_scheme, NULL, NULL, APR_HOOK_MIDDLE);
ap_hook_default_port(x_default_port, NULL, NULL, APR_HOOK_MIDDLE);
+ap_hook_pre_translate_name(x_pre_translate_name, NULL, NULL, APR_HOOK_MIDDLE);
ap_hook_translate_name(x_translate_name, NULL, NULL, APR_HOOK_MIDDLE);
ap_hook_map_to_storage(x_map_to_storage, NULL,NULL, APR_HOOK_MIDDLE);
ap_hook_header_parser(x_header_parser, NULL, NULL, APR_HOOK_MIDDLE);
Index: modules/generators/mod_autoindex.c
===
--- modules/generators/mod_autoindex.c (revision 1878328)
+++ modules/gen
Re: hardening mod_write and mod_proxy like mod_jk with servletnormalize
On 11/06/2020 07:51, jean-frederic clere wrote:
> On 10/06/2020 11:53, Ruediger Pluem wrote:
>>
>>
>> On 6/9/20 12:05 PM, jean-frederic clere wrote:
>>> Hi,
>>>
>>> Basically it adds servletnormalizecheck to mod_proxy for
>>> ProxyPass/ProxyPassMatch and mod_rewrite when using P
>>> I have tested the following uses:
>>> #ProxyPass /docs ajp://localhost:8009/docs secret=%A1b2!@
>>> servletnormalizecheck
>>>
>>> #ProxyPassMatch "^/docs(.*)$" "ajp://localhost:8009/docs$1"
>>> secret=%A1b2!@ servletnormalizecheck
>>>
>>> #RewriteEngine On
>>> #RewriteRule "^/docs(.*)$" "ajp://localhost:8009/docs$1" [P,SNC]
>>> #
>>> #ProxySet connectiontimeout=5 timeout=30 secret=%A1b2!@
>>> #
>>>
>>> #
>>> # ProxyPass ajp://localhost:8009/docs secret=%A1b2!@
>>> servletnormalizecheck
>>> #
>>>
>>> What is not supported is
>>> curl -v --path-as-is
>>> "http://localhost:8000/docs/..;foo=bar/;foo=bar/test/index.jsp";
>>>
>>> that could be remapped to
>>> ProxyPass /test ajp://localhost:8009/test secret=%A1b2!@
>>> servletnormalizecheck
>>> or a
>>>
>>> Comments?
>>
>> I understood from Mark that the request you do above with curl should
>> not be denied but just mapped to /test.
>> But rethinking that, it becomes real fun: For mapping we should use
>> the URI stripped off path parameters and then having done the
>> shrinking operation (servlet normalized) but we should use the
>> original URI having done the shrinking operation with path
>> parameters to sent to the backend. That might work for a simple prefix
>> matching, but it seems to be very difficult for regular
>> expression scenarios where you might use complex captures from the
>> matching to build the result. But if the matching was done
>> against the servlet normalized URI the captures might be different,
>> than the ones you would have got when doing the same against
>> not normalized URI. So I am little bit lost here.
I can see how this gets complicated for regular expression scenarios.
Since the servlet specification doesn't have the concept of regular
expression mapping, I don't think the rationale for servletnormalize
applies in that case. There is no expectation of how the mapping will
occur from a servlet perspective so the httpd behaviour cannot be
unexpected.
Coming from a servlet perspective I have no view on what the 'correct'
behaviour is in this case. I'll happily support whatever the httpd
community thinks is best.
>> What if we just have an option on virtual host base to drop path
>> parameters of the following kind
>>
>> s#/([.]{0,2})(;[^/]*)/#/$1/g
>>
>> do the usual shrinking operation afterwards and just process them
>> afterwards as usual.
>
> I think it makes sense to have it there but separated from the
> servletnormalizecheck because that changes the whole mapping
> So I will add something like MergeSlashes which will map
> http://localhost:8000/docs/..;foo=bar/;foo=bar/test/index.jsp
> to /test
> And arrange the proxy so that /docs/..;foo=bar/;foo=bar/test/index.jsp
> is sent to the back-end.
That sounds good to me. That is the expected mapping from a servlet
perspective.
Thanks for all your efforts on this.
Mark
Re: hardening mod_write and mod_proxy like mod_jk with servletnormalize
On Thu, Jun 11, 2020 at 9:50 AM Yann Ylavic 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():
/* Ignore URL unescaping for proxy requests */
if (!r->proxyreq && 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);
}
else {
access_status = ap_unescape_url(r->parsed_uri.path);
}
if (access_status) {
if (access_status == HTTP_NOT_FOUND) {
if (! d->allow_encoded_slashes) {
ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(00026)
"found %%2f (encoded '/') in URI "
"(decoded='%s'), returning 404",
r->uri);
}
}
return access_status;
}
}
Should go _after_ the following:
if ((access_status = ap_run_translate_name(r))) {
return decl_die(access_status, "translate", r);
}
But it looks like a breaking change for 2.4.x..
Re: hardening mod_write and mod_proxy like mod_jk with servletnormalize
On Thu, Jun 11, 2020 at 8:52 AM jean-frederic clere wrote:
>
> Should I commit my first proposal (it is easily backportable to 2.4.x)
> and later work on the next one?
How about something like the attached patch?
It's a new single ap_normalize_path() helper with options (like
AP_NORMALIZE_MERGE_SLASHES and AP_NORMALIZE_PATH_PARAMETERS), which we
can use in multiple places to replace ap_getparents(). The patch is
without the mod_rewrite bits (for now), and uses the "mapping=servlet"
syntax for the new proxypass parameter (which I found more
extensible).
The issue with re-calling ap_getparents() in ap_proxy_trans_match()
(or ap_normalize_path() in my patch still) is that it happens after
%-decoding, so bets are off.
For instance "/docs/..%3Bfoo=bar/%3Bfoo=bar/test/index.jsp" is not the
same as "http://localhost:8000/docs/..;foo=bar/;foo=bar/test/index.jsp";
and shouldn't be matched the same by mapping=servlet.
We need a way to forward non %-decoded URLs upto mod_proxy (reverse)
if we want to normalize a second time..
Regards;
Yann.
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/proxy/mod_proxy.c
===
--- modules/proxy/mod_proxy.c (revision 1878328)
+++ modules/proxy/mod_proxy.c (working copy)
@@ -753,6 +753,25 @@ PROXY_DECLARE(int) ap_proxy_trans_match(request_re
mismatch = 1;
use_uri = r->uri;
}
+else if (!nocanon && (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);
}
}
@@ -1806,6 +1825,9 @@ static const char *
else if (!strcasecmp(word,"noquery")) {
flags |= PROXYPASS_NOQUERY;
}
+else if (!strcasecmp(word,"mapping=servlet")) {
+flags |= PRO
