[PATCH][RFC] Abort on XML parse errors

[SimonPoole]

I haven't checked the source yet, but it seems that an unknown element in OSM XML input from the API will not cause the parser to abort, see

​https://lists.openstreetmap.org/pipermail/talk/2022-July/087672.html

and

​https://github.com/zerebubuth/openstreetmap-cgimap/issues/276

While there is a place for being lenient with what you accept, the XML parser probably isn't it.

[SimonPoole]

See source:trunk/src/org/openstreetmap/josm/io/OsmReader.java@18512:391-406#L391​ IMHO either what we are parsing is known, can be enumerated and if we don't want to process it can be ignored, or it is really unknown and should generate a parse exception.

[anonymous]

As far as user visibly is concerned - then #11652 is related.

[taylor.smock]

Please note that we do create the xml reader using the factory from source:trunk/src/org/openstreetmap/josm/tools/XmlUtils.java@18512:131-138#L131​.

Looking at the OWASP ​XML XXE and ​XML Security cheatsheets, we do the following:

• Disable external entities
• Disable validation
• Disable DTD

From the XML XXE, we aren't doing the following:

• xmlInputFactory.setProperty(XMLConstants.ACCESS_EXTERNAL_DTD, ""); -- this should be covered by the SUPPORT_DTD and IS_SUPPORTING_EXTERNAL_ENTITIES flags.

From XML Security:

• Invalid XML Documents:
  • There is no official OSM XML schema (see ​osmwiki:OSM_XML/XSD ), so we should not throw an error on unknown elements. Especially since the XML schema can be extended by third parties (e.g. overpass), and plugins may reuse the code for other purposes (i.e., some server may return an extended osm xml file), so we probably cannot throw an exception on an unknown element. I'll have to go through the code and see how plugins have extended it.

See #6742 and r4645 for when the parseUnknown code was introduced.

With that said, we should probably parse the <error> element, and throw an exception with the error body as the message.

[taylor.smock]

Parse error from OSM API

[taylor.smock]

Add unit tests for <error> tags

[taylor.smock]

In 18541/josm:

Fix #22250: Abort on XML error elements