Added JAXP secure processing features and properties.

[ronsigal]

See attached secure-configuration-guide.pdf.

[ronsigal]

secure-configuration-guide.pdf

[ronsigal]

This Pull Request replaces #11.

[ctomc]

@wolfc @dpospisil an you guys check this, as we do have some downstream changes wrt to this.

[scottmarlow]

I'm pushing a snapshot build of this change for TCK testing

[scottmarlow]

Any reason why scope is commented out? I tried to build wildfly-core with this change but got a warning about this change introducing a transitive dependency on junit:

[WARNING] Rule 0: org.apache.maven.plugins.enforcer.BanTransitiveDependencies failed with message:
org.wildfly.core:wildfly-core-feature-pack:pom:2.0.6.Final-SNAPSHOT
xerces:xercesImpl:jar:2.11.0.SP5-SNAPSHOT:compile has transitive dependencies:
junit:junit:jar:4.12:test
org.hamcrest:hamcrest-core:jar:1.3:test

[scottmarlow]

To continue with my tck testing, I pushed a snapshot build of this xerces change and wildfly-core. I hacked wildfly-core to ignore the junit dependency but still think this should be addressed.

[ronsigal]

Hmmm, I don't remember why I commented out the scope. It might be because xerces didn't have a src/test/java directory until I added one. Yeah, maybe I originally put my tests in their tests directory. But I just built xerces and ran my tests with test and it went OK. I'll change that.

[scottmarlow]

WF org.jboss.as.test.integration.jsf.beanvalidation.cdi.BeanValidationCdiIntegrationTestCase seemed to fail (three times) with the Xerces upgrade. To reproduce "/build.sh clean install -Denforcer.skip=true -Dversion.org.wildfly.core=2.0.6.Final-SNAPSHOT -DskipTests=true". If a new wildfly-core snapshot is pushed without the Xerces upgrade, it won't reproduce (snapshots are unreliable like that). server.log output is at http://pastebin.com/vGbEqCaK

[scottmarlow]

Ron, not sure if you reproduced the beanvalidation failure yet? After you build WildFly with the above "build.sh" command line, you need to change into wildfly/testsuite/integration/basic and run "mvn clean install -Dtest=org.jboss.as.test.integration.jsf.beanvalidation.cdi.BeanValidationCdiIntegrationTestCase

[scottmarlow]

We need to run the TCK again also, the last run didn't include this change. Perhaps we could run the tck after fixing the BeanValidationCdiIntegrationTestCase failure. Let me know when your ready.

[ronsigal]

Scott, before I submitted the PR I ran the entire testsuite and it passed. I just ran the basic integration testsuite and everything, including BeanValidationCdiIntegrationTestCase, passed.

I'm using

<module xmlns="urn:jboss:module:1.3" name="org.apache.xerces">
<resources>
<!--artifact name="xerces:xercesImpl:2.11.0.SP4"/-->
<artifact name="xerces:xercesImpl:2.11.0.SP5-SNAPSHOT"/>
</resources>
<dependencies>
<module name="javax.api"/>
</dependencies>
</module>

Could I be doing something wrong?

It looks like my copy of 10.0.0.CR6-redhat-SNAPSHOT is out of date. I'll update it and try again.

[wolfc]

As a side noted you should never be using SNAPSHOT in CI builds, always refer to the timed snapshot (for example 2.0.6.Final-20160113.202416-3 although I can't find that core version on nexus anymore).

[scottmarlow]

Thanks for the tip Carlo, I'll try that next time. Ron, I was running BeanValidationCdiIntegrationTestCase against WildFly master.

[scottmarlow]

Not sure if it matters but I'm on Fedora 23 (4.2.8-300.fc23.x86_64)

[ronsigal]

I downloaded branch 7.x of https://github.com/jbossas/jboss-eap7 last night and got

[INFO] WildFly Test Suite: Aggregator ..................... SUCCESS [ 23.248 s]
[INFO] WildFly Test Suite: Integration .................... SUCCESS [ 0.969 s]
[INFO] WildFly Test Suite: Integration - WS ............... SUCCESS [ 39.463 s]
[INFO] WildFly Test Suite: Integration - Web .............. SUCCESS [ 50.630 s]
[INFO] WildFly Test Suite: Integration - Smoke ............ SUCCESS [ 41.237 s]
[INFO] WildFly Test Suite: Integration - Basic ............ SUCCESS [10:28 min]
[INFO] WildFly Test Suite: Integration - IIOP ............. SUCCESS [ 21.862 s]
[INFO] WildFly Test Suite: Integration - XTS .............. SUCCESS [01:22 min]
[INFO] WildFly Test Suite: Integration - RTS .............. SUCCESS [ 13.325 s]
[INFO] WildFly Test Suite: Integration - Role Based Access Control SUCCESS [ 6.472 s]
[INFO] WildFly Test Suite: Integration - Clustering ....... FAILURE [06:10 min]

The clustering failure was

test(org.jboss.as.test.clustering.cluster.singleton.SingletonDeploymentJBossAllTestCase) Time elapsed: 30.526 sec <<< ERROR!
java.lang.NullPointerException: null at org.jboss.as.arquillian.container.ServerSetupObserver.handleAfterUndeploy(ServerSetupObserver.java:161)

which I've seen before and doesn't seem to be related to xerces. Anyway, BeanValidationCdiIntegrationTestCase passed. This is also on Fedora 23, by the way.

[ronsigal]

Scott, here's my latest attempt to duplicate your test situation:

git clone https://github.com/wildfly/wildfly -b master
build.sh clean install -Denforcer.skip=true -Dversion.org.wildfly.core=2.0.6.Final -DskipTests=true
cd testsuite
mvn test -DallTests

and I got

Tests in error:
SingletonDeploymentJBossAllTestCase>ClusterAbstractTestCase.afterTestMethod:101 » NullPointer

Tests run: 51, Failures: 0, Errors: 1, Skipped: 3

[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary:
[INFO]
[INFO] WildFly Test Suite: Aggregator ..................... SUCCESS [ 22.219 s]
[INFO] WildFly Test Suite: Integration .................... SUCCESS [ 1.393 s]
[INFO] WildFly Test Suite: Integration - WS ............... SUCCESS [ 42.490 s]
[INFO] WildFly Test Suite: Integration - Web .............. SUCCESS [ 51.904 s]
[INFO] WildFly Test Suite: Integration - Smoke ............ SUCCESS [ 40.927 s]
[INFO] WildFly Test Suite: Integration - Basic ............ SUCCESS [10:26 min]
[INFO] WildFly Test Suite: Integration - IIOP ............. SUCCESS [ 20.964 s]
[INFO] WildFly Test Suite: Integration - XTS .............. SUCCESS [01:19 min]
[INFO] WildFly Test Suite: Integration - RTS .............. SUCCESS [ 13.289 s]
[INFO] WildFly Test Suite: Integration - Role Based Access Control SUCCESS [ 6.316 s]
[INFO] WildFly Test Suite: Integration - Clustering ....... FAILURE [06:09 min]

with old and new xerces. Strange.

[ronsigal]

Scott, your morning email nailed it - I had to build wildfly-core. So, the problem is in com.sun.faces.facelets.compiler.SAXCompiler in jsf-impl-2.2.12-jbossorg-2, which needs

factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", false);

I saw that problem when I was testing with wildfly 8.2.1.Final, and I thought I was able to build that version of jsf-impl, but I can't seem to build jsf-impl-2.2.12-jbossorg-2. I've downloaded the pom.xml from Nexus, but it seems to be missing some dependencies. Do you know who would be responsible for that jar?

[scottmarlow]

Ron, Farah Juma can probably help with JSF. I wonder how the jsf-impl would know whether to call factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", false) or not? If secure processing is enabled, should that prevent the failing BeanValidationCdiIntegrationTestCase test from passing? Is there a way that the BeanValidationCdiIntegrationTestCase could disable secure processing? Do we purposely avoid allowing a system property disable secure processing because that would be easy to change when we are not running with a Java security manager? As you said in your documentation, applications may need additional configuration to control secure processing.

[wolfc]

Which document has the DOCTYPE declaration?
If we allow it via JSF, then we might re-introduce the vulnerability.

[scottmarlow]

https://github.com/wildfly/wildfly/blob/master/testsuite/integration/basic/src/test/java/org/jboss/as/test/integration/jsf/beanvalidation/cdi/register.xhtml has the DOCTYPE.

Error from http://pastebin.com/vGbEqCaK is below:

"
Error Parsing /register.xhtml: Error Traced[line: 1] DOCTYPE is disallowed when the feature "http://apache.org/xml/features/disallow-doctype-decl" set to true.
"

[fjuma]

I don't think factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", false) should be added to com.sun.faces.facelets.compiler.SAXCompiler in jsf-impl as this could re-introduce issues. It might be good if WildFly provided a setting to enable/disable secure processing.

To fix BeanValidationCdiIntegrationTestCase, we could remove the DOCTYPE declaration from register.xhtml. Since JSF 2.2, omitting the DOCTYPE declaration seems to be recommended since by default, JSF 2.2 will render the HTML5 DOCTYPE (i.e., ) regardless of what is set in the source xhtml file (see https://jsflive.wordpress.com/2013/08/08/jsf22-html5).

[ronsigal]

Aaarrgghh. I don't know why, but I can't get the test to fail today.

re: "Is there a way that the BeanValidationCdiIntegrationTestCase could disable secure processing?"

Not directly, but there is a system property, "jdk.xml.resolveExternalEntities", which defaults to false when the SECURE_PROCESSING_FEATURE is true. It turns off, or on, several other features and properties related to external files, including "http://apache.org/xml/features/disallow-doctype-decl". I'm trying to test it in BeanValidationCdiIntegrationTestCase, but, as I said, I can't get it to fail now.

The consensus seems to be that it would be better to fix the test rather than change jsf-impl, and setting "jdk.xml.resolveExternalEntities" in the test should do that. I have a question, though. Is reading xml files with DTDs something that jsf-impl would do in normal circumstances. If so, then there's a decision to be made, whether to allow DTDs or not.

[ronsigal]

re: "Do we purposely avoid allowing a system property disable secure processing because that would be easy to change when we are not running with a Java security manager?"

I've been working from the JDK version of xerces, in which properties can be set by either system properties or by passing values to parsers or parser factories. Features (basically, boolean valued properties) like SECURE_PROCESSING_FEATURE do not have system properties. "jdk.xml.resolveExternalEntities" is a special case which is found in Apache xerces but not JDK xerces.

[fjuma]

re: "Is reading xml files with DTDs something that jsf-impl would do in normal circumstances."

JSF pages are usually written in XHTML. With xercesImpl-2.11.0.SP5-SNAPSHOT.jar in WildFly, attempting to deploy and then access any JSF application that includes a DOCTYPE declaration in an .xhtml page will result in the exception Scott mentioned. Although the DOCTYPE declaration can be omitted from the .xhtml page in a JSF app, I'm not sure how often users do this.

[ronsigal]

Well, our conversation motivated me to go back and look at the sources I read in the course of doing this work. My goal was to replicate the security behavior in JDK xerces, but I see that I've gone somewhat further. The JAXP specification is rather loose in its prescription for security behavior. Basically, it says that setting SECURE_PROCESSING_FEATURE to true "instructs the implementation to process XML securely. This may set limits on XML constructs to avoid conditions such as denial of service attacks." The specific restrictions are implementation specific.

What I have done is to use SECURE_PROCESSING_FEATURE to toggle all of the relevant features and properties. I.e., setting it to true turns on all of the restrictions, and setting it to false turns them off. But I've just noticed that in JDK xerces, http://apache.org/xml/features/disallow-doctype-decl defaults to false even though SECURE_PROCESSING_FEATURE defaults to true. Regarding the related ACCESS_EXTERNAL_DTD_PROPERTY, which can restrict the protocols by which a DTD is loaded, the JAXP 1.6 spec says, "the default value is implementation specific and therefore not specified. ... When FEATURE_SECURE_PROCESSING` is enabled, it is recommended that
implementations restrict external connections by default, though this may cause
problems for applications that process XML/XSD/XSL with external references." On the other hand, I see that the Oracle JAXP tutorial says, "For JDK 8, the new accessExternal* properties are proposed to be set to the empty string [i.e., no protocols are permitted] when FSP is explicitly set. This is only the case when FSP is set through the API, e.g. factory.setFeature(FSP, true). Although FSP is true by default for DOM, SAX and Schema Validation it is not treated as if "explicitly" set, JDK 8 therefore does not set restrictions by default."

Actually, I think what I've done is simpler and makes as much sense as the JDK version, although I would change it if there is a strong desire in that direction. In either case, though, the discussion here raises an important point. The issue is not so much what are the default values, but what values are actually used, by default or by configuration. On one hand, setting DISALLOW_DOCTYPE_DECL_FEATURE to false opens up a security hole. On the other hand, Farah's latest comment suggests that maybe it should be set to false, since setting it to true might cause problems in the usual case.

Thoughts?

[wolfc]

The exercise is to plug potential security holes, not address common cases.
You could introduce an option in jsf-impl to go for insecure mode. That would be up to the user to activate. We must never activate something insecure out of the box.

[ronsigal]

re: You could introduce an option in jsf-impl to go for insecure mode.

I think that's a good idea.

[fjuma]

+1

[scottmarlow]

Adding a JSF configuration option to enable/disable DOCLET support could be useful. Does the JSF specification require DOCLET support?

[fjuma]

DOCTYPE is mentioned in Section 10.3.1.1 in the JSF 2.2 specification. Here's the relevant snippet:

When processing Facelet VDL files, the system must ensure that at most one XML declaration and at most one DOCTYPE declaration appear in the rendered markup, if and only if there is corresponding markup in the Facelet VDL files for those elements. If multiple occurrences of XML declaration and DOCTYPE declaration are encountered when processing Facelet VDL files, the “outer-most” occurrence is the one that must be rendered. If an XML declaration is present, it must be the very first markup rendered, and it must precede any DOCTYPE declaration (if present). The output of the XML and DOCTYPE declarations are subject to the configuration options listed in the table titled “Valid values and their implications on the processing of Facelet VDL files” in Section 1.1.1.1 “The facelets-processing element”.

Note that as mentioned previously, JSF 2.2 currently renders the HTML5 DOCTYPE regardless of what is set in the source .xhtml file.

One thing to keep in mind is that we would be adding the new configuration option to the latest version of Mojarra (e.g., 2.2.12). If an older version of Mojarra is used with WildFly (the JSF version can be switched easily, using the WildFly multi-JSF feature), then the same error would occur again if a JSF app contained a DOCTYPE declaration in its .xhtml file. This error could be fixed by removing the DOCTYPE declaration from the app of course, but just wanted to point this out.

[ronsigal]

So, I think the consensus is that there should be a configuration option that would appear in a configuration file like standalone.xml. I guess the default value should be to turn on security, which would prohibit DOCTYPE in .xhtml files.

1. Am I right?
2. Who will do it?

I suppose that implies two changes:

1. in jsf-impl
2. in configuration files

I don't know anything about either of those things. I can figure it out, unless someone else wants to do it. ;-)

[fjuma]

Ron, I can investigate this further and work on this.

[ronsigal]

Farah, that's wonderful. Let me know if there's anything I can do to help.

[ctomc]

@ronsigal @fjuma @scottmarlow any news on this?

[fjuma]

@ctomc Apologies for the delayed response, just got back from PTO. The Mojarra team just recently approved and merged my patch for adding a context param to indicate whether or not DOCTYPE declarations should be allowed (see JAVASERVERFACES-4130). I will be applying this patch to our fork and submitting a PR to add a disallow-doctype-decl attribute to the JSF subsystem (see WFLY-6545). This attribute will be used to specify the default value of the new context param, making it possible for users to allow DOCTYPE declarations by default for JSF apps, if desired.

[ronsigal]

Thank you, Farah!!

[fjuma]

I've applied the context param patch to our Mojarra 2.2.13 fork and released the 2.2.13.SP2 version of jsf-impl. I've also submitted the following PR against WildFly:

wildfly/wildfly#9076