From d809120723be65da0ee11a23f21ca19c0bec7371 Mon Sep 17 00:00:00 2001 From: Zac Spitzer Date: Tue, 18 Jul 2023 13:37:36 +0200 Subject: [PATCH 1/3] 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 | 115 ++++++++++++++++-- test/tickets/LDEV1676/Application.cfc | 78 +++++++++--- test/tickets/LDEV1676/LDEV1676.cfm | 51 ++++++-- 4 files changed, 247 insertions(+), 81 deletions(-) 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 2f6ba2a237..3764f06237 100755 --- a/core/src/main/java/lucee/runtime/text/xml/XMLUtil.java +++ b/core/src/main/java/lucee/runtime/text/xml/XMLUtil.java @@ -328,6 +328,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()); @@ -335,54 +341,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 index 12586a7a6f..983da40ed0 100644 --- a/test/tickets/LDEV1676.cfc +++ b/test/tickets/LDEV1676.cfc @@ -1,32 +1,125 @@ 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",body = function ( currentSpec ){ + it( title="Check xmlFeatures externalGeneralEntities=true, secure: false",body = function ( currentSpec ){ local.result = _InternalRequest( - template : "#uri#\LDEV1676.cfm", - forms : {scene=1} + template : "#uri#/LDEV1676.cfm", + forms : { scene: "externalGeneralEntities-True" } ).filecontent; - expect(trim(result)).toBe("http://update.lucee.org/rest/update/provider/echoGet/cgi"); + 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=2} + template : "#uri#/LDEV1676.cfm", + forms : { scene: "externalGeneralEntities-False" } ).filecontent; - expect(trim(result)).toInclude("security restrictions set by XMLFeatures"); + 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=3} + template : "#uri#/LDEV1676.cfm", + forms : { scene: "disallowDoctypeDecl-True" } ).filecontent; - expect(trim(result)).toInclude("DOCTYPE"); + 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" ); }); }); @@ -36,4 +129,4 @@ component extends = "org.lucee.cfml.test.LuceeTestCase" labels="xml" { var baseURI="/test/#listLast(getDirectoryFromPath(getCurrenttemplatepath()),"\/")#/"; return baseURI&""&calledName; } -} \ No newline at end of file +} diff --git a/test/tickets/LDEV1676/Application.cfc b/test/tickets/LDEV1676/Application.cfc index e302097293..5a3889b39a 100644 --- a/test/tickets/LDEV1676/Application.cfc +++ b/test/tickets/LDEV1676/Application.cfc @@ -1,24 +1,64 @@ component { this.name="LDEV1676"; - param name="FORM.Scene" default=""; + param name="FORM.Scene"; + param name="FORM.docType" default="true"; + param name="FORM.entity" default="true"; - if(FORM.Scene == 1) { - this.xmlFeatures.externalGeneralEntities = true; - } - - else if(FORM.Scene == 2) { - this.xmlFeatures = { - externalGeneralEntities: false, - secure: true, - disallowDoctypeDecl: false - }; - } - - else if(FORM.Scene == 3) { - this.xmlFeatures = { - externalGeneralEntities: false, - secure: true, - disallowDoctypeDecl: 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 index fa0ae7e0fa..baf29f238c 100644 --- a/test/tickets/LDEV1676/LDEV1676.cfm +++ b/test/tickets/LDEV1676/LDEV1676.cfm @@ -1,14 +1,39 @@ - - - ]> - &xxe; - - - - - #cfcatch.message# - - - \ No newline at end of file + + + + + + ]> + + + &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 From 695b31af95b5346a62d15af6280707db5c9c0759 Mon Sep 17 00:00:00 2001 From: Zac Spitzer Date: Tue, 18 Jul 2023 15:39:43 +0200 Subject: [PATCH 2/3] LDEV-4348 add xmlFeatures to getApplicationSettings https://luceeserver.atlassian.net/browse/LDEV-4348 --- .../system/GetApplicationSettings.java | 8 +++ test/tickets/LDEV4348.cfc | 56 +++++++++++++++++++ test/tickets/LDEV4348/Application.cfc | 26 +++++++++ test/tickets/LDEV4348/LDEV4348.cfm | 4 ++ 4 files changed, 94 insertions(+) create mode 100644 test/tickets/LDEV4348.cfc create mode 100644 test/tickets/LDEV4348/Application.cfc create mode 100644 test/tickets/LDEV4348/LDEV4348.cfm diff --git a/core/src/main/java/lucee/runtime/functions/system/GetApplicationSettings.java b/core/src/main/java/lucee/runtime/functions/system/GetApplicationSettings.java index 5d8ebd32db..bdb2b05d0e 100644 --- a/core/src/main/java/lucee/runtime/functions/system/GetApplicationSettings.java +++ b/core/src/main/java/lucee/runtime/functions/system/GetApplicationSettings.java @@ -109,6 +109,14 @@ public static Struct call(PageContext pc, boolean suppressFunctions, boolean onl sc.setEL("disableUpdate", sessionCookieData.isDisableUpdate()); sct.setEL("sessionCookie", sc); } + + Struct xmlFeatures = acs.getXmlFeatures(); + if (xmlFeatures == null) xmlFeatures = new StructImpl(); + Struct sxml = new StructImpl(Struct.TYPE_LINKED); + sxml.setEL("secure", Caster.toBoolean(xmlFeatures.get("secure", true))); + sxml.setEL("disallowDoctypeDecl", Caster.toBoolean(xmlFeatures.get("disallowDoctypeDecl", true))); + sxml.setEL("externalGeneralEntities", Caster.toBoolean(xmlFeatures.get("externalGeneralEntities", false))); + sct.setEL("xmlFeatures", sxml); sct.setEL("customTagPaths", toArray(ac.getCustomTagMappings())); sct.setEL("componentPaths", toArray(ac.getComponentMappings())); diff --git a/test/tickets/LDEV4348.cfc b/test/tickets/LDEV4348.cfc new file mode 100644 index 0000000000..ac04b5876b --- /dev/null +++ b/test/tickets/LDEV4348.cfc @@ -0,0 +1,56 @@ +component extends = "org.lucee.cfml.test.LuceeTestCase" labels="xml" { + function beforeAll(){ + variables.uri = createURI("LDEV4348"); + } + + function run( testresults , testbox ) { + + describe( "check combined xmlFeatures getApplicationSettings", function () { + + it( title="Check xmlFeatures default",body = function ( currentSpec ) { + local.result = _InternalRequest( + template : "#uri#/LDEV4348.cfm", + forms : { + scene: "default" + } + ).filecontent.deserializeJson(); + expect( result.secure ).toBeTrue(); + expect( result.disallowDoctypeDecl ).toBeTrue(); + expect( result.externalGeneralEntities ).toBeFalse(); + }); + + it( title="Check xmlFeatures all secure",body = function ( currentSpec ) { + local.result = _InternalRequest( + template : "#uri#/LDEV4348.cfm", + forms : { + scene: "all-secure" + } + ).filecontent.deserializeJson(); + expect( result.secure ).toBeTrue(); + expect( result.disallowDoctypeDecl ).toBeTrue(); + expect( result.externalGeneralEntities ).toBeFalse(); + }); + + it( title="Check xmlFeatures all insecure, bad xml",body = function ( currentSpec ) { + local.result = _InternalRequest( + template : "#uri#/LDEV4348.cfm", + forms : { + scene: "all-insecure" + } + ).filecontent.deserializeJson(); + expect( result.secure ).toBeFalse(); + expect( result.disallowDoctypeDecl ).toBeFalse(); + expect( result.externalGeneralEntities ).toBeTrue(); + }); + + }); + + } + + private string function createURI(string calledName){ + var baseURI="/test/#listLast(getDirectoryFromPath(getCurrenttemplatepath()),"\/")#/"; + return baseURI&""&calledName; + } +} + + diff --git a/test/tickets/LDEV4348/Application.cfc b/test/tickets/LDEV4348/Application.cfc new file mode 100644 index 0000000000..efeafcde28 --- /dev/null +++ b/test/tickets/LDEV4348/Application.cfc @@ -0,0 +1,26 @@ +component { + this.name="LDEV4348"; + param name="FORM.Scene"; + + switch (FORM.Scene){ + 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/LDEV4348/LDEV4348.cfm b/test/tickets/LDEV4348/LDEV4348.cfm new file mode 100644 index 0000000000..7b3068bc6b --- /dev/null +++ b/test/tickets/LDEV4348/LDEV4348.cfm @@ -0,0 +1,4 @@ + + settings = getApplicationSettings(); + echo( settings.xmlFeatures.toJson() ); + \ No newline at end of file From ac58f2747ad552998086c4bb93bc5070120aa7c7 Mon Sep 17 00:00:00 2001 From: Zac Spitzer Date: Tue, 18 Jul 2023 16:10:06 +0200 Subject: [PATCH 3/3] LDEV-3451 add a test to use cfapplication to override defaults --- .../functions/system/GetApplicationSettings.java | 6 +++--- test/tickets/LDEV1676.cfc | 14 ++++++++++++++ test/tickets/LDEV1676/Application.cfc | 1 + test/tickets/LDEV1676/LDEV1676.cfm | 11 +++++++++-- 4 files changed, 27 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/lucee/runtime/functions/system/GetApplicationSettings.java b/core/src/main/java/lucee/runtime/functions/system/GetApplicationSettings.java index bdb2b05d0e..41a4de0df6 100644 --- a/core/src/main/java/lucee/runtime/functions/system/GetApplicationSettings.java +++ b/core/src/main/java/lucee/runtime/functions/system/GetApplicationSettings.java @@ -113,9 +113,9 @@ public static Struct call(PageContext pc, boolean suppressFunctions, boolean onl Struct xmlFeatures = acs.getXmlFeatures(); if (xmlFeatures == null) xmlFeatures = new StructImpl(); Struct sxml = new StructImpl(Struct.TYPE_LINKED); - sxml.setEL("secure", Caster.toBoolean(xmlFeatures.get("secure", true))); - sxml.setEL("disallowDoctypeDecl", Caster.toBoolean(xmlFeatures.get("disallowDoctypeDecl", true))); - sxml.setEL("externalGeneralEntities", Caster.toBoolean(xmlFeatures.get("externalGeneralEntities", false))); + sxml.setEL("secure", xmlFeatures.get("secure", true)); + sxml.setEL("disallowDoctypeDecl", xmlFeatures.get("disallowDoctypeDecl", true)); + sxml.setEL("externalGeneralEntities", xmlFeatures.get("externalGeneralEntities", false)); sct.setEL("xmlFeatures", sxml); sct.setEL("customTagPaths", toArray(ac.getCustomTagMappings())); diff --git a/test/tickets/LDEV1676.cfc b/test/tickets/LDEV1676.cfc index 983da40ed0..57e7155c7b 100644 --- a/test/tickets/LDEV1676.cfc +++ b/test/tickets/LDEV1676.cfc @@ -94,6 +94,20 @@ component extends = "org.lucee.cfml.test.LuceeTestCase" labels="xml" { expect( trim( result ) ).toBe("lucee"); }); + // check if we can inline disable the settings back to the old behavior + it( title="Check xmlFeatures default, bad xml, cfapplication override",body = function ( currentSpec ) { + local.result = _InternalRequest( + template : "#uri#/LDEV1676.cfm", + forms : { + scene: "default", + doctype: true, + entity: true, + cfapplicationOverride: true + } + ).filecontent; + expect( trim( result ) ).toInclude("http://update.lucee.org/rest/update/provider/echoGet/cgi"); + }); + }); describe( "check bad config handling", function () { diff --git a/test/tickets/LDEV1676/Application.cfc b/test/tickets/LDEV1676/Application.cfc index 5a3889b39a..daee35c11a 100644 --- a/test/tickets/LDEV1676/Application.cfc +++ b/test/tickets/LDEV1676/Application.cfc @@ -3,6 +3,7 @@ component { param name="FORM.Scene"; param name="FORM.docType" default="true"; param name="FORM.entity" default="true"; + param name="FORM.cfapplicationOverride" default="false"; switch (FORM.Scene){ case "externalGeneralEntities-True": diff --git a/test/tickets/LDEV1676/LDEV1676.cfm b/test/tickets/LDEV1676/LDEV1676.cfm index baf29f238c..f67ed4aeee 100644 --- a/test/tickets/LDEV1676/LDEV1676.cfm +++ b/test/tickets/LDEV1676/LDEV1676.cfm @@ -14,6 +14,15 @@ + + if (form.cfapplicationOverride){ + //systemOutput("cfapplicationOverride", true) + application action="update" xmlFeatures={ + "externalGeneralEntities": true, + "secure": false, + "disallowDoctypeDecl": false + }; + } /* settings = getApplicationSettings(); @@ -31,9 +40,7 @@ //systemOutput( result, true ); echo( result ); } catch (e) { - //systemOutput(cfcatch.type & " " & cfcatch.message, true); - echo( cfcatch.type & " " & cfcatch.message ); } \ No newline at end of file