Skip to content

Commit a46fa57

Browse files
sbrunnersbrunner
authored
Merge pull request #4212 from mapfish/mapfish-print-GHSA-5v29-34h8-v68r
Harden the code, see commits
2 parents 93ecaf9 + 2737527 commit a46fa57

6 files changed

Lines changed: 199 additions & 14 deletions

File tree

‎core/src/main/java/org/mapfish/print/map/geotools/FeaturesParser.java‎

Lines changed: 45 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,50 @@ static CoordinateReferenceSystem parseCoordinateReferenceSystem(
137137
return crs;
138138
}
139139

140+
@VisibleForTesting
141+
static CoordinateReferenceSystem parseCoordinateReferenceSystem(
142+
final JSONObject geojson, final boolean forceLongitudeFirst) {
143+
CoordinateReferenceSystem crs = DefaultEngineeringCRS.GENERIC_2D;
144+
StringBuilder code = new StringBuilder();
145+
try {
146+
if (geojson.has("crs")) {
147+
JSONObject crsJson = geojson.getJSONObject("crs");
148+
String type = crsJson.optString("type", "");
149+
150+
if (type.equalsIgnoreCase("EPSG") || type.equalsIgnoreCase("CRS")) {
151+
code.append(type);
152+
String propCode = getProperty(crsJson, "code");
153+
if (propCode != null) {
154+
code.append(":").append(propCode);
155+
}
156+
} else if (type.equalsIgnoreCase("name")) {
157+
String propCode = getProperty(crsJson, "name");
158+
if (propCode != null) {
159+
code.append(propCode);
160+
}
161+
} else if (!type.equals("link")) {
162+
String propCode = getProperty(crsJson, "code");
163+
if (propCode != null) {
164+
code.append(propCode);
165+
}
166+
}
167+
}
168+
} catch (JSONException e) {
169+
LOGGER.warn(
170+
"Error reading the required elements to parse crs of the geojson: \n{}", geojson, e);
171+
}
172+
try {
173+
if (!code.isEmpty()) {
174+
crs = CRS.decode(code.toString(), forceLongitudeFirst);
175+
}
176+
} catch (NoSuchAuthorityCodeException e) {
177+
LOGGER.warn("No CRS with code: {}.\nRead from geojson: \n{}", code, geojson);
178+
} catch (FactoryException e) {
179+
LOGGER.warn("Error loading CRS with code: {}.\nRead from geojson: \n{}", code, geojson);
180+
}
181+
return crs;
182+
}
183+
140184
private static String getProperty(final JSONObject crsJson, final String nameCode)
141185
throws JSONException {
142186
if (crsJson.has("properties")) {
@@ -274,7 +318,7 @@ private SimpleFeatureType createFeatureType(@Nonnull final String geojsonData) {
274318
return null;
275319
}
276320
} catch (JSONException e) {
277-
throw new PrintException("Invalid geoJSON: \n" + geojsonData + ": " + e.getMessage(), e);
321+
throw new PrintException("Invalid GeoJSON data", e);
278322
}
279323
}
280324

‎core/src/main/java/org/mapfish/print/map/geotools/GmlLayer.java‎

Lines changed: 52 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,18 @@
11
package org.mapfish.print.map.geotools;
22

33
import jakarta.annotation.Nonnull;
4+
import java.io.ByteArrayInputStream;
45
import java.io.IOException;
56
import java.io.StringReader;
67
import java.net.MalformedURLException;
78
import java.net.URI;
89
import java.net.URISyntaxException;
910
import java.net.URL;
11+
import java.nio.charset.StandardCharsets;
1012
import java.util.concurrent.ExecutorService;
13+
import javax.xml.XMLConstants;
1114
import javax.xml.namespace.QName;
15+
import javax.xml.parsers.DocumentBuilderFactory;
1216
import javax.xml.parsers.ParserConfigurationException;
1317
import org.eclipse.emf.ecore.resource.URIHandler;
1418
import org.geotools.api.data.FeatureSource;
@@ -25,7 +29,9 @@
2529
import org.mapfish.print.http.MfClientHttpRequestFactory;
2630
import org.mapfish.print.map.AbstractLayerParams;
2731
import org.springframework.beans.factory.annotation.Autowired;
32+
import org.xml.sax.InputSource;
2833
import org.xml.sax.SAXException;
34+
import org.xml.sax.ext.EntityResolver2;
2935

3036
/** Parses GML from the request data. */
3137
public final class GmlLayer extends AbstractFeatureSourceLayer {
@@ -120,6 +126,7 @@ private SimpleFeatureCollection createFeatureSource(
120126
FileUtils.testForLegalFileUrl(template.getConfiguration(), url);
121127
try {
122128
final String gmlData = URIUtils.toString(httpRequestFactory, url.toURI());
129+
validateXmlInput(gmlData);
123130
final int endIndex = 200;
124131
String startOfData = gmlData.substring(0, endIndex);
125132
if (startOfData.contains("\"http://www.opengis.net/gml/3.2\"")) {
@@ -178,17 +185,60 @@ private SimpleFeatureCollection parseGml32(final String gmlData) throws IOExcept
178185
if (featureCollection instanceof SimpleFeatureCollection) {
179186
return (SimpleFeatureCollection) featureCollection;
180187
} else {
181-
throw new RuntimeException("unable to parse gml: \n\n" + gmlData);
188+
throw new RuntimeException("unable to parse GML data");
182189
}
183190

184191
} catch (SAXException | ParserConfigurationException e) {
185-
throw new PrintException("Failed to parse Gml32 " + gmlData, e);
192+
throw new PrintException("Failed to parse GML data", e);
193+
}
194+
}
195+
196+
private void validateXmlInput(final String gmlData) {
197+
try {
198+
final DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
199+
factory.setNamespaceAware(true);
200+
factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
201+
factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
202+
factory.setFeature("http://xml.org/sax/features/external-general-entities", false);
203+
factory.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
204+
factory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
205+
factory.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, "");
206+
factory.setAttribute(XMLConstants.ACCESS_EXTERNAL_SCHEMA, "");
207+
factory.setXIncludeAware(false);
208+
factory.setExpandEntityReferences(false);
209+
210+
factory
211+
.newDocumentBuilder()
212+
.parse(new ByteArrayInputStream(gmlData.getBytes(StandardCharsets.UTF_8)));
213+
} catch (ParserConfigurationException | SAXException | IOException e) {
214+
throw new PrintException("Failed to parse GML data", e);
186215
}
187216
}
188217

189218
private Parser createParser(final Configuration configuration) {
190219
final Parser parser = new Parser(configuration);
191220
parser.getURIHandlers().addFirst(this.cachingUrihandler);
221+
parser.setEntityResolver(
222+
new EntityResolver2() {
223+
@Override
224+
public InputSource getExternalSubset(final String name, final String baseURI) {
225+
return new InputSource(new StringReader(""));
226+
}
227+
228+
@Override
229+
public InputSource resolveEntity(
230+
final String name,
231+
final String publicId,
232+
final String baseURI,
233+
final String systemId) {
234+
return new InputSource(new StringReader(""));
235+
}
236+
237+
@Override
238+
public InputSource resolveEntity(final String publicId, final String systemId) {
239+
return new InputSource(new StringReader(""));
240+
}
241+
});
192242
return parser;
193243
}
194244
}

‎core/src/main/java/org/mapfish/print/map/style/SLDParserPlugin.java‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
import java.util.Objects;
1212
import java.util.Optional;
1313
import java.util.function.Function;
14+
import javax.xml.XMLConstants;
1415
import javax.xml.parsers.DocumentBuilder;
1516
import javax.xml.parsers.DocumentBuilderFactory;
1617
import javax.xml.parsers.ParserConfigurationException;
@@ -98,7 +99,15 @@ private Optional<Style> tryLoadSLD(
9899
// by setting a custom error handler.
99100
DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance();
100101
dbf.setNamespaceAware(true);
102+
dbf.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
101103
dbf.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
104+
dbf.setFeature("http://xml.org/sax/features/external-general-entities", false);
105+
dbf.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
106+
dbf.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
107+
dbf.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, "");
108+
dbf.setAttribute(XMLConstants.ACCESS_EXTERNAL_SCHEMA, "");
109+
dbf.setXIncludeAware(false);
110+
dbf.setExpandEntityReferences(false);
102111
DocumentBuilder db = dbf.newDocumentBuilder();
103112
db.setErrorHandler(new ErrorHandler());
104113
db.parse(new ByteArrayInputStream(bytes));

‎core/src/main/java/org/mapfish/print/output/AbstractJasperReportOutputFormat.java‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import java.util.concurrent.ForkJoinPool;
2323
import java.util.concurrent.ForkJoinTask;
2424
import java.util.concurrent.atomic.AtomicBoolean;
25+
import javax.xml.XMLConstants;
2526
import javax.xml.parsers.DocumentBuilder;
2627
import javax.xml.parsers.DocumentBuilderFactory;
2728
import javax.xml.parsers.ParserConfigurationException;
@@ -379,6 +380,15 @@ private void assertNoError(
379380
private Document parseXML(final Configuration configuration, final String reportTemplate)
380381
throws ParserConfigurationException, IOException, SAXException {
381382
final DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
383+
factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
384+
factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
385+
factory.setFeature("http://xml.org/sax/features/external-general-entities", false);
386+
factory.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
387+
factory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
388+
factory.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, "");
389+
factory.setAttribute(XMLConstants.ACCESS_EXTERNAL_SCHEMA, "");
390+
factory.setXIncludeAware(false);
391+
factory.setExpandEntityReferences(false);
382392
factory.setValidating(false);
383393
final DocumentBuilder documentBuilder = factory.newDocumentBuilder();
384394
final byte[] bytes = configuration.loadFile(reportTemplate);

‎core/src/test/java/org/mapfish/print/processor/map/CreateMapProcessorFlexibleScaleBBoxGmlTest.java‎

Lines changed: 64 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,16 @@
11
package org.mapfish.print.processor.map;
22

33
import static org.junit.jupiter.api.Assertions.assertEquals;
4+
import static org.junit.jupiter.api.Assertions.assertFalse;
5+
import static org.junit.jupiter.api.Assertions.assertThrows;
6+
import static org.junit.jupiter.api.Assertions.assertTrue;
47

58
import java.io.File;
69
import java.io.IOException;
710
import java.net.URI;
811
import java.util.HashMap;
912
import java.util.List;
13+
import java.util.concurrent.ExecutionException;
1014
import java.util.concurrent.ForkJoinPool;
1115
import java.util.concurrent.ForkJoinTask;
1216
import java.util.concurrent.atomic.AtomicBoolean;
@@ -64,17 +68,7 @@ public void testExecute() throws Exception {
6468
jsonLayer.remove("url");
6569
jsonLayer.accumulate("url", "http://" + host + ":23432" + "/gml/" + gmlDataName);
6670

67-
Values values =
68-
new Values(
69-
new HashMap<>(),
70-
requestData,
71-
template,
72-
getTaskDirectory(),
73-
this.requestFactory,
74-
new File("."),
75-
HTTP_REQUEST_MAX_NUMBER_FETCH_RETRY,
76-
HTTP_REQUEST_FETCH_RETRY_INTERVAL_MILLIS,
77-
new AtomicBoolean(false));
71+
Values values = createValues(requestData, template);
7872

7973
final ForkJoinTask<Values> taskFuture =
8074
this.forkJoinPool.submit(template.getProcessorGraph().createTask(values));
@@ -88,4 +82,63 @@ public void testExecute() throws Exception {
8882
.assertSimilarity(new File(layerGraphics.getFirst()), 0);
8983
}
9084
}
85+
86+
@Test
87+
@DirtiesContext
88+
public void testRejectsXxePayload() throws Exception {
89+
final String host = "center_gml_flexible_scale.com";
90+
requestFactory.registerHandler(
91+
input -> ("" + input.getHost()).contains(host) || input.getAuthority().contains(host),
92+
createFileHandler(uri -> "/map-data" + uri.getPath()));
93+
final Configuration config = configurationFactory.getConfig(getFile(BASE_DIR + "config.yaml"));
94+
final Template template = config.getTemplate("main");
95+
96+
PJsonObject requestData = loadJsonRequestData();
97+
final JSONObject jsonLayer =
98+
requestData
99+
.getJSONObject("attributes")
100+
.getJSONObject("map")
101+
.getJSONArray("layers")
102+
.getJSONObject(0)
103+
.getInternalObj();
104+
jsonLayer.remove("url");
105+
jsonLayer.accumulate("url", "http://" + host + ":23432/gml/malicious-doctype.gml");
106+
107+
Values values = createValues(requestData, template);
108+
final ForkJoinTask<Values> taskFuture =
109+
this.forkJoinPool.submit(template.getProcessorGraph().createTask(values));
110+
111+
ExecutionException exception = assertThrows(ExecutionException.class, taskFuture::get);
112+
String message = collectMessages(exception);
113+
assertTrue(message.contains("Failed to parse GML data"));
114+
assertFalse(message.contains("<!DOCTYPE"));
115+
}
116+
117+
private Values createValues(final PJsonObject requestData, final Template template) {
118+
return new Values(
119+
new HashMap<>(),
120+
requestData,
121+
template,
122+
getTaskDirectory(),
123+
this.requestFactory,
124+
new File("."),
125+
HTTP_REQUEST_MAX_NUMBER_FETCH_RETRY,
126+
HTTP_REQUEST_FETCH_RETRY_INTERVAL_MILLIS,
127+
new AtomicBoolean(false));
128+
}
129+
130+
private String collectMessages(final Throwable throwable) {
131+
StringBuilder sb = new StringBuilder();
132+
Throwable current = throwable;
133+
while (current != null) {
134+
if (current.getMessage() != null) {
135+
sb.append(current.getMessage());
136+
}
137+
current = current.getCause();
138+
if (current != null) {
139+
sb.append('\n');
140+
}
141+
}
142+
return sb.toString();
143+
}
91144
}

‎core/src/test/resources/map-data/gml/malicious-doctype.gml‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
<?xml version="1.0" encoding="UTF-8"?>
2+
<!DOCTYPE wfs:FeatureCollection [
3+
<!ENTITY xxe SYSTEM "file:///etc/passwd">
4+
]>
5+
<wfs:FeatureCollection
6+
xmlns:wfs="http://www.opengis.net/wfs"
7+
xmlns:gml="http://www.opengis.net/gml"
8+
xmlns:topp="http://www.openplans.org/topp">
9+
<gml:featureMember>
10+
<topp:streams fid="streams.1">
11+
<topp:the_geom>
12+
<gml:LineString srsName="EPSG:4326">
13+
<gml:coordinates>0,0 1,1</gml:coordinates>
14+
</gml:LineString>
15+
</topp:the_geom>
16+
<topp:name>&xxe;</topp:name>
17+
</topp:streams>
18+
</gml:featureMember>
19+
</wfs:FeatureCollection>

0 commit comments

Comments
 (0)