|Summary:||Incorrect User/Role classnames are silently ignored.|
|Product:||Tomcat 5||Reporter:||Tom <vandenberget>|
|Component:||Catalina||Assignee:||Tomcat Developers Mailing List <dev>|
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.