From 56e6bfc3ff90590c65b4498f924bd5dfa533ea6c Mon Sep 17 00:00:00 2001 From: Zac Spitzer Date: Tue, 18 Jul 2023 13:37:36 +0200 Subject: [PATCH] LDEV-3451 disable XXE by default https://luceeserver.atlassian.net/browse/LDEV-3451 --- .../java/lucee/runtime/text/xml/XMLUtil.java | 84 ++++++----- test/tickets/LDEV1676.cfc | 132 ++++++++++++++++++ test/tickets/LDEV1676/Application.cfc | 64 +++++++++ test/tickets/LDEV1676/LDEV1676.cfm | 39 ++++++ 4 files changed, 281 insertions(+), 38 deletions(-) create mode 100644 test/tickets/LDEV1676.cfc create mode 100644 test/tickets/LDEV1676/Application.cfc create mode 100644 test/tickets/LDEV1676/LDEV1676.cfm diff --git a/core/src/main/java/lucee/runtime/text/xml/XMLUtil.java b/core/src/main/java/lucee/runtime/text/xml/XMLUtil.java index 4642aa2b2f..ea15968e05 100755 --- a/core/src/main/java/lucee/runtime/text/xml/XMLUtil.java +++ b/core/src/main/java/lucee/runtime/text/xml/XMLUtil.java @@ -327,6 +327,12 @@ private static DocumentBuilderFactory newDocumentBuilderFactory(InputSource vali factory.setValidating(false); } + // secure by default LDEV-3451 + boolean featureSecure = true; + boolean disallowDocType = true; + boolean externalGeneralEntities = false; + + // can be overriden per application PageContext pc = ThreadLocalPageContext.get(); if (pc != null) { ApplicationContextSupport ac = ((ApplicationContextSupport) pc.getApplicationContext()); @@ -334,54 +340,56 @@ private static DocumentBuilderFactory newDocumentBuilderFactory(InputSource vali if (features != null) { try { // handle feature aliases, e.g. secure Object obj; - boolean featureValue; + obj = features.get(KEY_FEATURE_SECURE, null); - if (obj != null) { - featureValue = Caster.toBoolean(obj); - if (featureValue) { - // set features per - // https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html - factory.setFeature(XMLConstants.FEATURE_DISALLOW_DOCTYPE_DECL, true); - factory.setFeature(XMLConstants.FEATURE_EXTERNAL_GENERAL_ENTITIES, false); - factory.setFeature(XMLConstants.FEATURE_EXTERNAL_PARAMETER_ENTITIES, false); - factory.setFeature(XMLConstants.FEATURE_NONVALIDATING_LOAD_EXTERNAL_DTD, false); - factory.setXIncludeAware(false); - factory.setExpandEntityReferences(false); - factory.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, ""); - factory.setAttribute(XMLConstants.ACCESS_EXTERNAL_SCHEMA, ""); - } - features.remove(KEY_FEATURE_SECURE); - } + if (obj != null) featureSecure = Caster.toBoolean(obj); obj = features.get(KEY_FEATURE_DISALLOW_DOCTYPE_DECL, null); - if (obj != null) { - featureValue = Caster.toBoolean(obj); - factory.setFeature(XMLConstants.FEATURE_DISALLOW_DOCTYPE_DECL, featureValue); - features.remove(KEY_FEATURE_DISALLOW_DOCTYPE_DECL); - } + if (obj != null) disallowDocType = Caster.toBoolean(obj); obj = features.get(KEY_FEATURE_EXTERNAL_GENERAL_ENTITIES, null); - if (obj != null) { - featureValue = Caster.toBoolean(obj); - factory.setFeature(XMLConstants.FEATURE_EXTERNAL_GENERAL_ENTITIES, featureValue); - features.remove(KEY_FEATURE_EXTERNAL_GENERAL_ENTITIES); - } + if (obj != null) externalGeneralEntities = Caster.toBoolean(obj); } - catch (PageException | ParserConfigurationException ex) { + catch (PageException ex) { throw new RuntimeException(ex); } - - features.forEach((k, v) -> { - try { - factory.setFeature(k.toString().toLowerCase(), Caster.toBoolean(v)); - } - catch (PageException | ParserConfigurationException ex) { - throw new RuntimeException(ex); - } - }); } } - + + try { // handle feature aliases, e.g. secure + if (featureSecure) { + // set features per + // https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html + factory.setFeature(XMLConstants.FEATURE_DISALLOW_DOCTYPE_DECL, true); + factory.setFeature(XMLConstants.FEATURE_EXTERNAL_GENERAL_ENTITIES, false); + factory.setFeature(XMLConstants.FEATURE_EXTERNAL_PARAMETER_ENTITIES, false); + factory.setFeature(XMLConstants.FEATURE_NONVALIDATING_LOAD_EXTERNAL_DTD, false); + factory.setXIncludeAware(false); + factory.setExpandEntityReferences(false); + factory.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, ""); + factory.setAttribute(XMLConstants.ACCESS_EXTERNAL_SCHEMA, ""); + } + //features.remove(KEY_FEATURE_SECURE); + + factory.setFeature(XMLConstants.FEATURE_DISALLOW_DOCTYPE_DECL, disallowDocType); + //features.remove(KEY_FEATURE_DISALLOW_DOCTYPE_DECL); + + factory.setFeature(XMLConstants.FEATURE_EXTERNAL_GENERAL_ENTITIES, externalGeneralEntities); + //features.remove(KEY_FEATURE_EXTERNAL_GENERAL_ENTITIES); + } + catch (ParserConfigurationException ex) { + throw new RuntimeException(ex); + } + /* + features.forEach((k, v) -> { + try { + factory.setFeature(k.toString().toLowerCase(), Caster.toBoolean(v)); + } + catch (PageException | ParserConfigurationException ex) { + throw new RuntimeException(ex); + } + }); + */ return factory; } diff --git a/test/tickets/LDEV1676.cfc b/test/tickets/LDEV1676.cfc new file mode 100644 index 0000000000..7e38e31591 --- /dev/null +++ b/test/tickets/LDEV1676.cfc @@ -0,0 +1,132 @@ +component extends = "org.lucee.cfml.test.LuceeTestCase" labels="xml" { + function beforeAll(){ + variables.uri = createURI("LDEV1676"); + //systemOutput(" ", true); + } + + function run( testresults , testbox ) { + describe( "testcase for LDEV-1676", function () { + it( title="Check xmlFeatures externalGeneralEntities=true, secure: false",body = function ( currentSpec ){ + local.result = _InternalRequest( + template : "#uri#/LDEV1676.cfm", + forms : { scene: "externalGeneralEntities-True" } + ).filecontent; + expect( trim( result ) ).toInclude("http://update.lucee.org/rest/update/provider/echoGet/cgi"); + }); + + it( title="Check xmlFeatures externalGeneralEntities=false",body = function ( currentSpec ) { + local.result = _InternalRequest( + template : "#uri#/LDEV1676.cfm", + forms : { scene: "externalGeneralEntities-False" } + ).filecontent; + expect( trim( result ) ).toInclude("security restrictions set by XMLFeatures"); + expect( trim( result ) ).toInclude("NullPointerException"); + }); + + it( title="Check xmlFeatures disallowDoctypeDecl=true",body = function ( currentSpec ) { + local.result = _InternalRequest( + template : "#uri#/LDEV1676.cfm", + forms : { scene: "disallowDoctypeDecl-True" } + ).filecontent; + expect( trim( result ) ).toInclude("DOCTYPE"); + }); + }); + + describe( "check combined xmlFeatures directives", function () { + + it( title="Check xmlFeatures default, good xml",body = function ( currentSpec ) { + local.result = _InternalRequest( + template : "#uri#/LDEV1676.cfm", + forms : { + scene: "default", + doctype: false, + entity: false, + } + ).filecontent; + expect( trim( result ) ).toBe("lucee"); + }); + + it( title="Check xmlFeatures default, bad xml",body = function ( currentSpec ) { + local.result = _InternalRequest( + template : "#uri#/LDEV1676.cfm", + forms : { + scene: "default", + doctype: true, + entity: true + } + ).filecontent; + expect( trim( result ) ).toInclude("DOCTYPE is disallowed when the feature"); + }); + + it( title="Check xmlFeatures all secure, bad xml",body = function ( currentSpec ) { + local.result = _InternalRequest( + template : "#uri#/LDEV1676.cfm", + forms : { + scene: "all-secure", + doctype: true, + entity: true, + } + ).filecontent; + expect( trim( result ) ).toInclude("DOCTYPE is disallowed when the feature"); + }); + + it( title="Check xmlFeatures all insecure, bad xml",body = function ( currentSpec ) { + local.result = _InternalRequest( + template : "#uri#/LDEV1676.cfm", + forms : { + scene: "all-insecure", + doctype: true, + entity: true + } + ).filecontent; + expect( trim( result ) ).toInclude("http://update.lucee.org/rest/update/provider/echoGet/cgi"); + }); + + it( title="Check xmlFeatures all secure, good xml",body = function ( currentSpec ) { + local.result = _InternalRequest( + template : "#uri#/LDEV1676.cfm", + forms : { + scene: "all-secure", + doctype: false, + entity: false + } + ).filecontent; + expect( trim( result ) ).toBe("lucee"); + }); + + }); + + describe( "check bad config handling", function () { + + it( title="Check xmlFeatures invalidConfig secure",body = function ( currentSpec ) { + local.result = _InternalRequest( + template : "#uri#/LDEV1676.cfm", + forms : { scene: "invalidConfig-secure" } + ).filecontent; + expect( trim( result ) ).toInclude( "casterException" ); + }); + + it( title="Check xmlFeatures invalidConfig docType",body = function ( currentSpec ) { + local.result = _InternalRequest( + template : "#uri#/LDEV1676.cfm", + forms : { scene: "invalidConfig-docType" } + ).filecontent; + expect( trim( result ) ).toInclude( "casterException" ); + }); + + it( title="Check xmlFeatures invalidConfig Entities",body = function ( currentSpec ) { + local.result = _InternalRequest( + template : "#uri#/LDEV1676.cfm", + forms : { scene: "invalidConfig-Entities" } + ).filecontent; + expect( trim( result ) ).toInclude( "casterException" ); + }); + + }); + } + + private string function createURI(string calledName){ + var baseURI="/test/#listLast(getDirectoryFromPath(getCurrenttemplatepath()),"\/")#/"; + return baseURI&""&calledName; + } +} diff --git a/test/tickets/LDEV1676/Application.cfc b/test/tickets/LDEV1676/Application.cfc new file mode 100644 index 0000000000..7d7a2199e4 --- /dev/null +++ b/test/tickets/LDEV1676/Application.cfc @@ -0,0 +1,64 @@ +component { + this.name="LDEV1676"; + param name="FORM.Scene"; + param name="FORM.docType" default="true"; + param name="FORM.entity" default="true"; + + switch (FORM.Scene){ + case "externalGeneralEntities-True": + this.xmlFeatures ={ + "externalGeneralEntities": true, + "disallowDoctypeDecl": false, + "secure": false + } + break; + case "externalGeneralEntities-False": + this.xmlFeatures = { + "externalGeneralEntities": false, + "secure": true, + "disallowDoctypeDecl": false + }; + break; + case "disallowDoctypeDecl-True": + this.xmlFeatures = { + "externalGeneralEntities": false, + "secure": true, + "disallowDoctypeDecl": true + }; + break; + case "invalidConfig-Secure": + this.xmlFeatures = { + "secure": "lucee" + }; + break; + case "invalidConfig-Doctype": + this.xmlFeatures = { + "disallowDoctypeDecl": "lucee" + }; + break; + case "invalidConfig-Entities": + this.xmlFeatures = { + "disallowDoctypeDecl": "lucee" + }; + break; + case "all-secure": + this.xmlFeatures = { + "externalGeneralEntities": false, + "secure": true, + "disallowDoctypeDecl": true + }; + break; + case "all-insecure": + this.xmlFeatures = { + "externalGeneralEntities": true, + "secure": false, + "disallowDoctypeDecl": false + }; + break; + case "default": + break; + default: + throw "unknown scene: #form.scene#"; + break; + } +} \ No newline at end of file diff --git a/test/tickets/LDEV1676/LDEV1676.cfm b/test/tickets/LDEV1676/LDEV1676.cfm new file mode 100644 index 0000000000..98cc52dc69 --- /dev/null +++ b/test/tickets/LDEV1676/LDEV1676.cfm @@ -0,0 +1,39 @@ + + + + + + + ]> + + + &xxe; + + lucee + + + + /* + settings = getApplicationSettings(); + + systemOutput( form.toJson(), true ); + if (structKeyExists(settings, "xmlFeatures" ) ) { + systemOutput( settings.xmlFeatures.toJson(), true ); + } else { + systemOutput("xmlFeatures not set", true); + } + systemOutput( "LDEV1676.cfc:" & CallStackGet( "array" )[ 2 ].linenumber, true ); + systemOutput( xml, true ); + */ + try { + result = xmlSearch( xml, "/foo" )[1].xmltext; + //systemOutput( result, true ); + echo( result ); + } catch (e) { + + //systemOutput(cfcatch.type & " " & cfcatch.message, true); + + echo( cfcatch.type & " " & cfcatch.message ); + } + \ No newline at end of file