users@jersey.java.net

Thread safety of XML-based factories <was> Re: [Jersey] Fixed <was> Re: [Jersey] Jersey vulnerable to XXE attack?

From: Paul Sandoz <Paul.Sandoz_at_Sun.COM>
Date: Mon, 13 Jul 2009 10:50:56 +0200

Hi,

Anyone know if SAXParserFactory, DocumentBuilderFactory and
XMLInputFactory are thread safe?

The code currently assumes they are, but i am not sure i can assume
that.

Paul.

On Jul 11, 2009, at 4:55 PM, Paul Sandoz wrote:

> Hi Tim et. al.,
>
> I have fixed this in the trunk for SAX, DOM and StAX processing.
> Links are below to the code for the configuration settings, in case
> you want to double check the settings.
>
> The approach i took was to enable injection of SAXParserFactory,
> DocumentBuilderFactory and XMLInputFactory instances of which are
> configured appropriately by default to stop XXE attacks.
>
> Thus a developer can also get these injected by doing say:
>
> @Context SAXParserFactory spf
>
> The feature FeaturesAndProperties.FEATURE_DISABLE_XML_SECURITY has
> been added so the default, secure, behavior can be disabled.
>
> Paul.
>
> SAX:
>
> http://fisheye4.atlassian.com/browse/jersey/trunk/jersey/jersey-core/src/main/java/com/sun/jersey/core/impl/provider/xml/SAXParserContextProvider.java?r=2486
>
>
> DOM:
>
> http://fisheye4.atlassian.com/browse/jersey/trunk/jersey/jersey-core/src/main/java/com/sun/jersey/core/impl/provider/xml/DocumentBuilderFactoryProvider.java?r=2486
>
>
> StAX:
>
> http://fisheye4.atlassian.com/browse/jersey/trunk/jersey/jersey-core/src/main/java/com/sun/jersey/core/impl/provider/xml/XMLStreamReaderContextProvider.java?r=2486
>
>
> On Jul 9, 2009, at 11:18 PM, Tim McCune wrote:
>
>> On Thu, Jul 9, 2009 at 2:03 PM, Tatu Saloranta
>> <tsaloranta_at_gmail.com> wrote:
>> Also: similar option exists with Stax API,
>> XMLInputFactory.IS_REPLACING_ENTITY_REFERENCES;
>> factory.setProperty(XMLInputFactory.IS_REPLACING_ENTITY_REFERENCES.
>> Boolean.FALSE);
>> would do the trick.
>>
>> Given that you don't necessarily get to choose which parser gets
>> instantiated by Jersey, it would make sense to use a more generic
>> setting which can then translate to what is needed by specific parser
>> being instantiated?
>> Plus it definitely would be reasonable to set this when "secure mode"
>> is enabled.
>>
>> I looked into this, and it turns out the StAX parser that's built
>> into the JDK (SJSXP) is not standards-compliant, and will not only
>> default to not expanding entities, but will throw an exception if
>> you set that property to Boolean.TRUE. :) So while not an
>> immediate issue, I suppose it could become one down the road if the
>> JDK's implementation fixed that problem, and if Jersey were to
>> change to using StAX for parsing XML by default instead of SAX.
>>
>> I'm much more concerned about the immediate vulnerability though.
>