WebappClassLoader and prohibited classes/packages

[Christopher Schultz <ch...@christopherschultz.net>]

All,

I went looking into WebappClassLoader's validateJarFile() and filter()
methods, and I noticed two things:

1. The error message for locating an illegal class being loaded from a
JAR file references servlet spec 2.3 section 9.7.2. The current
published version of the spec (3.0) is now section 10.7.2. Any
objections to changing the message to reflect the new spec version
supported by Tomcat and the new section number?

2. In spite of the above spec section, Tomcat only checks for
javax.servlet.Servlet and/or javax.el.Expression and then rejects the
entire JAR file. It doesn't look like classes such as java.lang.String
or javax.xml.parsers.SAXParser, etc. are prohibited. This seems to be a
spec violation.

3. If a JAR file is found to contain a prohibited class, the entire JAR
file is rejected. This behavior is not mandated by the servlet spec and
may be over-reaching.

I'm sure there are all kinds of practical reasons to only look for
certain classes, and not to prohibit replacement of things like JSTL,
etc. But it seems like this should be tightened-up a bit, even if
through configuration things can be relaxed to their current state.

Any thoughts?

-chris

Re: WebappClassLoader and prohibited classes/packages

[Christopher Schultz <ch...@christopherschultz.net>]

Martin,

On 10/24/13 4:07 AM, Martin Grigorov wrote:
> Hi,
> 
> 
> On Wed, Oct 23, 2013 at 6:38 PM, Christopher Schultz <
> chris@christopherschultz.net> wrote:
> 
>> All,
>>
>> I went looking into WebappClassLoader's validateJarFile() and filter()
>> methods, and I noticed two things:
>>
>> 1. The error message for locating an illegal class being loaded from a
>> JAR file references servlet spec 2.3 section 9.7.2. The current
>> published version of the spec (3.0) is now section 10.7.2. Any
>> objections to changing the message to reflect the new spec version
>> supported by Tomcat and the new section number?
>>
>>
> +1
> 
> 
>> 2. In spite of the above spec section, Tomcat only checks for
>> javax.servlet.Servlet and/or javax.el.Expression and then rejects the
>> entire JAR file. It doesn't look like classes such as java.lang.String
>> or javax.xml.parsers.SAXParser, etc. are prohibited. This seems to be a
>> spec violation.
>>
> 
> It is not allowed to create a custom class in java.* package.
> You can probably put java.lang.String in a custom jar, but it will be the
> same class as provided by the JDK. It could be a different version than the
> runtime JRE though ...
> So I agree that it should be rejected.
> 
> I'm not 100% certain but I think it is OK to create custom classes in
> javax.** package. So I think these should not be prohibited.
> 
> 
>>
>> 3. If a JAR file is found to contain a prohibited class, the entire JAR
>> file is rejected. This behavior is not mandated by the servlet spec and
>> may be over-reaching.
>>
> 
> Do you know what would be the performance effect of rejecting just the
> problematic class and check the rest of the classes in the JAR ?
> The current fail-fast behavior seems like a good optimization to me.

The current behavior is to loop-over the prohibited class list and call
zipFile.getZipEntry() to check if any of those files exist in the ZIP
file. I'm not sure exactly how getZipEntry works, but it may not exactly
be the fastest way to scan for multiple (potential) ZIP entries.

I have no performance data for any of this. I was thinking more about
spec-compliance and protecting the web application against its own
stupidity than anything else.

>> I'm sure there are all kinds of practical reasons to only look for
>> certain classes, and not to prohibit replacement of things like JSTL,
>> etc. But it seems like this should be tightened-up a bit, even if
>> through configuration things can be relaxed to their current state.
>>
>> Any thoughts?
>>
>> -chris
>>
>>
> 

Re: WebappClassLoader and prohibited classes/packages

[Martin Grigorov <mg...@apache.org>]

Hi,


On Wed, Oct 23, 2013 at 6:38 PM, Christopher Schultz <
chris@christopherschultz.net> wrote:

> All,
>
> I went looking into WebappClassLoader's validateJarFile() and filter()
> methods, and I noticed two things:
>
> 1. The error message for locating an illegal class being loaded from a
> JAR file references servlet spec 2.3 section 9.7.2. The current
> published version of the spec (3.0) is now section 10.7.2. Any
> objections to changing the message to reflect the new spec version
> supported by Tomcat and the new section number?
>
>
+1


> 2. In spite of the above spec section, Tomcat only checks for
> javax.servlet.Servlet and/or javax.el.Expression and then rejects the
> entire JAR file. It doesn't look like classes such as java.lang.String
> or javax.xml.parsers.SAXParser, etc. are prohibited. This seems to be a
> spec violation.
>

It is not allowed to create a custom class in java.* package.
You can probably put java.lang.String in a custom jar, but it will be the
same class as provided by the JDK. It could be a different version than the
runtime JRE though ...
So I agree that it should be rejected.

I'm not 100% certain but I think it is OK to create custom classes in
javax.** package. So I think these should not be prohibited.


>
> 3. If a JAR file is found to contain a prohibited class, the entire JAR
> file is rejected. This behavior is not mandated by the servlet spec and
> may be over-reaching.
>

Do you know what would be the performance effect of rejecting just the
problematic class and check the rest of the classes in the JAR ?
The current fail-fast behavior seems like a good optimization to me.


>
> I'm sure there are all kinds of practical reasons to only look for
> certain classes, and not to prohibit replacement of things like JSTL,
> etc. But it seems like this should be tightened-up a bit, even if
> through configuration things can be relaxed to their current state.
>
> Any thoughts?
>
> -chris
>
>