Skip to content

Commit

Permalink
added support for cases when one property was set but the rest weren'…
Browse files Browse the repository at this point in the history 
…t. Expand Entity References support still pending. (#106)
  • Loading branch information
@Saumyanavani
Saumyanavani authored Sep 1, 2023
1 parent ab28b8d commit 47c6514
Show file tree
Hide file tree
Showing 3 changed files with 142 additions and 16 deletions.
48 changes: 35 additions & 13 deletions src/main/java/org/openrewrite/java/security/xml/DBFInsertPropertyStatementVisitor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ public class DBFInsertPropertyStatementVisitor<P> extends XmlFactoryInsertVisito
private final boolean disallowGeneralEntities;
private final boolean disallowParameterEntities;
private final boolean disallowLoadExternalDTD;
private final boolean setXIncludeAware;

public DBFInsertPropertyStatementVisitor(
J.Block scope,
Expand All @@ -35,8 +36,9 @@ public DBFInsertPropertyStatementVisitor(
boolean needsDisallowDoctypesTrue,
boolean needsDisableGeneralEntities,
boolean needsDisableParameterEntities,
boolean needsLoadExternalDTD
) {
boolean needsLoadExternalDTD,
boolean needsSetXIncludeAware,
boolean needsSetExpandEntityReferences) {
super(
scope,
dbfFactoryVariable,
Expand All @@ -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" +
Expand All @@ -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"
);
}
}
}
}
29 changes: 26 additions & 3 deletions src/main/java/org/openrewrite/java/security/xml/DocumentBuilderFactoryFixVisitor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@ public class DocumentBuilderFactoryFixVisitor<P> extends XmlFactoryVisitor<P> {

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";
Expand Down Expand Up @@ -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);
Expand All @@ -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) {
Expand All @@ -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;
Expand Down
81 changes: 81 additions & 0 deletions src/test/java/org/openrewrite/java/security/xml/DocumentBuilderFactoryXXETest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -277,4 +277,85 @@ class myDBFReader {
)
);
}

@Test
void factoryIsNotVulnerableWithDTDsAndXIncludeAwareButPropertiesMissing() {
rewriteRun(
xml(
"""
<!DOCTYPE xml [
<!ENTITY open-hatch-system
SYSTEM "http://www.textuality.com/boilerplate/OpenHatch.xml">
<!ENTITY open-hatch-public
PUBLIC "-//Textuality//TEXT Standard open-hatch boilerplate//EN"
"http://www.texty.com/boilerplate/OpenHatch.xml">
<!ENTITY hatch-pic
SYSTEM "../grafix/OpenHatch.gif"
NDATA gif>
]>
<root>
<!-- Your XML content here -->
</root>
"""
),
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();
}
"""
)
);

}
}

0 comments on commit 47c6514

Please sign in to comment.