I can reproduce that vulnerability which leverage "org.apache.naming.factory.BeanFactory" and "javax.el.ELProcessor" described in https://www.veracode.com/blog/research/exploiting-jndi-injections-java It would be great if tomcat can do something to mitigate it.
Can we drop "forceString" supports? https://github.com/apache/tomcat/blob/f5a732e74e2a36442b2bf562c665917c4bb1167a/java/org/apache/naming/factory/BeanFactory.java#L150
To be crystal clear: There is no Apache Tomcat vulnerability here. To quote from the linked article: <quote> The actual problem here is not within the JDK or Apache Tomcat library, but rather in custom applications that pass user-controllable data to the "InitialContext.lookup()" function, as it still represents a security risk even in fully patched JDK installations. </quote> Moving this to an enhancement request. It is highly unlikely Tomcat will remove/disable existing functionality. Suggestions for mitigation / hardening that can improve security without impacting legitimate uses will be welcomed.
Honestly, any "feature" that significantly reduces security should be difficult to enable. My initial reaction after reading that piece was "why is forceString enabled by default?" I don't know the history of that feature, so I'm not sure how popular it is or what the use-cases are. My guess is that, mostly, there are simple uses of JNDI in Tomcat. For more "exotic" use-cases, it shouldn't be too much trouble for an admin to enable this feature explicitly. It's also not clear to me how much *more* secure things are /without/ "forceString" available. JNDI lookups are, by definition, fairly sensitive things: if you allow users to control the lookups, they can kind of ... well, look-up ANYTHING.
The feature was added by Rainer in Jan 2015. The idea of the bean factory is to avoid having to use custom object factories (personally: I think using custom object factories is usually better), and this forceString increased flexibility further. Normally, once you get to the point where you can configure the JNDI environment of Tomcat, you can also configure everything else, so limiting forceString shouldn't add any extra safety.
I agree that "forceString" should be disabled by default and removed in future version, It will increase safety, "you can configure the JNDI environment of Tomcat" is more harder since it need another gadget, let's remove "org.apache.naming.factory.BeanFactory" from existing gadgets.
The history of forceString (thanks Remy) can be seen in the log message of svn r1655312 or github d1cf73ab16da6fccde3c323e16b582be8d579008. I paste it here. I am totally open to drop it, if it now turns out to pose security risks. Enhance our naming BeanFactory. If a bean property exists which the Introspector presents us with a type that we don't have a string conversion for, but the bean actually has a method to set the property from a string, allow to provide this information to the BeanFactory. New attribute "forceString" taking a comma separated list of items as values. Each item is either a bean property name (e.g. "foo") meaning that there is a setter function "setFoo(String)" for that property. Or the item is of the form "foo=method" meaning that property "foo" can be set by calling "method(String)". This should make writing a custom bean factory obsolete in quite a few cases. Concrete use case was tibco TibjmsConnectionFactory which has an attribute SSLIdentity detected by Introspector as byte[] but which can be set by setSSLIdentity(String). Existing BeanFactory throws NamingException. Regards, Rainer
Looking at this in a bit more detail I have a couple of observations/questions: 1. Has anyone got a suggestion to make enabling forceString support configurable that doesn't involve a system property? 2. Is removing this feature entirely in 10.1.x reasonable? 3. Why doesn't Introspector find the "setSSLIdentity(String)" method. It looks like it should. Is improving the method matching in Introspector an approach that could work long term? The answer to the first part of 3 may need some research. That is on my TODO list but I'm unlikely to get to it before January.
(In reply to Mark Thomas from comment #7) > Looking at this in a bit more detail I have a couple of > observations/questions: > > 1. Has anyone got a suggestion to make enabling forceString support > configurable that doesn't involve a system property? Nope. > 2. Is removing this feature entirely in 10.1.x reasonable? I think it's fine. Overall JNDI should use real object factories. > 3. Why doesn't Introspector find the "setSSLIdentity(String)" method. It > looks like it should. Is improving the method matching in Introspector an > approach that could work long term? No idea. But the BeanFactory doesn't use our IntrospectionUtils, as you just said, and we're totally used to its very user friendly behavior. > The answer to the first part of 3 may need some research. That is on my TODO > list but I'm unlikely to get to it before January.
(In reply to Mark Thomas from comment #7) > 1. Has anyone got a suggestion to make enabling forceString support > configurable that doesn't involve a system property? JNDI environment variable? (lol just kidding). I think this is either a system property (preferable to me, even though system properties kinda suck) or an otherwise unnecessary global Listener. > 2. Is removing this feature entirely in 10.1.x reasonable? +1 to reasoning provided by remm and rjung (via email)
(In reply to Remy Maucherat from comment #8) > No idea. But the BeanFactory doesn't use our IntrospectionUtils, as you just > said, and we're totally used to its very user friendly behavior. Doh! Of course. As a Bean factory it is following the Bean spec. I think we might be able to do something along the lines of if the setter method doesn't use String and we can't coerce it, is there a method identical to the setter part from it uses String? If so, use that. Is that worth implementing?
I've implemented this alternative approach for 10.1.x. It isn't as generic as forceString but it is sufficient to meet the original requirement. Two questions: 1. Should we back-port this? If so, how far? 2. Do we want to expand conversion so if the setter is for Type T that we can't convert and T has a constructor T(String) we use that constructor to create an instance of T and then pass that to the setter?
> 1. Should we back-port this? If so, how far? Yes, back to 8.x. > 2. Do we want to expand conversion so if the setter is for Type T that we can't convert and T has a constructor T(String) we use that constructor to create an instance of T and then pass that to the setter? I think we should keep it as simple as possible, BeanFactory is not widely used AFAIK.
Fixed in: - 10.1.x for 10.1.0-M14 onwards - 10.0.x for 10.0.21 onwards - 9.0.x for 9.0.63 onwards - 8.5.x for 8.5.79 onwards