etf-validator / etf-webapp

:earth_africa: :mag: ETF is an open source testing framework for spatial data and services
https://www.etf-validator.net
European Union Public License 1.2
19 stars 19 forks source link

"DOCTYPE disallowed" on ETF release candidate WMS INSPIRE ETS execution #222

Open carlospzurita opened 3 years ago

carlospzurita commented 3 years ago

Description

During the testing of the new release candidate of the ETF, an error has been encountered during the execution of the the INSPIRE WMS Executable Test Suite. This error happens on GetCapabiltiesOperation > at.05 > no request parameter , as shown on the attached TestReport.

The logs for the TestRuns displays this information

ERROR DOCTYPE is disallowed when the feature "http://apache.org/xml/features/disallow-doctype-decl" set to true.

The message does not appear on the production instance of the INSPIRE Reference validator. The mentioned requirement is correctly marked as passed for this WMS endpoint.

Operating systems and browser

Steps to Reproduce

  1. Load INSPIRE ETS on ETF
  2. Create a Test Run using the WMS ETS
  3. Use https://services.bgr.de/wms/inspire_nz/gerseis/?REQUEST=GetCapabilities&SERVICE=WMS&VERSION=1.3.0 as the service endpoint
  4. Check result on GetCapabiltiesOperation > at.05 > no request parameter
  5. Check log for test run

Expected behavior:

Test should marked as passed Actual behavior: Test fails due to unexpected error on XML parsing. Error on DOCTYPE declaration appears on log file.

Causes and alternatives

The main suspect to be the source of this issue is the default configuration for the XMLSlurper library that is used on Groovy to parse the contents of the GetCapabilities XML. We have tried to override this check using the solution provided on this post, without success


parser=new XmlSlurper()
parser.setFeature("http://apache.org/xml/features/disallow-doctype-decl", false) 
parser.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
parser.parse(it)
carlospzurita commented 3 years ago

As further clarification for the issue, what we are trying to accomplish for this feature is to override the check and try to execute the validation as usual for the services ETS, given that the DOCTYPE check is not enforced in the Implementing Rules. This is what we tried on our first approach disabling the feature on the XmlSlurper.

However, we understand that this element can cause some troubles on the ETF, and maybe it is best to leave the check in place and try to circumvent the error somehow.

Do you think the check for the DOCTYPE element is mandatory? And in that case, should we raise an exception as usual wrapping the instance of the XmlSlurper in a try-catch and send an error message to the user in the Test Report? Any feedback that you can offer on this matter is welcome.

jonherrmann commented 3 years ago

Do you think the check for the DOCTYPE element is mandatory?

Yes, it is. At least for public instances. See this page of the Open Web Application Security Project.

And in that case, should we raise an exception as usual wrapping the instance of the XmlSlurper in a try-catch and send an error message to the user in the Test Report?

It would be good for the user to know what the problem is and the easiest way would be for her/him to see it in the report.

cportele commented 3 years ago

I think there is an added complication that may need a discussion in INSPIRE. INSPIRE has Technical Guidance for both WMS 1.1.1 and WMS 1.3.0.

WMS 1.1.1 is so old, it uses DTD and requires the DOCTYPE declaration. WMS 1.3.0 uses XML Schema and not DTD / DOCTYPE. For security reasons, DOCTYPE and DTD should not be allowed (see the link above), but that is not compatible with WMS 1.1.1. Maybe it would be time to deprecate WMS 1.1.1 in INSPIRE?

As an aside, the behaviour of the WMS is incorrect. The response that creates the issue is a WMS 1.1.1 exception report (hence the DTD reference), but since the service supports WMS 1.3.0 and the request has not requested version=1.1.1 it should have been a WMS 1.3.0 service exception report, which would not have used DOCTYPE and created no failure. However, the general issue of supporting WMS 1.1.1 at all is separate from this.

jonherrmann commented 3 years ago

Thank you, Clemens. Let's summarise that this issue reveals three points:

  1. the "outdated" INSPIRE TG for WMS
  2. the resulting DTD XXE security issue
  3. the test case that does not check for the correct version in the exception report.

At the implementation level, the problem could at least be mitigated by point 3. That would probably be a good first step, with little effort.

carlospzurita commented 3 years ago

Dear all,

After discussing this issue and how to proceed regarding the INSPIRE validator, we have decided to handle the !DOCTYPE declaration at ETS level for WMS.

We are rejecting the file if the response contains that string, before parsing it as an XML, and then inform the user of the issue and sharing the recommendations from OWASP.

This way we reduce the impact on the source code and can be maintained in a simpler manner.