From 4d72fb7036fc750013d6c6ae9686ec8855136c09 Mon Sep 17 00:00:00 2001 From: Aaron Blume Date: Thu, 17 Aug 2023 13:22:57 -0700 Subject: [PATCH 1/3] Adding two abstract visitor classes for XXE vulnerabilities --- .../xml/TransformerFactoryFixVisitor.java | 33 ++++---- ...actoryInsertAttributeStatementVisitor.java | 63 ++++---------- ...FactoryInsertPropertyStatementVisitor.java | 74 +++++------------ .../security/xml/XmlFactoryInsertVisitor.java | 83 +++++++++++++++++++ .../java/security/xml/XmlFactoryVisitor.java | 54 ++++++++++++ .../xml/XmlInputFactoryFixVisitor.java | 39 ++++----- .../XmlInputFactoryXXEVulnerabilityTest.java | 2 +- 7 files changed, 202 insertions(+), 146 deletions(-) create mode 100644 src/main/java/org/openrewrite/java/security/xml/XmlFactoryInsertVisitor.java create mode 100644 src/main/java/org/openrewrite/java/security/xml/XmlFactoryVisitor.java diff --git a/src/main/java/org/openrewrite/java/security/xml/TransformerFactoryFixVisitor.java b/src/main/java/org/openrewrite/java/security/xml/TransformerFactoryFixVisitor.java index bf8ff3f..c750563 100644 --- a/src/main/java/org/openrewrite/java/security/xml/TransformerFactoryFixVisitor.java +++ b/src/main/java/org/openrewrite/java/security/xml/TransformerFactoryFixVisitor.java @@ -15,21 +15,17 @@ */ package org.openrewrite.java.security.xml; -import lombok.AllArgsConstructor; import org.openrewrite.Cursor; import org.openrewrite.ExecutionContext; import org.openrewrite.Preconditions; import org.openrewrite.TreeVisitor; -import org.openrewrite.java.JavaIsoVisitor; import org.openrewrite.java.MethodMatcher; import org.openrewrite.java.search.UsesType; import org.openrewrite.java.tree.J; -import org.openrewrite.java.tree.TypeUtils; import javax.xml.XMLConstants; -@AllArgsConstructor -public class TransformerFactoryFixVisitor

extends JavaIsoVisitor

{ +public class TransformerFactoryFixVisitor

extends XmlFactoryVisitor

{ static final MethodMatcher TRANSFORMER_FACTORY_INSTANCE = new MethodMatcher("javax.xml.transform.TransformerFactory new*()"); static final MethodMatcher TRANSFORMER_FACTORY_SET_ATTRIBUTE = new MethodMatcher("javax.xml.transform.TransformerFactory setAttribute(java.lang.String, ..)"); static final MethodMatcher TRANSFORMER_FACTORY_SET_FEATURE = new MethodMatcher("javax.xml.transform.TransformerFactory setFeature(java.lang.String, ..)"); @@ -44,6 +40,16 @@ public class TransformerFactoryFixVisitor

extends JavaIsoVisitor

{ private static final String DISALLOW_MODIFY_FLAG = "DISALLOW_MODIFY_FLAG"; + public TransformerFactoryFixVisitor(ExternalDTDAccumulator acc) { + super( + TRANSFORMER_FACTORY_INSTANCE, + TRANSFORMER_FACTORY_FQN, + TRANSFORMER_FACTORY_INITIALIZATION_METHOD, + TRANSFORMER_FACTORY_VARIABLE_NAME, + acc + ); + } + @Override public J.ClassDeclaration visitClassDeclaration(J.ClassDeclaration classDecl, P ctx) { J.ClassDeclaration cd = super.visitClassDeclaration(classDecl, ctx); @@ -78,21 +84,10 @@ public J.ClassDeclaration visitClassDeclaration(J.ClassDeclaration classDecl, P return cd; } - @Override - public J.VariableDeclarations.NamedVariable visitVariable(J.VariableDeclarations.NamedVariable variable, P ctx) { - J.VariableDeclarations.NamedVariable v = super.visitVariable(variable, ctx); - if (TypeUtils.isOfClassType(v.getType(), TRANSFORMER_FACTORY_FQN)) { - getCursor().putMessageOnFirstEnclosing(J.ClassDeclaration.class, TRANSFORMER_FACTORY_VARIABLE_NAME, v.getSimpleName()); - } - return v; - } - @Override public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, P ctx) { J.MethodInvocation m = super.visitMethodInvocation(method, ctx); - if (TRANSFORMER_FACTORY_INSTANCE.matches(m)) { - getCursor().putMessageOnFirstEnclosing(J.ClassDeclaration.class, TRANSFORMER_FACTORY_INITIALIZATION_METHOD, getCursor().dropParentUntil(J.Block.class::isInstance)); - } else if (TRANSFORMER_FACTORY_SET_ATTRIBUTE.matches(m) && m.getArguments().get(0) instanceof J.FieldAccess) { + if (TRANSFORMER_FACTORY_SET_ATTRIBUTE.matches(m) && m.getArguments().get(0) instanceof J.FieldAccess) { // If either attribute value is not equal to the empty string, do not make any changes if (m.getArguments().get(1) instanceof J.Literal) { J.Literal string = (J.Literal) m.getArguments().get(1); @@ -112,7 +107,7 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, P ctx if (m.getArguments().get(1) instanceof J.Literal) { J.Literal bool = (J.Literal) m.getArguments().get(1); assert bool.getValue() != null; - if (!((Boolean) bool.getValue())) { + if (Boolean.FALSE.equals(bool.getValue())) { getCursor().putMessageOnFirstEnclosing(J.ClassDeclaration.class, DISALLOW_MODIFY_FLAG, getCursor().dropParentUntil(J.Block.class::isInstance)); } } @@ -132,6 +127,6 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, P ctx } public static TreeVisitor create(ExternalDTDAccumulator acc) { - return Preconditions.check(new UsesType<>(TRANSFORMER_FACTORY_FQN, true), new TransformerFactoryFixVisitor<>()); + return Preconditions.check(new UsesType<>(TRANSFORMER_FACTORY_FQN, true), new TransformerFactoryFixVisitor<>(acc)); } } diff --git a/src/main/java/org/openrewrite/java/security/xml/TransformerFactoryInsertAttributeStatementVisitor.java b/src/main/java/org/openrewrite/java/security/xml/TransformerFactoryInsertAttributeStatementVisitor.java index 62097e3..f57289b 100644 --- a/src/main/java/org/openrewrite/java/security/xml/TransformerFactoryInsertAttributeStatementVisitor.java +++ b/src/main/java/org/openrewrite/java/security/xml/TransformerFactoryInsertAttributeStatementVisitor.java @@ -15,18 +15,13 @@ */ package org.openrewrite.java.security.xml; -import org.openrewrite.Cursor; -import org.openrewrite.java.JavaIsoVisitor; -import org.openrewrite.java.JavaTemplate; import org.openrewrite.java.tree.J; -import org.openrewrite.java.tree.JavaCoordinates; import org.openrewrite.java.tree.Statement; -public class TransformerFactoryInsertAttributeStatementVisitor

extends JavaIsoVisitor

{ - private final J.Block scope; - private final StringBuilder attributeTemplate = new StringBuilder(); - private final String transformerFactoryVariableName; +import java.util.stream.Collectors; +import java.util.stream.Stream; +public class TransformerFactoryInsertAttributeStatementVisitor

extends XmlFactoryInsertVisitor

{ public TransformerFactoryInsertAttributeStatementVisitor( J.Block scope, String factoryVariableName, @@ -34,57 +29,31 @@ public TransformerFactoryInsertAttributeStatementVisitor( boolean needsStylesheetsDisabled, boolean needsFeatureSecureProcessing ) { - this.scope = scope; - this.transformerFactoryVariableName = factoryVariableName; + super( + scope, + new StringBuilder(), + factoryVariableName, + TransformerFactoryFixVisitor.TRANSFORMER_FACTORY_INSTANCE, + TransformerFactoryFixVisitor.TRANSFORMER_FACTORY_SET_ATTRIBUTE + ); if (needsExternalEntitiesDisabled) { - attributeTemplate.append(transformerFactoryVariableName).append(".setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, \"\");"); + getTemplate().append(getFactoryVariableName()).append(".setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, \"\");"); } if (needsStylesheetsDisabled) { - attributeTemplate.append(transformerFactoryVariableName).append(".setAttribute(XMLConstants.ACCESS_EXTERNAL_STYLESHEET, \"\");"); + getTemplate().append(getFactoryVariableName()).append(".setAttribute(XMLConstants.ACCESS_EXTERNAL_STYLESHEET, \"\");"); } if (needsFeatureSecureProcessing) { - attributeTemplate.append(transformerFactoryVariableName).append(".setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);"); + getTemplate().append(getFactoryVariableName()).append(".setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);"); } } @Override public J.Block visitBlock(J.Block block, P ctx) { J.Block b = super.visitBlock(block, ctx); - Statement beforeStatement = null; - if (b.isScope(scope)) { - for (int i = b.getStatements().size() - 2; i > -1; i--) { - Statement st = b.getStatements().get(i); - Statement stBefore = b.getStatements().get(i + 1); - if (st instanceof J.MethodInvocation) { - J.MethodInvocation m = (J.MethodInvocation) st; - if (TransformerFactoryFixVisitor.TRANSFORMER_FACTORY_INSTANCE.matches(m) || TransformerFactoryFixVisitor.TRANSFORMER_FACTORY_SET_ATTRIBUTE.matches(m)) { - beforeStatement = stBefore; - } - } else if (st instanceof J.VariableDeclarations) { - J.VariableDeclarations vd = (J.VariableDeclarations) st; - if (vd.getVariables().get(0).getInitializer() instanceof J.MethodInvocation) { - J.MethodInvocation m = (J.MethodInvocation) vd.getVariables().get(0).getInitializer(); - if (m != null && TransformerFactoryFixVisitor.TRANSFORMER_FACTORY_INSTANCE.matches(m)) { - beforeStatement = stBefore; - } - } - } - } - - if (getCursor().getParent() != null && getCursor().getParent().getValue() instanceof J.ClassDeclaration) { - attributeTemplate.insert(0, "{").append("}"); - } - JavaCoordinates insertCoordinates = beforeStatement != null ? - beforeStatement.getCoordinates().before() : - b.getCoordinates().lastStatement(); - b = JavaTemplate - .builder(attributeTemplate.toString()) - .imports("javax.xml.XMLConstants") - .contextSensitive() - .build() - .apply(new Cursor(getCursor().getParent(), b), insertCoordinates); - maybeAddImport("javax.xml.XMLConstants"); + Statement beforeStatement = getInsertStatement(b); + if (b.isScope(getScope())) { + b = updateTemplate(b, block, beforeStatement, Stream.of("javax.xml.XMLConstants").collect(Collectors.toSet())); } return b; } diff --git a/src/main/java/org/openrewrite/java/security/xml/XmlFactoryInsertPropertyStatementVisitor.java b/src/main/java/org/openrewrite/java/security/xml/XmlFactoryInsertPropertyStatementVisitor.java index 5be1b61..362e825 100644 --- a/src/main/java/org/openrewrite/java/security/xml/XmlFactoryInsertPropertyStatementVisitor.java +++ b/src/main/java/org/openrewrite/java/security/xml/XmlFactoryInsertPropertyStatementVisitor.java @@ -1,5 +1,5 @@ /* - * Copyright 2022 the original author or authors. + * Copyright 2023 the original author or authors. *

* Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -15,12 +15,8 @@ */ package org.openrewrite.java.security.xml; -import org.openrewrite.Cursor; -import org.openrewrite.java.JavaIsoVisitor; -import org.openrewrite.java.JavaTemplate; import org.openrewrite.java.VariableNameUtils; import org.openrewrite.java.tree.J; -import org.openrewrite.java.tree.JavaCoordinates; import org.openrewrite.java.tree.Statement; import java.util.Collections; @@ -28,13 +24,10 @@ import java.util.Set; import java.util.stream.Collectors; -class XmlFactoryInsertPropertyStatementVisitor

extends JavaIsoVisitor

{ - private final J.Block scope; - private final StringBuilder propertyTemplate = new StringBuilder(); +class XmlFactoryInsertPropertyStatementVisitor

extends XmlFactoryInsertVisitor

{ private final ExternalDTDAccumulator acc; private final boolean generateAllowList; - private final String xmlFactoryVariableName; public XmlFactoryInsertPropertyStatementVisitor( J.Block scope, @@ -46,21 +39,26 @@ public XmlFactoryInsertPropertyStatementVisitor( boolean needsResolverMethod, ExternalDTDAccumulator acc ) { - this.scope = scope; + super( + scope, + new StringBuilder(), + factoryVariableName, + XmlInputFactoryFixVisitor.XML_PARSER_FACTORY_INSTANCE, + XmlInputFactoryFixVisitor.XML_PARSER_FACTORY_SET_PROPERTY + ); this.acc = acc; - this.xmlFactoryVariableName = factoryVariableName; if (needsExternalEntitiesDisabled) { - propertyTemplate.append(xmlFactoryVariableName).append(".setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false);"); + getTemplate().append(getFactoryVariableName()).append(".setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false);"); } if (needsSupportDTDFalse && accIsEmpty) { if (needsSupportDTDTrue) { - propertyTemplate.append(xmlFactoryVariableName).append(".setProperty(XMLInputFactory.SUPPORT_DTD, false);"); + getTemplate().append(getFactoryVariableName()).append(".setProperty(XMLInputFactory.SUPPORT_DTD, false);"); } } if (needsSupportDTDFalse && !accIsEmpty) { if (needsResolverMethod && needsSupportDTDTrue) { - propertyTemplate.append(xmlFactoryVariableName).append(".setProperty(XMLInputFactory.SUPPORT_DTD, true);"); + getTemplate().append(getFactoryVariableName()).append(".setProperty(XMLInputFactory.SUPPORT_DTD, true);"); } this.generateAllowList = needsResolverMethod; } else if (!needsSupportDTDTrue && !accIsEmpty) { @@ -86,12 +84,12 @@ private Set addAllowList(boolean generateAllowList) { if (acc.getExternalDTDs().size() > 1) { imports.add("java.util.Arrays"); - propertyTemplate.append( + getTemplate().append( "Collection" + newAllowListVariableName + " = Arrays.asList(\n" ); } else { imports.add("java.util.Collections"); - propertyTemplate.append( + getTemplate().append( "Collection" + newAllowListVariableName + " = Collections.singleton(\n" ); } @@ -101,8 +99,8 @@ private Set addAllowList(boolean generateAllowList) { "\t", "" )); - propertyTemplate.append(allowListContent).append("\n);\n"); - propertyTemplate.append(xmlFactoryVariableName).append( + getTemplate().append(allowListContent).append("\n);\n"); + getTemplate().append(getFactoryVariableName()).append( ".setXMLResolver((publicID, systemID, baseURI, namespace) -> {\n" + " if (" + newAllowListVariableName + ".contains(systemID)){\n" + " // returning null will cause the parser to resolve the entity\n" + @@ -117,44 +115,10 @@ private Set addAllowList(boolean generateAllowList) { @Override public J.Block visitBlock(J.Block block, P ctx) { J.Block b = super.visitBlock(block, ctx); - Statement beforeStatement = null; - if (b.isScope(scope)) { - for (int i = b.getStatements().size() - 2; i > -1; i--) { - Statement st = b.getStatements().get(i); - Statement stBefore = b.getStatements().get(i + 1); - if (st instanceof J.MethodInvocation) { - J.MethodInvocation m = (J.MethodInvocation) st; - if (XmlInputFactoryFixVisitor.XML_PARSER_FACTORY_INSTANCE.matches(m) || XmlInputFactoryFixVisitor.XML_PARSER_FACTORY_SET_PROPERTY.matches(m)) { - beforeStatement = stBefore; - } - } else if (st instanceof J.VariableDeclarations) { - J.VariableDeclarations vd = (J.VariableDeclarations) st; - if (vd.getVariables().get(0).getInitializer() instanceof J.MethodInvocation) { - J.MethodInvocation m = (J.MethodInvocation) vd.getVariables().get(0).getInitializer(); - if (m != null && XmlInputFactoryFixVisitor.XML_PARSER_FACTORY_INSTANCE.matches(m)) { - beforeStatement = stBefore; - } - } - } - } - + Statement beforeStatement = getInsertStatement(b); + if (b.isScope(getScope())) { Set imports = addAllowList(generateAllowList); - - if (getCursor().getParent() != null && getCursor().getParent().getValue() instanceof J.ClassDeclaration) { - propertyTemplate.insert(0, "{\n").append("}"); - } - JavaCoordinates insertCoordinates = beforeStatement != null ? - beforeStatement.getCoordinates().before() : - b.getCoordinates().lastStatement(); - b = JavaTemplate - .builder(propertyTemplate.toString()) - .imports(imports.toArray(new String[0])) - .contextSensitive() - .build() - .apply(new Cursor(getCursor().getParent(), b), insertCoordinates); - if (b != block) { - imports.forEach(this::maybeAddImport); - } + b = updateTemplate(b, block, beforeStatement, imports); } return b; } diff --git a/src/main/java/org/openrewrite/java/security/xml/XmlFactoryInsertVisitor.java b/src/main/java/org/openrewrite/java/security/xml/XmlFactoryInsertVisitor.java new file mode 100644 index 0000000..e6e5e9b --- /dev/null +++ b/src/main/java/org/openrewrite/java/security/xml/XmlFactoryInsertVisitor.java @@ -0,0 +1,83 @@ +/* + * Copyright 2023 the original author or authors. + *

+ * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.openrewrite.java.security.xml; + +import lombok.AllArgsConstructor; +import lombok.Getter; +import org.openrewrite.Cursor; +import org.openrewrite.java.JavaIsoVisitor; +import org.openrewrite.java.JavaTemplate; +import org.openrewrite.java.MethodMatcher; +import org.openrewrite.java.tree.J; +import org.openrewrite.java.tree.JavaCoordinates; +import org.openrewrite.java.tree.Statement; + +import java.util.Set; + +@AllArgsConstructor +@Getter +public abstract class XmlFactoryInsertVisitor

extends JavaIsoVisitor

{ + private final J.Block scope; + private final StringBuilder template; + private final String factoryVariableName; + private final MethodMatcher factoryInstanceMatcher; + private final MethodMatcher factoryMethodCallMatcher; + + public Statement getInsertStatement(J.Block b) { + Statement beforeStatement = null; + if (b.isScope(scope)) { + for (int i = b.getStatements().size() - 2; i > -1; i--) { + Statement st = b.getStatements().get(i); + Statement stBefore = b.getStatements().get(i + 1); + if (st instanceof J.MethodInvocation) { + J.MethodInvocation m = (J.MethodInvocation) st; + if (factoryInstanceMatcher.matches(m) || factoryMethodCallMatcher.matches(m)) { + beforeStatement = stBefore; + } + } else if (st instanceof J.VariableDeclarations) { + J.VariableDeclarations vd = (J.VariableDeclarations) st; + if (vd.getVariables().get(0).getInitializer() instanceof J.MethodInvocation) { + J.MethodInvocation m = (J.MethodInvocation) vd.getVariables().get(0).getInitializer(); + if (m != null && factoryInstanceMatcher.matches(m)) { + beforeStatement = stBefore; + } + } + } + } + } + return beforeStatement; + } + + private JavaCoordinates getInsertCoordinates(J.Block b, Statement s) { + return s != null ? s.getCoordinates().before() : b.getCoordinates().lastStatement(); + } + + public J.Block updateTemplate(J.Block b, J.Block block, Statement beforeStatement, Set imports) { + if (getCursor().getParent() != null && getCursor().getParent().getValue() instanceof J.ClassDeclaration) { + template.insert(0, "{\n").append("}"); + } + b = JavaTemplate + .builder(template.toString()) + .imports(imports.toArray(new String[0])) + .contextSensitive() + .build() + .apply(new Cursor(getCursor().getParent(), b), getInsertCoordinates(b, beforeStatement)); + if (b != block) { + imports.forEach(this::maybeAddImport); + } + return b; + } +} diff --git a/src/main/java/org/openrewrite/java/security/xml/XmlFactoryVisitor.java b/src/main/java/org/openrewrite/java/security/xml/XmlFactoryVisitor.java new file mode 100644 index 0000000..14f9b09 --- /dev/null +++ b/src/main/java/org/openrewrite/java/security/xml/XmlFactoryVisitor.java @@ -0,0 +1,54 @@ +/* + * Copyright 2023 the original author or authors. + *

+ * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.openrewrite.java.security.xml; + +import lombok.AllArgsConstructor; +import lombok.Getter; +import org.openrewrite.java.JavaIsoVisitor; +import org.openrewrite.java.MethodMatcher; +import org.openrewrite.java.tree.J; +import org.openrewrite.java.tree.TypeUtils; + +@AllArgsConstructor +@Getter +public abstract class XmlFactoryVisitor

extends JavaIsoVisitor

{ + private final MethodMatcher FACTORY_INSTANCE; + + private final String FACTORY_FQN; + + private final String FACTORY_INITIALIZATION_METHOD; + private final String FACTORY_VARIABLE_NAME; + + private final ExternalDTDAccumulator acc; + + @Override + public J.VariableDeclarations.NamedVariable visitVariable(J.VariableDeclarations.NamedVariable variable, P ctx) { + J.VariableDeclarations.NamedVariable v = super.visitVariable(variable, ctx); + if (TypeUtils.isOfClassType(v.getType(), FACTORY_FQN)) { + getCursor().putMessageOnFirstEnclosing(J.ClassDeclaration.class, FACTORY_VARIABLE_NAME, v.getSimpleName()); + } + return v; + } + + @Override + public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, P ctx) { + J.MethodInvocation m = super.visitMethodInvocation(method, ctx); + if (FACTORY_INSTANCE.matches(m)) { + getCursor().putMessageOnFirstEnclosing(J.ClassDeclaration.class, FACTORY_INITIALIZATION_METHOD, getCursor().dropParentUntil(J.Block.class::isInstance)); + } + return m; + } +} diff --git a/src/main/java/org/openrewrite/java/security/xml/XmlInputFactoryFixVisitor.java b/src/main/java/org/openrewrite/java/security/xml/XmlInputFactoryFixVisitor.java index 30beeba..a118b2d 100644 --- a/src/main/java/org/openrewrite/java/security/xml/XmlInputFactoryFixVisitor.java +++ b/src/main/java/org/openrewrite/java/security/xml/XmlInputFactoryFixVisitor.java @@ -1,5 +1,5 @@ /* - * Copyright 2022 the original author or authors. + * Copyright 2023 the original author or authors. *

* Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -15,12 +15,10 @@ */ package org.openrewrite.java.security.xml; -import lombok.AllArgsConstructor; import org.openrewrite.Cursor; import org.openrewrite.ExecutionContext; import org.openrewrite.Preconditions; import org.openrewrite.TreeVisitor; -import org.openrewrite.java.JavaIsoVisitor; import org.openrewrite.java.MethodMatcher; import org.openrewrite.java.search.UsesType; import org.openrewrite.java.tree.J; @@ -28,8 +26,7 @@ import javax.xml.stream.XMLInputFactory; -@AllArgsConstructor -public class XmlInputFactoryFixVisitor

extends JavaIsoVisitor

{ +public class XmlInputFactoryFixVisitor

extends XmlFactoryVisitor

{ static final MethodMatcher XML_PARSER_FACTORY_INSTANCE = new MethodMatcher("javax.xml.stream.XMLInputFactory new*()"); static final MethodMatcher XML_PARSER_FACTORY_SET_PROPERTY = new MethodMatcher("javax.xml.stream.XMLInputFactory setProperty(java.lang.String, ..)"); @@ -45,10 +42,18 @@ public class XmlInputFactoryFixVisitor

extends JavaIsoVisitor

{ private static final String XML_RESOLVER_METHOD = "xml-resolver-initialization-method"; - private final ExternalDTDAccumulator acc; + public XmlInputFactoryFixVisitor(ExternalDTDAccumulator acc) { + super( + XML_PARSER_FACTORY_INSTANCE, + XML_FACTORY_FQN, + XML_PARSER_INITIALIZATION_METHOD, + XML_FACTORY_VARIABLE_NAME, + acc + ); + } + @Override public J.ClassDeclaration visitClassDeclaration(J.ClassDeclaration classDecl, P ctx) { - J.ClassDeclaration cd = super.visitClassDeclaration(classDecl, ctx); Cursor supportsExternalCursor = getCursor().getMessage(SUPPORTING_EXTERNAL_ENTITIES_PROPERTY_NAME); Cursor supportsFalseDTDCursor = getCursor().getMessage(SUPPORT_DTD_FALSE_PROPERTY_NAME); @@ -60,7 +65,6 @@ public J.ClassDeclaration visitClassDeclaration(J.ClassDeclaration classDecl, P Cursor setPropertyBlockCursor = null; if (supportsExternalCursor == null && supportsFalseDTDCursor == null) { setPropertyBlockCursor = initializationCursor; - } else if (supportsExternalCursor == null ^ supportsFalseDTDCursor == null) { setPropertyBlockCursor = supportsExternalCursor == null ? supportsFalseDTDCursor : supportsExternalCursor; } @@ -70,30 +74,19 @@ public J.ClassDeclaration visitClassDeclaration(J.ClassDeclaration classDecl, P xmlFactoryVariableName, supportsExternalCursor == null, supportsFalseDTDCursor == null, - acc.getExternalDTDs().isEmpty(), + getAcc().getExternalDTDs().isEmpty(), supportsDTDTrueCursor == null, xmlResolverMethod == null, - acc + getAcc() )); } return cd; } - @Override - public J.VariableDeclarations.NamedVariable visitVariable(J.VariableDeclarations.NamedVariable variable, P ctx) { - J.VariableDeclarations.NamedVariable v = super.visitVariable(variable, ctx); - if (TypeUtils.isOfClassType(v.getType(), XML_FACTORY_FQN)) { - getCursor().putMessageOnFirstEnclosing(J.ClassDeclaration.class, XML_FACTORY_VARIABLE_NAME, v.getSimpleName()); - } - return v; - } - @Override public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, P ctx) { J.MethodInvocation m = super.visitMethodInvocation(method, ctx); - if (XML_PARSER_FACTORY_INSTANCE.matches(m)) { - getCursor().putMessageOnFirstEnclosing(J.ClassDeclaration.class, XML_PARSER_INITIALIZATION_METHOD, getCursor().dropParentUntil(J.Block.class::isInstance)); - } else if (XML_PARSER_FACTORY_SET_PROPERTY.matches(m) && m.getArguments().get(0) instanceof J.FieldAccess) { + if (XML_PARSER_FACTORY_SET_PROPERTY.matches(m) && m.getArguments().get(0) instanceof J.FieldAccess) { J.FieldAccess fa = (J.FieldAccess) m.getArguments().get(0); if (SUPPORTING_EXTERNAL_ENTITIES_PROPERTY_NAME.equals(fa.getSimpleName())) { getCursor().putMessageOnFirstEnclosing(J.ClassDeclaration.class, SUPPORTING_EXTERNAL_ENTITIES_PROPERTY_NAME, getCursor().dropParentUntil(J.Block.class::isInstance)); @@ -113,8 +106,6 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, P ctx getCursor().putMessageOnFirstEnclosing(J.ClassDeclaration.class, XML_RESOLVER_METHOD, getCursor().dropParentUntil((J.Block.class::isInstance))); } return m; - - } private void checkDTDSupport(J.MethodInvocation m) { diff --git a/src/test/java/org/openrewrite/java/security/xml/XmlInputFactoryXXEVulnerabilityTest.java b/src/test/java/org/openrewrite/java/security/xml/XmlInputFactoryXXEVulnerabilityTest.java index a31ed29..f8b84d5 100644 --- a/src/test/java/org/openrewrite/java/security/xml/XmlInputFactoryXXEVulnerabilityTest.java +++ b/src/test/java/org/openrewrite/java/security/xml/XmlInputFactoryXXEVulnerabilityTest.java @@ -1,5 +1,5 @@ /* - * Copyright 2021 the original author or authors. + * Copyright 2023 the original author or authors. *

* Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. From edbfdaf3548f5b95392c551efde4d938211ce7ce Mon Sep 17 00:00:00 2001 From: Aaron Blume Date: Thu, 17 Aug 2023 15:59:00 -0700 Subject: [PATCH 2/3] Rename method and code cleanup --- .../TransformerFactoryInsertAttributeStatementVisitor.java | 6 ++---- .../xml/XmlFactoryInsertPropertyStatementVisitor.java | 3 +-- .../java/security/xml/XmlFactoryInsertVisitor.java | 4 ++-- 3 files changed, 5 insertions(+), 8 deletions(-) diff --git a/src/main/java/org/openrewrite/java/security/xml/TransformerFactoryInsertAttributeStatementVisitor.java b/src/main/java/org/openrewrite/java/security/xml/TransformerFactoryInsertAttributeStatementVisitor.java index f57289b..ea40cf3 100644 --- a/src/main/java/org/openrewrite/java/security/xml/TransformerFactoryInsertAttributeStatementVisitor.java +++ b/src/main/java/org/openrewrite/java/security/xml/TransformerFactoryInsertAttributeStatementVisitor.java @@ -18,8 +18,7 @@ import org.openrewrite.java.tree.J; import org.openrewrite.java.tree.Statement; -import java.util.stream.Collectors; -import java.util.stream.Stream; +import java.util.Collections; public class TransformerFactoryInsertAttributeStatementVisitor

extends XmlFactoryInsertVisitor

{ public TransformerFactoryInsertAttributeStatementVisitor( @@ -31,7 +30,6 @@ public TransformerFactoryInsertAttributeStatementVisitor( ) { super( scope, - new StringBuilder(), factoryVariableName, TransformerFactoryFixVisitor.TRANSFORMER_FACTORY_INSTANCE, TransformerFactoryFixVisitor.TRANSFORMER_FACTORY_SET_ATTRIBUTE @@ -53,7 +51,7 @@ public J.Block visitBlock(J.Block block, P ctx) { J.Block b = super.visitBlock(block, ctx); Statement beforeStatement = getInsertStatement(b); if (b.isScope(getScope())) { - b = updateTemplate(b, block, beforeStatement, Stream.of("javax.xml.XMLConstants").collect(Collectors.toSet())); + b = updateBlock(b, block, beforeStatement, Collections.singleton("javax.xml.XMLConstants")); } return b; } diff --git a/src/main/java/org/openrewrite/java/security/xml/XmlFactoryInsertPropertyStatementVisitor.java b/src/main/java/org/openrewrite/java/security/xml/XmlFactoryInsertPropertyStatementVisitor.java index 362e825..be33a57 100644 --- a/src/main/java/org/openrewrite/java/security/xml/XmlFactoryInsertPropertyStatementVisitor.java +++ b/src/main/java/org/openrewrite/java/security/xml/XmlFactoryInsertPropertyStatementVisitor.java @@ -41,7 +41,6 @@ public XmlFactoryInsertPropertyStatementVisitor( ) { super( scope, - new StringBuilder(), factoryVariableName, XmlInputFactoryFixVisitor.XML_PARSER_FACTORY_INSTANCE, XmlInputFactoryFixVisitor.XML_PARSER_FACTORY_SET_PROPERTY @@ -118,7 +117,7 @@ public J.Block visitBlock(J.Block block, P ctx) { Statement beforeStatement = getInsertStatement(b); if (b.isScope(getScope())) { Set imports = addAllowList(generateAllowList); - b = updateTemplate(b, block, beforeStatement, imports); + b = updateBlock(b, block, beforeStatement, imports); } return b; } diff --git a/src/main/java/org/openrewrite/java/security/xml/XmlFactoryInsertVisitor.java b/src/main/java/org/openrewrite/java/security/xml/XmlFactoryInsertVisitor.java index e6e5e9b..3d84f34 100644 --- a/src/main/java/org/openrewrite/java/security/xml/XmlFactoryInsertVisitor.java +++ b/src/main/java/org/openrewrite/java/security/xml/XmlFactoryInsertVisitor.java @@ -30,8 +30,8 @@ @AllArgsConstructor @Getter public abstract class XmlFactoryInsertVisitor

extends JavaIsoVisitor

{ + private final StringBuilder template = new StringBuilder(); private final J.Block scope; - private final StringBuilder template; private final String factoryVariableName; private final MethodMatcher factoryInstanceMatcher; private final MethodMatcher factoryMethodCallMatcher; @@ -65,7 +65,7 @@ private JavaCoordinates getInsertCoordinates(J.Block b, Statement s) { return s != null ? s.getCoordinates().before() : b.getCoordinates().lastStatement(); } - public J.Block updateTemplate(J.Block b, J.Block block, Statement beforeStatement, Set imports) { + public J.Block updateBlock(J.Block b, J.Block block, Statement beforeStatement, Set imports) { if (getCursor().getParent() != null && getCursor().getParent().getValue() instanceof J.ClassDeclaration) { template.insert(0, "{\n").append("}"); } From 458c3ffbae9e9848fc937ec5c184cb79eac17956 Mon Sep 17 00:00:00 2001 From: Aaron Blume Date: Fri, 18 Aug 2023 12:06:29 -0700 Subject: [PATCH 3/3] Abstract away common implementation of adding messages --- .../security/xml/TransformerFactoryFixVisitor.java | 12 ++++++------ .../java/security/xml/XmlFactoryVisitor.java | 9 +++++++++ .../java/security/xml/XmlInputFactoryFixVisitor.java | 10 +++++----- 3 files changed, 20 insertions(+), 11 deletions(-) diff --git a/src/main/java/org/openrewrite/java/security/xml/TransformerFactoryFixVisitor.java b/src/main/java/org/openrewrite/java/security/xml/TransformerFactoryFixVisitor.java index c750563..5616b15 100644 --- a/src/main/java/org/openrewrite/java/security/xml/TransformerFactoryFixVisitor.java +++ b/src/main/java/org/openrewrite/java/security/xml/TransformerFactoryFixVisitor.java @@ -93,14 +93,14 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, P ctx J.Literal string = (J.Literal) m.getArguments().get(1); assert string.getValue() != null; if (!(((String) string.getValue()).isEmpty())) { - getCursor().putMessageOnFirstEnclosing(J.ClassDeclaration.class, DISALLOW_MODIFY_FLAG, getCursor().dropParentUntil(J.Block.class::isInstance)); + addMessage(DISALLOW_MODIFY_FLAG); } } J.FieldAccess fa = (J.FieldAccess) m.getArguments().get(0); if (ACCESS_EXTERNAL_DTD_NAME.equals(fa.getSimpleName())) { - getCursor().putMessageOnFirstEnclosing(J.ClassDeclaration.class, ACCESS_EXTERNAL_DTD_NAME, getCursor().dropParentUntil(J.Block.class::isInstance)); + addMessage(ACCESS_EXTERNAL_DTD_NAME); } else if (ACCESS_EXTERNAL_STYLESHEET_NAME.equals(fa.getSimpleName())) { - getCursor().putMessageOnFirstEnclosing(J.ClassDeclaration.class, ACCESS_EXTERNAL_STYLESHEET_NAME, getCursor().dropParentUntil(J.Block.class::isInstance)); + addMessage(ACCESS_EXTERNAL_STYLESHEET_NAME); } } else if (TRANSFORMER_FACTORY_SET_FEATURE.matches(m)) { // If FEATURE_SECURE_PROCESSING is set to false, do not make any changes @@ -108,18 +108,18 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, P ctx J.Literal bool = (J.Literal) m.getArguments().get(1); assert bool.getValue() != null; if (Boolean.FALSE.equals(bool.getValue())) { - getCursor().putMessageOnFirstEnclosing(J.ClassDeclaration.class, DISALLOW_MODIFY_FLAG, getCursor().dropParentUntil(J.Block.class::isInstance)); + addMessage(DISALLOW_MODIFY_FLAG); } } if (m.getArguments().get(0) instanceof J.FieldAccess) { J.FieldAccess fa = (J.FieldAccess) m.getArguments().get(0); if (FEATURE_SECURE_PROCESSING_NAME.equals(fa.getSimpleName())) { - getCursor().putMessageOnFirstEnclosing(J.ClassDeclaration.class, FEATURE_SECURE_PROCESSING_NAME, getCursor().dropParentUntil(J.Block.class::isInstance)); + addMessage(FEATURE_SECURE_PROCESSING_NAME); } } else if (m.getArguments().get(0) instanceof J.Literal) { J.Literal literal = (J.Literal) m.getArguments().get(0); if (XMLConstants.FEATURE_SECURE_PROCESSING.equals(literal.getValue())) { - getCursor().putMessageOnFirstEnclosing(J.ClassDeclaration.class, FEATURE_SECURE_PROCESSING_NAME, getCursor().dropParentUntil(J.Block.class::isInstance)); + addMessage(FEATURE_SECURE_PROCESSING_NAME); } } } diff --git a/src/main/java/org/openrewrite/java/security/xml/XmlFactoryVisitor.java b/src/main/java/org/openrewrite/java/security/xml/XmlFactoryVisitor.java index 14f9b09..f3c5386 100644 --- a/src/main/java/org/openrewrite/java/security/xml/XmlFactoryVisitor.java +++ b/src/main/java/org/openrewrite/java/security/xml/XmlFactoryVisitor.java @@ -51,4 +51,13 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, P ctx } return m; } + + /** + * Adds a message/flag on the first enclosing class instance. + * + * @param message The message to be added. + */ + public void addMessage(String message) { + getCursor().putMessageOnFirstEnclosing(J.ClassDeclaration.class, message, getCursor().dropParentUntil(J.Block.class::isInstance)); + } } diff --git a/src/main/java/org/openrewrite/java/security/xml/XmlInputFactoryFixVisitor.java b/src/main/java/org/openrewrite/java/security/xml/XmlInputFactoryFixVisitor.java index a118b2d..f56af5a 100644 --- a/src/main/java/org/openrewrite/java/security/xml/XmlInputFactoryFixVisitor.java +++ b/src/main/java/org/openrewrite/java/security/xml/XmlInputFactoryFixVisitor.java @@ -89,7 +89,7 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, P ctx if (XML_PARSER_FACTORY_SET_PROPERTY.matches(m) && m.getArguments().get(0) instanceof J.FieldAccess) { J.FieldAccess fa = (J.FieldAccess) m.getArguments().get(0); if (SUPPORTING_EXTERNAL_ENTITIES_PROPERTY_NAME.equals(fa.getSimpleName())) { - getCursor().putMessageOnFirstEnclosing(J.ClassDeclaration.class, SUPPORTING_EXTERNAL_ENTITIES_PROPERTY_NAME, getCursor().dropParentUntil(J.Block.class::isInstance)); + addMessage(SUPPORTING_EXTERNAL_ENTITIES_PROPERTY_NAME); } else if (SUPPORT_DTD_FALSE_PROPERTY_NAME.equals(fa.getSimpleName())) { checkDTDSupport(m); } @@ -97,13 +97,13 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, P ctx J.Literal literal = (J.Literal) m.getArguments().get(0); if (TypeUtils.isString(literal.getType())) { if (XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES.equals(literal.getValue())) { - getCursor().putMessageOnFirstEnclosing(J.ClassDeclaration.class, SUPPORTING_EXTERNAL_ENTITIES_PROPERTY_NAME, getCursor().dropParentUntil(J.Block.class::isInstance)); + addMessage(SUPPORTING_EXTERNAL_ENTITIES_PROPERTY_NAME); } else if (XMLInputFactory.SUPPORT_DTD.equals(literal.getValue())) { checkDTDSupport(m); } } } else if (XML_PARSER_FACTORY_SET_RESOLVER.matches(m)) { - getCursor().putMessageOnFirstEnclosing(J.ClassDeclaration.class, XML_RESOLVER_METHOD, getCursor().dropParentUntil((J.Block.class::isInstance))); + addMessage(XML_RESOLVER_METHOD); } return m; } @@ -112,9 +112,9 @@ private void checkDTDSupport(J.MethodInvocation m) { if (m.getArguments().get(1) instanceof J.Literal) { J.Literal literal = (J.Literal) m.getArguments().get(1); if (Boolean.TRUE.equals(literal.getValue())) { - getCursor().putMessageOnFirstEnclosing(J.ClassDeclaration.class, SUPPORT_DTD_TRUE_PROPERTY_NAME, getCursor().dropParentUntil(J.Block.class::isInstance)); + addMessage(SUPPORT_DTD_TRUE_PROPERTY_NAME); } else { - getCursor().putMessageOnFirstEnclosing(J.ClassDeclaration.class, SUPPORT_DTD_FALSE_PROPERTY_NAME, getCursor().dropParentUntil(J.Block.class::isInstance)); + addMessage(SUPPORT_DTD_FALSE_PROPERTY_NAME); } } }