XXE in MsBibImporter

[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 Occurred

The following code snippet displays the usage of DocumentBuilderFactory without disabling entities:

jabref/src/main/java/org/jabref/logic/importer/fileformat/MsBibImporter.java

Line 40 in 2f5f2e5

DocumentBuilder dbuild = DocumentBuilderFactory.newInstance().newDocumentBuilder();

The following code snippet displays the parsing of the XML:

jabref/src/main/java/org/jabref/logic/importer/fileformat/MsBibImporter.java

Line 58 in 2f5f2e5

docin = dbuild.parse(new InputSource(reader));

[Siedlerchr]

Method for fixing this:
https://www.owasp.org/index.php/XML_External_Entity_(XXE)_Prevention_Cheat_Sheet#JAXP_DocumentBuilderFactory.2C_SAXParserFactory_and_DOM4J

[nicksw]

Hi, Do you all mind if I try to give this one a shot? I take it the class to look at is the one linked by ProDigySML?

[prodigysml]

Hey @nicksw! Yes it should be a simple fix. Adding in the above-mentioned prevention measures within the MsBibImporter class should fix the issue.

[Siedlerchr]

@nicksw Feel free to start over! Check out the contribution guideline and go ahead: https://github.com/JabRef/jabref/blob/master/CONTRIBUTING.md

[nicksw]

Thanks for the free range @Siedlerchr though I just opted to create a private method with the fixes and also correct a spelling error. My one issue is that I am failing 5 tests when I run gradlew check The weird thing is that I fail the same 5 tests with the original MsBibImporter file, see attached . Did I incorrectly setup my workspace?
reports.zip

[Siedlerchr]

@nicksw You did all right. And your changes did not break the implementation, as the failing tests are not related to your importer. As gradlew check runs all tests and consumes much time we rareley use it locally as they are all run automatically on travis CI server. When you create a PR (Pull Request), your changes are automatically tested on the CI (Continuous Integration) and you will see when they fail. Sometimes tests fail due to some weird circumstances. I had this problem once with encoding differences from the command line which resulted in all importer tests failing, because the default encoding was not uf8 on the command line.

Just execute the corresponding tests, e.g. in your case the importer tests in/from the IDE to see if it still works and create a PR.
In addition to these automatic tests we conduct code reviews, e.g one or two devs look over the changes before merging them.

[nicksw]

@Siedlerchr Great! Thanks for the all the info. Ran JUnit testing for the above mentioned and created a pull request. Thanks for all the advise and help on my first contribution.

[tobiasdiez]

This is now fixed thanks to @nicksw!