XML 2.6.2 security fix and Xerces dependency

[Robert Marcano]

Greetings. I am always very picky about adding new dependencies,
specially XML parsers. I have been in situations when two libraries I
need need two different XML parser implementations.

This time I understand that the external Xerces parser is being used in
order to guarantee that some features are supported and to be sure the
parser is properly secured, correct me if I am mistaken. The embedded
Oracle and OpenJDK JVM use a copy of Xerces, and I know this is not
guaranteed to be always true, another JVM can use another parser (IBM?),
the default parser could change, or there is another default installed
on the class path. JAXP already provide a way to know if the parser
support a feature. Why not try to use the Java default parser and if
there is no support for those features, try to use Xerces. Something like:

> trait SecurityHelpers {
> import SecurityHelpers._
>
> def secureXML: XMLLoader[Elem] = {
> val parserFactory =
> if (useDefaultParser) {
> newDefaultFactory()
> } else {
> val f = SAXParserFactory.newInstance(
> "org.apache.xerces.jaxp.SAXParserFactoryImpl",
> SecurityHelpers.getClass.getClassLoader)
> setupFactory(f)
> }
>
> val saxParser = parserFactory.newSAXParser();
> XML.withSAXParser(saxParser)
> }
> }
>
> object SecurityHelpers extends SecurityHelpers {
> private lazy val useDefaultParser: Boolean = {
> try {
> // used only for testing feature compatibility
> newDefaultFactory()
> true
> } catch {
> case e: SAXNotRecognizedException =>
> // TODO Log default JVM parser is not enough
> false
> }
> }
>
> private def newDefaultFactory() = setupFactory(SAXParserFactory.newInstance())
>
> private def setupFactory(parserFactory: SAXParserFactory) = {
> parserFactory.setNamespaceAware(false)
> parserFactory.setFeature("http://xml.org/sax/features/external-general-entities", false);
> parserFactory.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
> parserFactory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true)
> parserFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true)
> parserFactory
> }
> }

this way, people like me that run on a JVM that support those features,
can avoid shipping another parser, and allows us to be sure we use the
same parser for everything. In case someone run Lift on a JVM that don't
support those features, they can ship Xerces too, and problem solved. It
will not be possible to run Lift with a parser that doesn't understand
those features, because a SAXNotRecognizedException will be thrown.

[Robert Marcano]

Another implementation can try Xerces first and if it is not found try
the default parser. If the default parser is not found, it will still
check the features are supported, any compliant parser must throw an
exception if the feature is not supported. For example:

> trait SecurityHelpers {
> import SecurityHelpers._
>
> def secureXML: XMLLoader[Elem] = {
> val parserFactory =

> if (useXerces) {
> val f = SAXParserFactory.newInstance(
> XercesImpl,
> SecurityHelpers.getClass.getClassLoader)
> setupFactory(f)
> } else {
> setupFactory(SAXParserFactory.newInstance())
> }
>
> val saxParser = parserFactory.newSAXParser();
> println(saxParser)
> XML.withSAXParser(saxParser)

> }
>
> private def setupFactory(parserFactory: SAXParserFactory) = {
> parserFactory.setNamespaceAware(false)
> parserFactory.setFeature("http://xml.org/sax/features/external-general-entities", false);
> parserFactory.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
> parserFactory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true)
> parserFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true)
> parserFactory
> }
> }
>

> object SecurityHelpers extends SecurityHelpers {
> private final val XercesImpl = "org.apache.xerces.jaxp.SAXParserFactoryImpl"
>
> private lazy val useXerces = try {
> Class.forName(
> XercesImpl,
> true,
> SecurityHelpers.getClass.getClassLoader)
> true
> } catch {
> case e: ClassNotFoundException =>
> false
> }
> }

[Antonio Salazar Cardozo]

Hmm… Is this a solution to a concrete problem you're having, or a suggestion to

prevent a potential problem that someone may have?

Thanks,

Antonio

[Robert Marcano]

On 04/15/2015 01:19 PM, Antonio Salazar Cardozo wrote:
> Hmm… Is this a solution to a concrete problem you're having, or a
> suggestion to
> prevent a potential problem that someone may have?
> Thanks,

Some people may not call it a problem, but I patched my Lift in order to
not require Xerces, because I want only one XML parser implementation on
all my codebase, I use 4 tools that depend on XML files and I am happy
with not need to use one parser for each one. My proposals still provide
the security fixes the Lift team added.

> --
> --
> Lift, the simply functional web framework: http://liftweb.net
> Code: http://github.com/lift
> Discussion: http://groups.google.com/group/liftweb
> Stuck? Help us help you:
> https://www.assembla.com/wiki/show/liftweb/Posting_example_code
>
> ---
> You received this message because you are subscribed to the Google
> Groups "Lift" group.
> To unsubscribe from this group and stop receiving emails from it, send
> an email to liftweb+u...@googlegroups.com
> <mailto:liftweb+u...@googlegroups.com>.
> For more options, visit https://groups.google.com/d/optout.

[Antonio Salazar Cardozo]

Trying Xerces first and falling back to the default parser is a great way to not

get the fixes we provide, since we're using Xerces features that aren't supported

in the default parser, as far as I can tell (in particular, disallowing doctype declarations

seems to be Xerces-specific).

Are you forced to use the Xerces parser if you don't use the specific SAXParserFactory

that Xerces provides?

Pending more data (i.e., others chiming in that this is a problem for them), I'm tending

towards not adding this to our already-long list of todos. I am playing with the idea of

providing a LiftRule that allows changing the XML parser that's used by lift-webkit, though.

I also need to check a couple of things related to whether we're actually relying on Xerces-

specific features. If we aren't, then we'll definitely throw the Xerces dependency out

the window.

Thanks,

Antonio

> an email to liftweb+unsubscribe@googlegroups.com
> <mailto:liftweb+unsubscribe@googlegroups.com>.

[Robert Marcano]

On 04/16/2015 04:07 PM, Antonio Salazar Cardozo wrote:
> Trying Xerces first and falling back to the default parser is a great

> way to /not/

> get the fixes we provide, since we're using Xerces features that aren't
> supported
> in the default parser, as far as I can tell (in particular, disallowing
> doctype declarations
> seems to be Xerces-specific).

You missed the line where I said that when a JAXP parser doesn't
implements a feature a SAXNotRecognizedException is thrown, so if
someone runs Lift without Xerces and on a JVM that doesn't understand
"http://apache.org/xml/features/disallow-doctype-decl,
SAXNotRecognizedException means Lift will fail hard if the security will
be compromised. Oracle JVM and OpenJDK understand that feature (based on
Xerces).

>
> Are you forced to use the Xerces parser if you don't use the specific
> SAXParserFactory
> that Xerces provides?

Yes, by default, because the Xerces jar includes
META-INF/services/javax.xml.parsers.SAXParserFactory. There are ways to
avoid Xerces with the default factory builder method, removing that file
from the jar or to use javax.xml.parsers.SAXParserFactory system
property pointing it again to the JVM default factory.

By the way. a little note: Read
https://jaxp.java.net/docs/spec/html/#plugabililty-thread-safety ,
SAXParserFactory.newSAXParser() must be thread safe, so lazy
initializing a global SAXParserFactory (lazy ensures thread safety when
initializing the factory), avoid creating one every time.

> > an email to liftweb+u...@googlegroups.com
> <mailto:liftweb%2Bunsu...@googlegroups.com>
> > <mailto:liftweb+u...@googlegroups.com
> <mailto:liftweb%2Bunsu...@googlegroups.com>>.

> > For more options, visit https://groups.google.com/d/optout

> <https://groups.google.com/d/optout>.

>
> --
> --
> Lift, the simply functional web framework: http://liftweb.net
> Code: http://github.com/lift
> Discussion: http://groups.google.com/group/liftweb
> Stuck? Help us help you:
> https://www.assembla.com/wiki/show/liftweb/Posting_example_code
>
> ---
> You received this message because you are subscribed to the Google
> Groups "Lift" group.
> To unsubscribe from this group and stop receiving emails from it, send

> an email to liftweb+u...@googlegroups.com
> <mailto:liftweb+u...@googlegroups.com>.

[Antonio Salazar Cardozo]

I see what you're saying. This still seems like a complicated solution to a problem

whose extent is unclear to me. I'll let someone with a clearer understanding of the

impact step in, or wait for more complaints on the issue or for me to get a chance to

get a deeper understanding before doing anything I don't follow super-clearly.

Thanks,

Antonio

>      > an email to liftweb+unsubscribe@googlegroups.com
>     <mailto:liftweb%2Bunsubscribe@googlegroups.com>
>      > <mailto:liftweb+unsubscribe@googlegroups.com
>     <mailto:liftweb%2Bunsubscribe@googlegroups.com>>.

>      > For more options, visit https://groups.google.com/d/optout
>     <https://groups.google.com/d/optout>.
>
> --
> --
> Lift, the simply functional web framework: http://liftweb.net
> Code: http://github.com/lift
> Discussion: http://groups.google.com/group/liftweb
> Stuck? Help us help you:
> https://www.assembla.com/wiki/show/liftweb/Posting_example_code
>
> ---
> You received this message because you are subscribed to the Google
> Groups "Lift" group.
> To unsubscribe from this group and stop receiving emails from it, send

> an email to liftweb+unsubscribe@googlegroups.com
> <mailto:liftweb+unsubscribe@googlegroups.com>.

[Robert Marcano]

On 04/17/2015 01:55 PM, Antonio Salazar Cardozo wrote:
> I see what you're saying. This still seems like a complicated solution
> to a problem
> whose extent is unclear to me. I'll let someone with a clearer
> understanding of the
> impact step in, or wait for more complaints on the issue or for me to
> get a chance to
> get a deeper understanding before doing anything I don't follow
> super-clearly.

Sure, to wait for more people to chime in is good. One more thing.

This is a security update, adding a new dependency that changes by
default the parser to be used on every code that use the default JAXP
parser (even code outside Lift), as I said on the previous email, just
adding the jar change the default for the entire application. I am not
having bugs by using another parser, my tests pass perfectly with
Xerces, but having a security update for a library that change the
defaults for other parts of you applications is not right. It probably
can be fine for a final 3.0.

I already patched my copy of Lift to use the JVM parser (that will get
updated and fixed by each automatic JVM update by the OS), if needed I
will patch it indefinitely :(

I rest my case here :), waiting for other opinions

Note: I decided to go in my patch for the second implementation and
using a single factory instance.

> Thanks,
> Antonio
>
> On Thursday, April 16, 2015 at 8:53:27 PM UTC-4, Robert Marcano wrote:
>
> On 04/16/2015 04:07 PM, Antonio Salazar Cardozo wrote:
> > Trying Xerces first and falling back to the default parser is a
> great
> > way to /not/
> > get the fixes we provide, since we're using Xerces features that
> aren't
> > supported
> > in the default parser, as far as I can tell (in particular,
> disallowing
> > doctype declarations
> > seems to be Xerces-specific).
>
> You missed the line where I said that when a JAXP parser doesn't
> implements a feature a SAXNotRecognizedException is thrown, so if
> someone runs Lift without Xerces and on a JVM that doesn't understand
> "http://apache.org/xml/features/disallow-doctype-decl

> <http://apache.org/xml/features/disallow-doctype-decl>,