You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by rj...@apache.org on 2011/10/23 18:19:59 UTC
svn commit: r1187916 - in /tomcat/jk/trunk: native/iis/jk_isapi_plugin.c
xdocs/miscellaneous/changelog.xml
Author: rjung
Date: Sun Oct 23 16:19:59 2011
New Revision: 1187916
URL: http://svn.apache.org/viewvc?rev=1187916&view=rev
Log:
BZ 51769: IIS: Allow URIs which contain "META-INF" or
"WEB-INF" as long as they are not path components of the URI.
I kept the old stristr function, because it
could be useful for something else.
Modified:
tomcat/jk/trunk/native/iis/jk_isapi_plugin.c
tomcat/jk/trunk/xdocs/miscellaneous/changelog.xml
Modified: tomcat/jk/trunk/native/iis/jk_isapi_plugin.c
URL: http://svn.apache.org/viewvc/tomcat/jk/trunk/native/iis/jk_isapi_plugin.c?rev=1187916&r1=1187915&r2=1187916&view=diff
==============================================================================
--- tomcat/jk/trunk/native/iis/jk_isapi_plugin.c (original)
+++ tomcat/jk/trunk/native/iis/jk_isapi_plugin.c Sun Oct 23 16:19:59 2011
@@ -842,12 +842,30 @@ static char *stristr(const char *s, cons
return ((char *)s);
}
+/*
+ * Find the first occurrence of path in uri tokenized by "/".
+ * The comparison is done case insensitive.
+ */
+static const char *find_path_in_uri(const char *uri, const char *path)
+{
+ size_t len = strlen(path);
+ while (uri = strchr(uri, '/')) {
+ uri++;
+ if (!strncmp(uri, path, len) &&
+ (*(uri + len) == '/' ||
+ strlen(uri) == len)) {
+ return uri;
+ }
+ }
+ return NULL;
+}
+
static int uri_is_web_inf(const char *uri)
{
- if (stristr(uri, "/web-inf")) {
+ if (find_path_in_uri(uri, "web-inf")) {
return JK_TRUE;
}
- if (stristr(uri, "/meta-inf")) {
+ if (find_path_in_uri(uri, "meta-inf")) {
return JK_TRUE;
}
Modified: tomcat/jk/trunk/xdocs/miscellaneous/changelog.xml
URL: http://svn.apache.org/viewvc/tomcat/jk/trunk/xdocs/miscellaneous/changelog.xml?rev=1187916&r1=1187915&r2=1187916&view=diff
==============================================================================
--- tomcat/jk/trunk/xdocs/miscellaneous/changelog.xml (original)
+++ tomcat/jk/trunk/xdocs/miscellaneous/changelog.xml Sun Oct 23 16:19:59 2011
@@ -45,6 +45,10 @@
<subsection name="Native">
<changelog>
<fix>
+ <bug>51769</bug>: IIS: Allow URIs which contain "META-INF" or
+ "WEB-INF" as long as they are not path components of the URI. (rjung)
+ </fix>
+ <fix>
<bug>52056</bug>: HTTPD: JK request log does not always log
correct response status. Fixed by refactoring JK request
log to use the standard request log hook. (rjung)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1187916 - in /tomcat/jk/trunk: native/iis/jk_isapi_plugin.c
xdocs/miscellaneous/changelog.xml
Posted by Rainer Jung <ra...@kippdata.de>.
On 24.10.2011 13:54, Konstantin Kolinko wrote:
> Old code used stristr.
> New code uses strncmp.
>
> If I understand correctly, the old one is case-insensitive, while the
> new one is case-sensitive.
Looks like I had tomatoes on my eyes.
Will fix.
Thanks!
Rainer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1187916 - in /tomcat/jk/trunk: native/iis/jk_isapi_plugin.c
xdocs/miscellaneous/changelog.xml
Posted by Konstantin Kolinko <kn...@gmail.com>.
Old code used stristr.
New code uses strncmp.
If I understand correctly, the old one is case-insensitive, while the
new one is case-sensitive.
Best regards,
Konstantin Kolinko
2011/10/23 <rj...@apache.org>:
> Author: rjung
> Date: Sun Oct 23 16:19:59 2011
> New Revision: 1187916
>
> URL: http://svn.apache.org/viewvc?rev=1187916&view=rev
> Log:
> BZ 51769: IIS: Allow URIs which contain "META-INF" or
> "WEB-INF" as long as they are not path components of the URI.
>
> I kept the old stristr function, because it
> could be useful for something else.
>
> Modified:
> tomcat/jk/trunk/native/iis/jk_isapi_plugin.c
> tomcat/jk/trunk/xdocs/miscellaneous/changelog.xml
>
> Modified: tomcat/jk/trunk/native/iis/jk_isapi_plugin.c
> URL: http://svn.apache.org/viewvc/tomcat/jk/trunk/native/iis/jk_isapi_plugin.c?rev=1187916&r1=1187915&r2=1187916&view=diff
> ==============================================================================
> --- tomcat/jk/trunk/native/iis/jk_isapi_plugin.c (original)
> +++ tomcat/jk/trunk/native/iis/jk_isapi_plugin.c Sun Oct 23 16:19:59 2011
> @@ -842,12 +842,30 @@ static char *stristr(const char *s, cons
> return ((char *)s);
> }
>
> +/*
> + * Find the first occurrence of path in uri tokenized by "/".
> + * The comparison is done case insensitive.
> + */
> +static const char *find_path_in_uri(const char *uri, const char *path)
> +{
> + size_t len = strlen(path);
> + while (uri = strchr(uri, '/')) {
> + uri++;
> + if (!strncmp(uri, path, len) &&
> + (*(uri + len) == '/' ||
> + strlen(uri) == len)) {
> + return uri;
> + }
> + }
> + return NULL;
> +}
> +
> static int uri_is_web_inf(const char *uri)
> {
> - if (stristr(uri, "/web-inf")) {
> + if (find_path_in_uri(uri, "web-inf")) {
> return JK_TRUE;
> }
> - if (stristr(uri, "/meta-inf")) {
> + if (find_path_in_uri(uri, "meta-inf")) {
> return JK_TRUE;
> }
>
>
> Modified: tomcat/jk/trunk/xdocs/miscellaneous/changelog.xml
> URL: http://svn.apache.org/viewvc/tomcat/jk/trunk/xdocs/miscellaneous/changelog.xml?rev=1187916&r1=1187915&r2=1187916&view=diff
> ==============================================================================
> --- tomcat/jk/trunk/xdocs/miscellaneous/changelog.xml (original)
> +++ tomcat/jk/trunk/xdocs/miscellaneous/changelog.xml Sun Oct 23 16:19:59 2011
> @@ -45,6 +45,10 @@
> <subsection name="Native">
> <changelog>
> <fix>
> + <bug>51769</bug>: IIS: Allow URIs which contain "META-INF" or
> + "WEB-INF" as long as they are not path components of the URI. (rjung)
> + </fix>
> + <fix>
> <bug>52056</bug>: HTTPD: JK request log does not always log
> correct response status. Fixed by refactoring JK request
> log to use the standard request log hook. (rjung)
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1187916 - in /tomcat/jk/trunk: native/iis/jk_isapi_plugin.c
xdocs/miscellaneous/changelog.xml
Posted by Rainer Jung <ra...@kippdata.de>.
On 25.10.2011 20:03, Christopher Schultz wrote:
> Rainer,
>
> On 10/23/2011 12:19 PM, rjung@apache.org wrote:
>> +/* + * Find the first occurrence of path in uri tokenized by
>> "/". + * The comparison is done case insensitive. + */ +static
>> const char *find_path_in_uri(const char *uri, const char *path)
>> +{ + size_t len = strlen(path); + while (uri = strchr(uri,
>> '/')) {
>
> I think "//" in a URL will cause this loop to exit early, possibly
> avoiding this security check.
Why should it? strchr() goes to all occurances of '/' and the loop
will exit at the first occurance which is followed by path and '/' or
'0'. Note the not so nice strcmp() convention that it returns 0 for
equality, so !strncmp() means equality.
>> + uri++; + if (!strncmp(uri, path, len) &&
>
> strncmp doesn't use case-insensitive compare: will this ever match
> if you use "web-inf" (as below)?
Konstantin already observed that. I fixed it yesterday in r1188226.
>> + (*(uri + len) == '/' || + strlen(uri) ==
>> len)) { + return uri; + } + } + return
>> NULL; +} + static int uri_is_web_inf(const char *uri) { - if
>> (stristr(uri, "/web-inf")) { + if (find_path_in_uri(uri,
>> "web-inf")) { return JK_TRUE;
>
> This will return JK_TRUE if "web-inf" occurs at any place in the
> path, not just at the context level. Is that a problem? I can
> imagine that a request for /context/foo/WEB-INF/something might be
> valid.
But that was the original intention. Anything below web-inf should be
restricted from access, independent of how deeply below it is. That
was the original behaviour. Don't want to change that.
Thanks for the review!
Regards,
Rainer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1187916 - in /tomcat/jk/trunk: native/iis/jk_isapi_plugin.c
xdocs/miscellaneous/changelog.xml
Posted by Konstantin Kolinko <kn...@gmail.com>.
2011/10/25 Christopher Schultz <ch...@christopherschultz.net>:
> All,
>
> On 10/25/2011 2:03 PM, Christopher Schultz wrote:
>> On 10/23/2011 12:19 PM, rjung@apache.org wrote:
>>>
>>> + if (!strncmp(uri, path, len) &&
>>
>> strncmp doesn't use case-insensitive compare: will this ever match if
>> you use "web-inf" (as below)?
>
> Duh just saw Konstantin's response. Apologies for the noise.
>
> I still think the // might be a problem, though.
> + while (uri = strchr(uri, '/')) {
If uri starts with '/' then strchr will return uri without advancing
the pointer. I see no problem with //.
Best regards,
Konstantin Kolinko
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1187916 - in /tomcat/jk/trunk: native/iis/jk_isapi_plugin.c
xdocs/miscellaneous/changelog.xml
Posted by Christopher Schultz <ch...@christopherschultz.net>.
All,
On 10/25/2011 2:03 PM, Christopher Schultz wrote:
> On 10/23/2011 12:19 PM, rjung@apache.org wrote:
>>
>> + if (!strncmp(uri, path, len) &&
>
> strncmp doesn't use case-insensitive compare: will this ever match if
> you use "web-inf" (as below)?
Duh just saw Konstantin's response. Apologies for the noise.
I still think the // might be a problem, though.
-chris
Re: svn commit: r1187916 - in /tomcat/jk/trunk: native/iis/jk_isapi_plugin.c
xdocs/miscellaneous/changelog.xml
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Rainer,
On 10/23/2011 12:19 PM, rjung@apache.org wrote:
> +/*
> + * Find the first occurrence of path in uri tokenized by "/".
> + * The comparison is done case insensitive.
> + */
> +static const char *find_path_in_uri(const char *uri, const char *path)
> +{
> + size_t len = strlen(path);
> + while (uri = strchr(uri, '/')) {
I think "//" in a URL will cause this loop to exit early, possibly
avoiding this security check.
> + uri++;
> + if (!strncmp(uri, path, len) &&
strncmp doesn't use case-insensitive compare: will this ever match if
you use "web-inf" (as below)?
> + (*(uri + len) == '/' ||
> + strlen(uri) == len)) {
> + return uri;
> + }
> + }
> + return NULL;
> +}
> +
> static int uri_is_web_inf(const char *uri)
> {
> - if (stristr(uri, "/web-inf")) {
> + if (find_path_in_uri(uri, "web-inf")) {
> return JK_TRUE;
This will return JK_TRUE if "web-inf" occurs at any place in the path,
not just at the context level. Is that a problem? I can imagine that a
request for /context/foo/WEB-INF/something might be valid.
-chris
Re: svn commit: r1187916 - in /tomcat/jk/trunk: native/iis/jk_isapi_plugin.c
xdocs/miscellaneous/changelog.xml
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Rainer,
On 10/25/2011 3:37 PM, Rainer Jung wrote:
> On 25.10.2011 20:07, Christopher Schultz wrote:
>> Rainer,
>>
>> On 10/23/2011 12:19 PM, rjung@apache.org wrote:
>>> +static const char *find_path_in_uri(const char *uri, const char
>>> *path) +{ + size_t len = strlen(path); + while (uri =
>>> strchr(uri, '/')) { + uri++; + if (!strncmp(uri,
>>> path, len) && + (*(uri + len) == '/' || +
>>> strlen(uri) == len)) { + return uri; + } +
>>> }
>>
>> Also, 'len' is never updated in the loop, so the call to strncmp
>> could potentially cause a SIGSEGV -- but only in the cases where
>> something truly nefarious is going on, anyway.
>
> Hmmm, I don't get that: path isn't changed, strncmp() will never
> compare beyond terminating '0', and uri+len must be inside uri if
> length of path is len, and uri and path coincide for len chars.
Yeah, I'm re-thinking my assertion: the code is probably safe.
On the other hand, why bother using strNcmp instead of just strcmp given
that you are trusting 'path' to be clean already. I guess there's no
reason NOT to use strNcmp when you have a choice.
> Of course *(uri+len) could be '0', but that's OK.
Also nevermind about the // : strchr returns a pointer, not an index. :(
-chris
Re: svn commit: r1187916 - in /tomcat/jk/trunk: native/iis/jk_isapi_plugin.c
xdocs/miscellaneous/changelog.xml
Posted by Rainer Jung <ra...@kippdata.de>.
On 25.10.2011 20:07, Christopher Schultz wrote:
> Rainer,
>
> On 10/23/2011 12:19 PM, rjung@apache.org wrote:
>> +static const char *find_path_in_uri(const char *uri, const char
>> *path) +{ + size_t len = strlen(path); + while (uri =
>> strchr(uri, '/')) { + uri++; + if (!strncmp(uri,
>> path, len) && + (*(uri + len) == '/' || +
>> strlen(uri) == len)) { + return uri; + } +
>> }
>
> Also, 'len' is never updated in the loop, so the call to strncmp
> could potentially cause a SIGSEGV -- but only in the cases where
> something truly nefarious is going on, anyway.
Hmmm, I don't get that: path isn't changed, strncmp() will never
compare beyond terminating '0', and uri+len must be inside uri if
length of path is len, and uri and path coincide for len chars. Of
course *(uri+len) could be '0', but that's OK.
Regards,
Rainer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1187916 - in /tomcat/jk/trunk: native/iis/jk_isapi_plugin.c
xdocs/miscellaneous/changelog.xml
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Rainer,
On 10/23/2011 12:19 PM, rjung@apache.org wrote:
> +static const char *find_path_in_uri(const char *uri, const char *path)
> +{
> + size_t len = strlen(path);
> + while (uri = strchr(uri, '/')) {
> + uri++;
> + if (!strncmp(uri, path, len) &&
> + (*(uri + len) == '/' ||
> + strlen(uri) == len)) {
> + return uri;
> + }
> + }
Also, 'len' is never updated in the loop, so the call to strncmp could
potentially cause a SIGSEGV -- but only in the cases where something
truly nefarious is going on, anyway.
-chris