Passed: apache/httpd#885 (trunk - e238232)
Build Update for apache/httpd - Build: #885 Status: Passed Duration: 3 mins and 38 secs Commit: e238232 (trunk) Author: Joe Orton Message: Add a plain mod_cgid build, explicitly without mod_cgi. git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1879184 13f79535-47bb-0310-9956-ffa450edef68 View the changeset: https://github.com/apache/httpd/compare/114421d22f84...e23823217472 View the full build log and details: https://travis-ci.org/github/apache/httpd/builds/702008462?utm_medium=notification_source=email -- You can unsubscribe from build emails from the apache/httpd repository going to https://travis-ci.org/account/preferences/unsubscribe?repository=69847_medium=notification_source=email. Or unsubscribe from *all* email updating your settings at https://travis-ci.org/account/preferences/unsubscribe?utm_medium=notification_source=email. Or configure specific recipients for build notifications in your .travis.yml file. See https://docs.travis-ci.com/user/notifications.
Re: svn commit: r1879080 - in /httpd/httpd/trunk/modules/proxy: mod_proxy.c mod_proxy.h
On 6/24/20 1:09 PM, Yann Ylavic wrote:
> On Tue, Jun 23, 2020 at 9:59 AM Ruediger Pluem wrote:
>>
>> On 6/22/20 12:37 PM, yla...@apache.org wrote:
>>>
>>> +/*
>>> + * Inspired by mod_jk's jk_servlet_normalize().
>>> + */
>>> +static int alias_match_servlet(apr_pool_t *p,
>>> + const char *uri,
>>> + const char *alias)
>>> +{
>>
>> The code for alias_match_servlet looks awful complex.
>
> Yeah I know..
>
>> I think it is this way not because it is done wrong in some way, but because
>> the problem to solve is complex. This brought me to the point if it is worth
>> having this complexity.
>
> I suppose that it helps to close a mod_jk vs mod_proxy_ajp gap, but I
> don't use either actually.
> It seems that Jean-Frédéric is interested in the feature, probably
> some Tomcat users too who want the proxy to isolate/restrict
> applications (paths) on the proxy.
>
>> My understanding is that the original issue we faced was that traversal
>> problem as HTTP and Servlet spec handle path parameters
>> differently in the '..' case. So we need to do something about
>>
>> /app/..;pp=somevalue/admin
>>
>> URI patterns. The question for me is: Are URI's that contain these patterns
>> sensible for a servlet based application or can we
>> always assume bad intentions by the originator?
>
> Dunno, we do not assume bad intentions for "../" patterns in
> non-servlet paths though, and normalize them.
In non-servlet paths we just normalize them in case of ../, but we just keep
them in case of ..;pp=somevalue/.
I think now in the servlet case we silently ignore the path parameter and just
normalize as if it was just ../
> I know about the OpenAPI specification, and you probably don't want to
> see how applications put segment parameters all over the place ;)
> All I can tell is that in the API/REST world, it's quite used (never
> saw the "..;" thing, though).
>
>> If we always assume bad intentions we could just reject such URI's (probably
>> only
>> if they match a ProxyPass with a flag set, for the Rewrite case we could
>> just document a Rewrite rule that kills them).
>
> Could be, but the nominal use case is probably the below (IMHO).
>
>> The other case I see covered with alias_match_servlet is the case where we
>> have path parameters on the prefix part that ProxyPass
>> should match. Without this commit
>>
>> /app;pp=something/somethingelse
>>
>> wouldn't match
>>
>> ProxyPass /app schema://backend/app
>
> This, I suppose applications that handle path/segment parameters want
> to see the ones for /app.
> That's a real use case IMO.
>
>>
>> But given the complexity of the code I am not sure if these cases are worth
>> fixing or if people should just use ProyPassMatch / a
>> RewriteRule that covers such cases if the application needs that. Of course
>> that depends on how often such cases appear. If they
>> are frequent than a direct ProxyPass support seems sensible. If they are
>> rare probably not.
>
> Yeah, it depends, Tomcat users are probably more able to answer this than me.
> I _think_ I got alias_match_servlet() right, sure it's complex, but we
> have it now..
Yes and this is fine. Thanks for doing. I just worry a little bit about if such
complex code isn't prone to vulnerabilities.
> It can possibly be simplified a bit (store more than a single int per
> segment in the stack, so that we don't need to maintain a normalized
> path separately for rewind), but it's complex anyway I concur.
>
> Possibly I'll need a mapping=openapi some day, at least I have a base
> implementation for that now (not sure it's interesting people either
> and that I would propose it upstream..).
>
> Anyway, if there is a need for app isolation on the proxy with
> accurate path parameters forwarding, it's not such a bad way to do it.
> If there is no need, and we simply want to block "..;", I agree that a
> RewriteRule is more than enough.
The other thing that still worries me and that I think is not covered is the
following scenario:
. some authn and authz
ProxyPass / http://
Here the Apache is used to do authn and authz for the backend.
/app/..;pp=somevalue/admin/nastything
IMHO could circumvent this no matter what the mapping option is set to in
ProxyPass
Regards
Rüdiger
Re: svn commit: r1879080 - in /httpd/httpd/trunk/modules/proxy: mod_proxy.c mod_proxy.h
On Thu, Jun 25, 2020 at 9:24 AM Ruediger Pluem wrote:
>
> The other thing that still worries me and that I think is not covered is the
> following scenario:
>
>
>
> . some authn and authz
>
>
>
> ProxyPass / http://
>
> Here the Apache is used to do authn and authz for the backend.
>
> /app/..;pp=somevalue/admin/nastything
>
> IMHO could circumvent this no matter what the mapping option is set to in
> ProxyPass
Yes, very true.
But if we want mapping=servlet to take "ownership" of r->uri, we can
do something like the attached patch.
The result is that if mod_proxy matches a servlet URI-path, r->uri
gets servlet normalized (without the path parameters) for the rest of
the request handling.
So your above example gets rewritten to "/admin/nastything" and
"" now applies.
Would that be better?
Regards;
Yann
Index: modules/proxy/mod_proxy.c
===
--- modules/proxy/mod_proxy.c (revision 1879145)
+++ modules/proxy/mod_proxy.c (working copy)
@@ -574,10 +574,11 @@ static int alias_match(const char *uri, const char
* Inspired by mod_jk's jk_servlet_normalize().
*/
static int alias_match_servlet(apr_pool_t *p,
- const char *uri,
+ const char **urip,
const char *alias)
{
char *map;
+const char *uri = *urip;
apr_array_header_t *stack;
int map_pos, uri_pos, alias_pos, first_pos;
int alias_depth = 0, depth;
@@ -759,6 +760,8 @@ static int alias_match_servlet(apr_pool_t *p,
if (alias[alias_pos - 1] != '/' && uri[uri_pos - 1] == '/') {
uri_pos--;
}
+
+*urip = map;
return uri_pos;
}
@@ -869,7 +872,7 @@ PROXY_DECLARE(int) ap_proxy_trans_match(request_re
ap_regmatch_t regm[AP_MAX_REG_MATCH];
ap_regmatch_t reg1[AP_MAX_REG_MATCH];
char *found = NULL;
-int mismatch = 0;
+int mismatch = 0, servlet = 0;
unsigned int nocanon = ent->flags & PROXYPASS_NOCANON;
const char *use_uri = nocanon ? r->unparsed_uri : r->uri;
@@ -933,8 +936,9 @@ PROXY_DECLARE(int) ap_proxy_trans_match(request_re
}
else {
if ((ent->flags & PROXYPASS_MAP_SERVLET) == PROXYPASS_MAP_SERVLET) {
+len = alias_match_servlet(r->pool, (const char **)>uri, fake);
nocanon = 0; /* ignored since servlet's normalization applies */
-len = alias_match_servlet(r->pool, r->uri, fake);
+servlet = 1;
}
else {
len = alias_match(r->uri, fake);
@@ -969,7 +973,7 @@ PROXY_DECLARE(int) ap_proxy_trans_match(request_re
*/
int rc = proxy_run_check_trans(r, found + 6);
if (rc != OK && rc != DECLINED) {
-return DONE;
+return HTTP_CONTINUE;
}
r->filename = found;
@@ -987,10 +991,10 @@ PROXY_DECLARE(int) ap_proxy_trans_match(request_re
"URI path '%s' matches proxy handler '%s'", r->uri,
found);
-return OK;
+return servlet ? DONE : OK;
}
-return DONE;
+return HTTP_CONTINUE;
}
static int proxy_trans(request_rec *r, int pre_trans)
@@ -1042,7 +1046,7 @@ static int proxy_trans(request_rec *r, int pre_tra
enc = (dconf->alias->flags & PROXYPASS_MAP_ENCODED) != 0;
if (!(pre_trans ^ enc)) {
int rv = ap_proxy_trans_match(r, dconf->alias, dconf);
-if (DONE != rv) {
+if (rv != HTTP_CONTINUE) {
return rv;
}
}
@@ -1054,7 +1058,7 @@ static int proxy_trans(request_rec *r, int pre_tra
enc = (ent->flags & PROXYPASS_MAP_ENCODED) != 0;
if (!(pre_trans ^ enc)) {
int rv = ap_proxy_trans_match(r, ent, dconf);
-if (DONE != rv) {
+if (rv != HTTP_CONTINUE) {
return rv;
}
}
Re: svn commit: r1879080 - in /httpd/httpd/trunk/modules/proxy: mod_proxy.c mod_proxy.h
On Thu, Jun 25, 2020 at 1:27 PM Ruediger Pluem wrote:
>
> On 6/25/20 12:13 PM, Yann Ylavic wrote:
> > Index: modules/proxy/mod_proxy.c
> > ===
> > --- modules/proxy/mod_proxy.c (revision 1879145)
> > +++ modules/proxy/mod_proxy.c (working copy)
>
> > @@ -987,10 +991,10 @@ PROXY_DECLARE(int) ap_proxy_trans_match(request_re
> >"URI path '%s' matches proxy handler '%s'", r->uri,
> >found);
> >
> > -return OK;
> > +return servlet ? DONE : OK;
>
> Why setting it to DONE in the servlet case? Wouldn't that cause
> ap_process_request_internal to be left early?
No, ap_process_request_internal() would just skip r->uri decoding (we
are in pre_trans hook here, since mapping=servlet only happens there).
Anyway, it's an orthogonal change sorry, maybe we still want uri
decoding for directory/location walk in the servlet case, but since
this patch actually modifies r->uri (while other proxy mappings do
not), I thought it could be the final transformation (that's DONE
returned from pre_trans).
The only case where it matters is for encoded control/reserved chars
(unreserved are always decoded during initial normalization). So for
the authz/authn case you mentioned it means that the admin would have
to use/match the encoded form of the path (in ) when the
concerned path contains reserved chars.
Something like if the final app on Tomcat is
really called "admin;foo" (which I don't recommend!), but it kind of
make sense because with servlet normalization
would never match..
That's all theoretical and unlikely case, but that's where we are ;)
By the way, I think a more correct patch would be this one (attached),
whether it returns DONE or OK (I kept DONE for now, until we get
further on this lovely encoding discussion :)
The change is that r->uri is rewritten in-place so that
r->parsed_uri.path gets updated too.
Regards;
Yann.
Index: modules/proxy/mod_proxy.c
===
--- modules/proxy/mod_proxy.c (revision 1879145)
+++ modules/proxy/mod_proxy.c (working copy)
@@ -17,6 +17,7 @@
#include "mod_proxy.h"
#include "mod_core.h"
#include "apr_optional.h"
+#include "apr_strings.h"
#include "scoreboard.h"
#include "mod_status.h"
#include "proxy_util.h"
@@ -574,10 +575,11 @@ static int alias_match(const char *uri, const char
* Inspired by mod_jk's jk_servlet_normalize().
*/
static int alias_match_servlet(apr_pool_t *p,
- const char *uri,
+ const char **urip,
const char *alias)
{
char *map;
+const char *uri = *urip;
apr_array_header_t *stack;
int map_pos, uri_pos, alias_pos, first_pos;
int alias_depth = 0, depth;
@@ -759,6 +761,8 @@ static int alias_match_servlet(apr_pool_t *p,
if (alias[alias_pos - 1] != '/' && uri[uri_pos - 1] == '/') {
uri_pos--;
}
+
+*urip = map;
return uri_pos;
}
@@ -872,6 +876,7 @@ PROXY_DECLARE(int) ap_proxy_trans_match(request_re
int mismatch = 0;
unsigned int nocanon = ent->flags & PROXYPASS_NOCANON;
const char *use_uri = nocanon ? r->unparsed_uri : r->uri;
+const char *servlet_uri = NULL;
if (dconf && (dconf->interpolate_env == 1) && (ent->flags & PROXYPASS_INTERPOLATE)) {
fake = proxy_interpolate(r, ent->fake);
@@ -933,8 +938,9 @@ PROXY_DECLARE(int) ap_proxy_trans_match(request_re
}
else {
if ((ent->flags & PROXYPASS_MAP_SERVLET) == PROXYPASS_MAP_SERVLET) {
+servlet_uri = r->uri;
+len = alias_match_servlet(r->pool, _uri, fake);
nocanon = 0; /* ignored since servlet's normalization applies */
-len = alias_match_servlet(r->pool, r->uri, fake);
}
else {
len = alias_match(r->uri, fake);
@@ -969,7 +975,7 @@ PROXY_DECLARE(int) ap_proxy_trans_match(request_re
*/
int rc = proxy_run_check_trans(r, found + 6);
if (rc != OK && rc != DECLINED) {
-return DONE;
+return HTTP_CONTINUE;
}
r->filename = found;
@@ -983,14 +989,27 @@ PROXY_DECLARE(int) ap_proxy_trans_match(request_re
apr_table_setn(r->notes, "proxy-noquery", "1");
}
+if (servlet_uri) {
+ap_log_rerror(APLOG_MARK, APLOG_TRACE1, 0, r, APLOGNO()
+ "Servlet path '%s' (%s) matches proxy handler '%s'",
+ r->uri, servlet_uri, found);
+
+/* We change r->uri in-place so that r->parsed_uri.path gets
+ * updated too. Normalized servlet_uri is necessarily shorter
+ * than the original r->uri, so strcpy() is fine.
+ */
+AP_DEBUG_ASSERT(strlen(r->uri) >= strlen(servlet_uri));
+strcpy(r->uri, servlet_uri);
+return DONE;
+}
+
Re: svn commit: r1879080 - in /httpd/httpd/trunk/modules/proxy: mod_proxy.c mod_proxy.h
On 6/25/20 12:13 PM, Yann Ylavic wrote: > Index: modules/proxy/mod_proxy.c > === > --- modules/proxy/mod_proxy.c (revision 1879145) > +++ modules/proxy/mod_proxy.c (working copy) > @@ -987,10 +991,10 @@ PROXY_DECLARE(int) ap_proxy_trans_match(request_re >"URI path '%s' matches proxy handler '%s'", r->uri, >found); > > -return OK; > +return servlet ? DONE : OK; Why setting it to DONE in the servlet case? Wouldn't that cause ap_process_request_internal to be left early? > } > > -return DONE; > +return HTTP_CONTINUE; > } > > static int proxy_trans(request_rec *r, int pre_trans) Regards Rüdiger
