Bug 40150

Summary: Incorrect User/Role classnames are silently ignored.
Product: Tomcat 5 Reporter: Tom <vandenberget>
Component: CatalinaAssignee: Tomcat Developers Mailing List <dev>
Status: RESOLVED FIXED    
Severity: blocker CC: ate
Priority: P2    
Version: 5.5.17   
Target Milestone: ---   
Hardware: All   
OS: other   
Attachments: Proposed fixed version of JAASRealm.
Improved version of the patch
Patch of JAASRealm.java (in diff format)

Description Tom 2006-08-01 08:52:32 UTC
org.apache.catalina.realm.JAASRealm does not verify any of the class names that
are set through setRoleClassNames() and setUserClassNames().

If an incorrect class name (e.g. a typo) is configured in context.xml, this is
unnoticed by JAASRealm. The result is that during authentication, when the
subject's principals are checked against the configured class names, the
principals are not recognised, and therefore not added to the subject.

The fact an incorrect configured class name is currently not detected and logged
makes it very hard to find the source of the problem. It can be easily fixed by
checking the class names in the two methods mentioned above. The class must
exist, and it must implement java.security.Principal, which is currently not
enforced/checked by the code.
Comment 1 Tom 2006-08-01 09:00:04 UTC
Created attachment 18668 [details]
Proposed fixed version of JAASRealm.

This version of JAASRealm validates the class names for setUserClassNames and
setRoleClassNames. It verifies if the class exists, and if it implements
java.security.Principal. If not, it logs a message (severe), that allows users
to detect the incorrect class name.

It might even be better if it threw an exception.

I've also restructured the code to parse the comma-delimited class name string,
as it was rather inefficient. It uses a StringTokenizer now.
Comment 2 Tom 2006-08-01 11:28:27 UTC
Created attachment 18669 [details]
Improved version of the patch

No longer using StringTokenizer, but String.split, as StringTokenizer is
considered legacy. Thanks to Sameer Acharya.
Comment 3 Yoav Shapira 2006-12-26 05:24:27 UTC
This looks like a good idea to enhance.  However, please submit your patch in
diff  format rather than the whole file, that would make its review and
application much faster: http://www.apache.org/dev/contributors.html#patches
provides more details.
Comment 4 Tom 2006-12-27 00:54:31 UTC
Created attachment 19306 [details]
Patch of JAASRealm.java (in diff format)
Comment 5 Yoav Shapira 2007-03-25 14:42:32 UTC
Tom, thanks for providing this patch in diff form.  I've applied it to the
Tomcat 5.5 and 6.0 trunks, I really like it.  Sorry it took so long.
Comment 6 Ate Douma 2007-12-13 06:13:55 UTC
While this patch might have improved feedback when an incorrect classnames are
provided, it actually fully *breaks* JAASRealm usage when correct classnames are
provided but need to be accessed through a ContextClassLoader.

We at Apache Jetspeed-2 use the useContextClassLoader=true setting for hooking
up our own custom Principal classes as these are provided through the portal
application itself, not from a common/shared classloader.

Because the new parseClassNames only does a simple Class.forName() check this
now fails to validate our classnames for Tomcat 5.5.24 and later and thereby
breaking our JAAS based security :(

I suggest this to be solved by either:
- reverting the patch
- keep the current patch but *ignore* a ClassNotFoundException except for
logging that it happened
- run this method in the appropriate ContextClassLoader for the web app if possible

FYI: we have a Jetspeed JIRA issue opened on this bug with some additional
information: https://issues.apache.org/jira/browse/JS2-828

Hopefully this issue can be resolved quickly as right now we cannot run Jetspeed
on Tomcat >= 5.5.24

Regards, Ate
Comment 7 Yoav Shapira 2007-12-27 14:33:40 UTC
See http://issues.apache.org/bugzilla/show_bug.cgi?id=44084 for the Tomcat 6
version of this issue.  This has been fixed in the Tomcat SVN trunk, and should
be integrated into the next 5.5 and 6 releases.

Comment 8 Mark Thomas 2008-01-21 13:01:59 UTC
This has been fixed as per the comments in 44084.