Re: svn commit: r1187916 - in /tomcat/jk/trunk: native/iis/jk_isapi_plugin.c xdocs/miscellaneous/changelog.xml
Rainer,
On 10/23/2011 12:19 PM, rj...@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
signature.asc
Description: OpenPGP digital signature
Re: svn commit: r1187916 - in /tomcat/jk/trunk: native/iis/jk_isapi_plugin.c xdocs/miscellaneous/changelog.xml
Rainer,
On 10/23/2011 12:19 PM, rj...@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
signature.asc
Description: OpenPGP digital signature
Re: svn commit: r1187916 - in /tomcat/jk/trunk: native/iis/jk_isapi_plugin.c xdocs/miscellaneous/changelog.xml
On 25.10.2011 20:07, Christopher Schultz wrote:
Rainer,
On 10/23/2011 12:19 PM, rj...@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-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: svn commit: r1187916 - in /tomcat/jk/trunk: native/iis/jk_isapi_plugin.c xdocs/miscellaneous/changelog.xml
On 25.10.2011 20:03, Christopher Schultz wrote:
Rainer,
On 10/23/2011 12:19 PM, rj...@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-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: svn commit: r1187916 - in /tomcat/jk/trunk: native/iis/jk_isapi_plugin.c xdocs/miscellaneous/changelog.xml
All, On 10/25/2011 2:03 PM, Christopher Schultz wrote: On 10/23/2011 12:19 PM, rj...@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 signature.asc Description: OpenPGP digital signature
Re: svn commit: r1187916 - in /tomcat/jk/trunk: native/iis/jk_isapi_plugin.c xdocs/miscellaneous/changelog.xml
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, rj...@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-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: svn commit: r1187916 - in /tomcat/jk/trunk: native/iis/jk_isapi_plugin.c xdocs/miscellaneous/changelog.xml
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, rj...@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
signature.asc
Description: OpenPGP digital signature
Re: svn commit: r1187916 - in /tomcat/jk/trunk: native/iis/jk_isapi_plugin.c xdocs/miscellaneous/changelog.xml
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=1187916view=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=1187916r1=1187915r2=1187916view=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=1187916r1=1187915r2=1187916view=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
+ bug51769/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
bug52056/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-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: svn commit: r1187916 - in /tomcat/jk/trunk: native/iis/jk_isapi_plugin.c xdocs/miscellaneous/changelog.xml
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-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
