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)
+++
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 |=
Re: hardening mod_write and mod_proxy like mod_jk with servletnormalize
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.
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.
Should I commit my first proposal (it is easily backportable to 2.4.x)
and later work on the next one?
Regards
Rüdiger
--
Cheers
Jean-Frederic
