From 47c65149aea164a71df78b3284045b53634029c3 Mon Sep 17 00:00:00 2001 From: Saumya Navani <70484210+Saumyanavani@users.noreply.github.com> Date: Fri, 1 Sep 2023 15:39:35 -0400 Subject: [PATCH] added support for cases when one property was set but the rest weren't. Expand Entity References support still pending. (#106) --- .../DBFInsertPropertyStatementVisitor.java | 48 ++++++++--- .../xml/DocumentBuilderFactoryFixVisitor.java | 29 ++++++- .../xml/DocumentBuilderFactoryXXETest.java | 81 +++++++++++++++++++ 3 files changed, 142 insertions(+), 16 deletions(-) diff --git a/src/main/java/org/openrewrite/java/security/xml/DBFInsertPropertyStatementVisitor.java b/src/main/java/org/openrewrite/java/security/xml/DBFInsertPropertyStatementVisitor.java index d67d2e9..c2962c6 100644 --- a/src/main/java/org/openrewrite/java/security/xml/DBFInsertPropertyStatementVisitor.java +++ b/src/main/java/org/openrewrite/java/security/xml/DBFInsertPropertyStatementVisitor.java @@ -27,6 +27,7 @@ public class DBFInsertPropertyStatementVisitor

extends XmlFactoryInsertVisito private final boolean disallowGeneralEntities; private final boolean disallowParameterEntities; private final boolean disallowLoadExternalDTD; + private final boolean setXIncludeAware; public DBFInsertPropertyStatementVisitor( J.Block scope, @@ -35,8 +36,9 @@ public DBFInsertPropertyStatementVisitor( boolean needsDisallowDoctypesTrue, boolean needsDisableGeneralEntities, boolean needsDisableParameterEntities, - boolean needsLoadExternalDTD - ) { + boolean needsLoadExternalDTD, + boolean needsSetXIncludeAware, + boolean needsSetExpandEntityReferences) { super( scope, dbfFactoryVariable, @@ -50,27 +52,32 @@ public DBFInsertPropertyStatementVisitor( disallowGeneralEntities = false; disallowParameterEntities = false; disallowLoadExternalDTD = false; + setXIncludeAware = false; } else if (needsDisallowDoctypesTrue && !accIsEmpty) { disallowDoctypes = false; disallowGeneralEntities = needsDisableGeneralEntities; disallowParameterEntities = needsDisableParameterEntities; disallowLoadExternalDTD = needsLoadExternalDTD; + setXIncludeAware = needsSetXIncludeAware; } else if (!needsDisallowDoctypesTrue && !accIsEmpty) { disallowDoctypes = false; disallowGeneralEntities = false; disallowLoadExternalDTD = false; disallowParameterEntities = false; + setXIncludeAware = false; } else { disallowDoctypes = false; disallowGeneralEntities = false; disallowLoadExternalDTD = false; disallowParameterEntities = false; + setXIncludeAware = false; } + } @Override public void updateTemplate() { - if (disallowDoctypes && !disallowGeneralEntities && !disallowParameterEntities && !disallowLoadExternalDTD) { + if (disallowDoctypes && !disallowGeneralEntities && !disallowParameterEntities && !disallowLoadExternalDTD && !setXIncludeAware) { getTemplate().append( "String FEATURE = \"http://apache.org/xml/features/disallow-doctype-decl\";\n" + "try {\n" + @@ -92,17 +99,32 @@ public void updateTemplate() { "\n" + " FEATURE = \"http://xml.org/sax/features/external-general-entities\";\n" + " " + getFactoryVariableName() + ".setFeature(FEATURE, false);\n" + - "\n" + - " " + getFactoryVariableName() + ".setXIncludeAware(false);\n" + - " " + getFactoryVariableName() + ".setExpandEntityReferences(false);\n" + - "\n" + - " " + getFactoryVariableName() + ".setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);\n" + - "\n" + - "} catch (ParserConfigurationException e) {\n" + - " throw new IllegalStateException(\"The feature '\"\n" + - " + FEATURE + \"' is not supported by your XML processor.\", e);\n" + - "}\n" + "\n" ); + if (setXIncludeAware){ + getTemplate().append( + " " + getFactoryVariableName() + ".setXIncludeAware(false);\n" + + " " + getFactoryVariableName() + ".setExpandEntityReferences(false);\n" + + "\n" + + " " + getFactoryVariableName() + ".setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);\n" + + "\n" + + "} catch (ParserConfigurationException e) {\n" + + " throw new IllegalStateException(\"The feature '\"\n" + + " + FEATURE + \"' is not supported by your XML processor.\", e);\n" + + "}\n" + ); + } else { + getTemplate().append( + " " + getFactoryVariableName() + ".setExpandEntityReferences(false);\n" + + "\n" + + " " + getFactoryVariableName() + ".setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);\n" + + "\n" + + "} catch (ParserConfigurationException e) {\n" + + " throw new IllegalStateException(\"The feature '\"\n" + + " + FEATURE + \"' is not supported by your XML processor.\", e);\n" + + "}\n" + ); + } } } } diff --git a/src/main/java/org/openrewrite/java/security/xml/DocumentBuilderFactoryFixVisitor.java b/src/main/java/org/openrewrite/java/security/xml/DocumentBuilderFactoryFixVisitor.java index 2c4bde3..79bf94b 100644 --- a/src/main/java/org/openrewrite/java/security/xml/DocumentBuilderFactoryFixVisitor.java +++ b/src/main/java/org/openrewrite/java/security/xml/DocumentBuilderFactoryFixVisitor.java @@ -40,7 +40,8 @@ public class DocumentBuilderFactoryFixVisitor

extends XmlFactoryVisitor

{ static final InvocationMatcher DBF_NEW_INSTANCE = InvocationMatcher.fromMethodMatcher(new MethodMatcher("javax.xml.parsers.DocumentBuilderFactory newInstance*()")); static final InvocationMatcher DBF_PARSER_SET_FEATURE = InvocationMatcher.fromMethodMatcher(new MethodMatcher("javax.xml.parsers.DocumentBuilderFactory setFeature(java.lang.String, boolean)")); - + static final InvocationMatcher DBF_PARSER_SET_X_INCLUDE_AWARE = InvocationMatcher.fromMethodMatcher(new MethodMatcher("javax.xml.parsers.DocumentBuilderFactory setXIncludeAware(boolean)")); + static final InvocationMatcher DBF_PARSER_SET_EXPAND_ENTITY_REFERENCES = InvocationMatcher.fromMethodMatcher(new MethodMatcher("javax.xml.parsers.DocumentBuilderFactory setExpandEntityReferences(boolean)")); private static final String DBF_FQN = "javax.xml.parsers.DocumentBuilderFactory"; private static final String DISALLOW_DOCTYPE_DECLARATIONS = "http://apache.org/xml/features/disallow-doctype-decl"; private static final String DISABLE_GENERAL_ENTITIES = "http://xml.org/sax/features/external-general-entities"; @@ -127,6 +128,25 @@ public Expression visitExpression(Expression expression, P p) { return super.visitExpression(expression, p); } + @Override + public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, P ctx) { + J.MethodInvocation mi = super.visitMethodInvocation(method, ctx); + if (DBF_PARSER_SET_X_INCLUDE_AWARE.matches(mi)) { + addMessage(SET_X_INCLUDE_AWARE_PROPERTY_NAME); + } else if (DBF_PARSER_SET_EXPAND_ENTITY_REFERENCES.matches(mi)){ + addMessage(SET_EXPAND_ENTITY_REFERENCES_PROPERTY_NAME); + } + return mi; + } + + @Override + public J.Block visitBlock(J.Block block, P p) { + if (J.Block.isInitBlock(getCursor())) { + addMessage(DBF_INITIALIZATION_METHOD); + } + return super.visitBlock(block, p); + } + @Override public J.ClassDeclaration visitClassDeclaration(J.ClassDeclaration classDecl, P ctx) { J.ClassDeclaration cd = super.visitClassDeclaration(classDecl, ctx); @@ -138,6 +158,8 @@ public J.ClassDeclaration visitClassDeclaration(J.ClassDeclaration classDecl, P Cursor generalEntitiesDisabledCursor = getCursor().getMessage(DISABLE_GENERAL_ENTITIES + i); Cursor parameterEntitiesDisabledCursor = getCursor().getMessage(DISABLE_PARAMETER_ENTITIES + i); Cursor loadExternalDTDCursor = getCursor().getMessage(LOAD_EXTERNAL_DTD + i); + Cursor setXIncludeAwareCursor = getCursor().getMessage(SET_X_INCLUDE_AWARE_PROPERTY_NAME + i); + Cursor setExpandEntityReferencesCursor = getCursor().getMessage(SET_EXPAND_ENTITY_REFERENCES_PROPERTY_NAME + i); Cursor setPropertyBlockCursor = null; if (disallowedDTDTrueCursor == null) { @@ -154,8 +176,9 @@ public J.ClassDeclaration visitClassDeclaration(J.ClassDeclaration classDecl, P disallowedDTDTrueCursor == null, generalEntitiesDisabledCursor == null, parameterEntitiesDisabledCursor == null, - loadExternalDTDCursor == null - )); + loadExternalDTDCursor == null, + setXIncludeAwareCursor == null, + setExpandEntityReferencesCursor == null)); } } return cd; diff --git a/src/test/java/org/openrewrite/java/security/xml/DocumentBuilderFactoryXXETest.java b/src/test/java/org/openrewrite/java/security/xml/DocumentBuilderFactoryXXETest.java index 92fa8cc..d7075c6 100644 --- a/src/test/java/org/openrewrite/java/security/xml/DocumentBuilderFactoryXXETest.java +++ b/src/test/java/org/openrewrite/java/security/xml/DocumentBuilderFactoryXXETest.java @@ -277,4 +277,85 @@ class myDBFReader { ) ); } + + @Test + void factoryIsNotVulnerableWithDTDsAndXIncludeAwareButPropertiesMissing() { + rewriteRun( + xml( + """ + + + + ]> + + + + """ + ), + java( + """ + import javax.xml.parsers.DocumentBuilderFactory; + import javax.xml.parsers.DocumentBuilder; + import javax.xml.parsers.ParserConfigurationException; // catching unsupported features + import javax.xml.XMLConstants; + + class myDBFReader { + DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance(); + + { + dbf.setXIncludeAware(false); + } + + DocumentBuilder safebuilder = dbf.newDocumentBuilder(); + } + """, + """ + import javax.xml.parsers.DocumentBuilderFactory; + import javax.xml.parsers.DocumentBuilder; + import javax.xml.parsers.ParserConfigurationException; // catching unsupported features + import javax.xml.XMLConstants; + + class myDBFReader { + DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance(); + + { + String FEATURE = null; + try { + FEATURE = "http://xml.org/sax/features/external-parameter-entities"; + dbf.setFeature(FEATURE, false); + + FEATURE = "http://apache.org/xml/features/nonvalidating/load-external-dtd"; + dbf.setFeature(FEATURE, false); + + FEATURE = "http://xml.org/sax/features/external-general-entities"; + dbf.setFeature(FEATURE, false); + + dbf.setExpandEntityReferences(false); + + dbf.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); + + } catch (ParserConfigurationException e) { + throw new IllegalStateException("The feature '" + + FEATURE + "' is not supported by your XML processor.", e); + } + + } + + { + dbf.setXIncludeAware(false); + } + + DocumentBuilder safebuilder = dbf.newDocumentBuilder(); + } + """ + ) + ); + + } }