Skip to content

Commit 3c591cd

Browse files
Use centralized XML parser config - backport (#7751)
#### Rationale Backport changes already in develop to help configure XML parsers consistently #### Changes - Configure parsers for security #### Tasks 📍 - [x] Claude Code Review - ~Manual Testing~ - ~Test Automation~
1 parent 49800b1 commit 3c591cd

1 file changed

Lines changed: 57 additions & 14 deletions

File tree

api/src/org/labkey/api/util/XmlBeansUtil.java

Lines changed: 57 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
import org.labkey.api.settings.LookAndFeelProperties;
2929
import org.xml.sax.SAXException;
3030

31+
import javax.xml.XMLConstants;
3132
import javax.xml.parsers.DocumentBuilderFactory;
3233
import javax.xml.parsers.ParserConfigurationException;
3334
import javax.xml.parsers.SAXParserFactory;
@@ -121,36 +122,78 @@ public static void addComment(XmlTokenSource doc, String comment)
121122
cursor.dispose();
122123
}
123124

124-
/** XML parsing factories preconfigured to prevent XML external entity references (XXE) */
125+
/**
126+
* XML parsing factories preconfigured to prevent XML external entity references (XXE).
127+
* These are static and are unfortunately mutable. We could switch to a factory pattern to create
128+
* freshly configured factories.
129+
*/
125130
public static final SAXParserFactory SAX_PARSER_FACTORY;
131+
public static final SAXParserFactory SAX_PARSER_FACTORY_ALLOWING_DOCTYPE;
126132
public static final XMLInputFactory XML_INPUT_FACTORY;
127133
public static final DocumentBuilderFactory DOCUMENT_BUILDER_FACTORY;
134+
public static final DocumentBuilderFactory DOCUMENT_BUILDER_FACTORY_ALLOWING_DOCTYPE;
128135

129136
static
130137
{
138+
//noinspection XMLInputFactory
131139
XML_INPUT_FACTORY = XMLInputFactory.newInstance();
132140
XML_INPUT_FACTORY.setProperty(XMLInputFactory.SUPPORT_DTD, false);
133141
XML_INPUT_FACTORY.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false);
134142

135-
SAX_PARSER_FACTORY = SAXParserFactory.newInstance();
136143
try
137144
{
138-
SAX_PARSER_FACTORY.setNamespaceAware(true);
139-
SAX_PARSER_FACTORY.setFeature("http://xml.org/sax/features/validation", false);
140-
SAX_PARSER_FACTORY.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
141-
SAX_PARSER_FACTORY.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
142-
143-
DOCUMENT_BUILDER_FACTORY = DocumentBuilderFactory.newInstance();
144-
DOCUMENT_BUILDER_FACTORY.setNamespaceAware(true);
145-
DOCUMENT_BUILDER_FACTORY.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
146-
DOCUMENT_BUILDER_FACTORY.setFeature("http://xml.org/sax/features/external-general-entities", false);
147-
DOCUMENT_BUILDER_FACTORY.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
148-
DOCUMENT_BUILDER_FACTORY.setXIncludeAware(false);
149-
DOCUMENT_BUILDER_FACTORY.setExpandEntityReferences(false);
145+
SAX_PARSER_FACTORY = saxParserFactory(false);
146+
SAX_PARSER_FACTORY_ALLOWING_DOCTYPE = saxParserFactory(true);
147+
148+
DOCUMENT_BUILDER_FACTORY = documentBuilderFactory(false);
149+
// Use the ALLOWING_DOCTYPE variant when parsing XML that contains a <!DOCTYPE> declaration (e.g. NCBI's eSummary responses)
150+
DOCUMENT_BUILDER_FACTORY_ALLOWING_DOCTYPE = documentBuilderFactory(true);
150151
}
151152
catch (ParserConfigurationException | SAXException e)
152153
{
153154
throw UnexpectedException.wrap(e);
154155
}
155156
}
157+
158+
private static SAXParserFactory saxParserFactory(boolean allowDocType) throws SAXException, ParserConfigurationException
159+
{
160+
//noinspection XMLInputFactory
161+
SAXParserFactory result = SAXParserFactory.newInstance();
162+
result.setNamespaceAware(true);
163+
result.setFeature("http://xml.org/sax/features/validation", false);
164+
result.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
165+
166+
// Disable features that could lead to XXE or other vulnerabilities
167+
// Keep in sync with ModuleArchive.nameFromModuleXML()
168+
if (!allowDocType)
169+
{
170+
result.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
171+
}
172+
result.setFeature("http://xml.org/sax/features/external-general-entities", false);
173+
result.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
174+
result.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
175+
return result;
176+
}
177+
178+
private static DocumentBuilderFactory documentBuilderFactory(boolean allowDocType) throws ParserConfigurationException
179+
{
180+
//noinspection XMLInputFactory
181+
DocumentBuilderFactory result = DocumentBuilderFactory.newInstance();
182+
result.setNamespaceAware(true);
183+
184+
// Disable features that could lead to XXE or other vulnerabilities.
185+
// When allowDocType is true the DOCTYPE declaration is permitted. External entity
186+
// resolution remains disabled, so XXE protection is still in effect.
187+
if (!allowDocType)
188+
{
189+
result.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
190+
}
191+
result.setFeature("http://xml.org/sax/features/external-general-entities", false);
192+
result.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
193+
result.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
194+
result.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
195+
result.setXIncludeAware(false);
196+
result.setExpandEntityReferences(false);
197+
return result;
198+
}
156199
}

0 commit comments

Comments
 (0)