From 57d3a8a5ce5ee92d09bb8c673d67fcb0b8e577bb Mon Sep 17 00:00:00 2001 From: Nicksw Date: Sun, 29 Jul 2018 11:05:54 -0400 Subject: [PATCH 1/4] Fix importer vulnerability Fixed issue #4229 where importer was vulnerable to XXE attacks by disabling DTDs along with adding warning to logger if features are unavailable. fixes #4229 --- CHANGELOG.md | 2 +- .../importer/fileformat/MsBibImporter.java | 38 +++++++++++++++++-- 2 files changed, 36 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1a8805404d5..b0a1710545f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -58,7 +58,7 @@ We refer to [GitHub issues](/~https://github.com/JabRef/jabref/issues) by using `# - We fixed an issue where the "Convert to BibTeX-Cleanup" moved the content of the `file` field to the `pdf` field [#4120](/~https://github.com/JabRef/jabref/issues/4120) - We fixed an issue where the preview pane in entry preview in preferences wasn't showing the citation style selected [#3849](/~https://github.com/JabRef/jabref/issues/3849) - We fixed an issue where the default entry preview style still contained the field `review`. The field `review` in the style is now replaced with comment to be consistent with the entry editor [#4098](/~https://github.com/JabRef/jabref/issues/4098) - +- We an issue where users were vulnerable to XXE attacks during parsing [#4229](/~https://github.com/JabRef/jabref/issues/4229) diff --git a/src/main/java/org/jabref/logic/importer/fileformat/MsBibImporter.java b/src/main/java/org/jabref/logic/importer/fileformat/MsBibImporter.java index 500cec32b63..907564705d8 100644 --- a/src/main/java/org/jabref/logic/importer/fileformat/MsBibImporter.java +++ b/src/main/java/org/jabref/logic/importer/fileformat/MsBibImporter.java @@ -6,6 +6,7 @@ import javax.xml.parsers.DocumentBuilder; import javax.xml.parsers.DocumentBuilderFactory; +import javax.xml.parsers.ParserConfigurationException; import org.jabref.logic.importer.Importer; import org.jabref.logic.importer.ParserResult; @@ -18,26 +19,31 @@ import org.xml.sax.SAXException; import org.xml.sax.SAXParseException; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + /** * Importer for the MS Office 2007 XML bibliography format - * By S. M. Mahbub Murshed + * By S. M. Mahbub Murshed & Nicholas S. Weatherley * * ... */ public class MsBibImporter extends Importer { + private static final Logger LOGGER = LoggerFactory.getLogger(MsBibImporter.class); + @Override public boolean isRecognizedFormat(BufferedReader reader) throws IOException { Objects.requireNonNull(reader); /* - The correct behaviour is to return false if it is certain that the file is + The correct behavior is to return false if it is certain that the file is not of the MsBib type, and true otherwise. Returning true is the safe choice if not certain. */ Document docin; try { - DocumentBuilder dbuild = DocumentBuilderFactory.newInstance().newDocumentBuilder(); + DocumentBuilder dbuild = makeSafeDocBuilderFactory(DocumentBuilderFactory.newInstance()).newDocumentBuilder(); dbuild.setErrorHandler(new ErrorHandler() { @Override @@ -55,6 +61,7 @@ public void error(SAXParseException exception) throws SAXException { throw exception; } }); + docin = dbuild.parse(new InputSource(reader)); } catch (Exception e) { return false; @@ -85,4 +92,29 @@ public String getDescription() { return "Importer for the MS Office 2007 XML bibliography format."; } + /** + * DocumentBuilderFactory makes a XXE safe Builder factory from dBuild. If not supported by current + * XML then returns original builder given and logs error. + * @param dBuild | DocumentBuilderFactory to be made XXE safe. + * @return If supported, XXE safe DocumentBuilderFactory. Else, returns original builder given + */ + private DocumentBuilderFactory makeSafeDocBuilderFactory(DocumentBuilderFactory dBuild) { + String FEATURE = null; + try { + FEATURE = "http://apache.org/xml/features/disallow-doctype-decl"; + dBuild.setFeature(FEATURE, true); + + FEATURE = "http://apache.org/xml/features/nonvalidating/load-external-dtd"; + dBuild.setFeature(FEATURE, false); + + dBuild.setXIncludeAware(false); + dBuild.setExpandEntityReferences(false); + + } catch (ParserConfigurationException e) { + LOGGER.warn("Builder not fully configured. ParserConfigurationException was thrown. Feature:'" + + FEATURE + "' is probably not supported by current XML processor."); + } + + return dBuild; + } } From 87a768243448b89d13437f7e1b3c54c2d655ee70 Mon Sep 17 00:00:00 2001 From: Nicksw Date: Sun, 29 Jul 2018 20:06:47 -0400 Subject: [PATCH 2/4] Fix minor code errors and logger optimization Removed author line in class comment. Reworded CHANGLOG.md. Set DTD features to individual final static constants. Optimized logger by parameterizing feature and error. --- CHANGELOG.md | 2 +- .../importer/fileformat/MsBibImporter.java | 18 +++++++++--------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b0a1710545f..74547cd1eb9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -58,7 +58,7 @@ We refer to [GitHub issues](/~https://github.com/JabRef/jabref/issues) by using `# - We fixed an issue where the "Convert to BibTeX-Cleanup" moved the content of the `file` field to the `pdf` field [#4120](/~https://github.com/JabRef/jabref/issues/4120) - We fixed an issue where the preview pane in entry preview in preferences wasn't showing the citation style selected [#3849](/~https://github.com/JabRef/jabref/issues/3849) - We fixed an issue where the default entry preview style still contained the field `review`. The field `review` in the style is now replaced with comment to be consistent with the entry editor [#4098](/~https://github.com/JabRef/jabref/issues/4098) -- We an issue where users were vulnerable to XXE attacks during parsing [#4229](/~https://github.com/JabRef/jabref/issues/4229) +- We fixed an issue where users were vulnerable to XXE attacks during parsing [#4229](/~https://github.com/JabRef/jabref/issues/4229) diff --git a/src/main/java/org/jabref/logic/importer/fileformat/MsBibImporter.java b/src/main/java/org/jabref/logic/importer/fileformat/MsBibImporter.java index 907564705d8..40f2a6fc7b1 100644 --- a/src/main/java/org/jabref/logic/importer/fileformat/MsBibImporter.java +++ b/src/main/java/org/jabref/logic/importer/fileformat/MsBibImporter.java @@ -21,16 +21,16 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; - /** * Importer for the MS Office 2007 XML bibliography format - * By S. M. Mahbub Murshed & Nicholas S. Weatherley * * ... */ public class MsBibImporter extends Importer { private static final Logger LOGGER = LoggerFactory.getLogger(MsBibImporter.class); + private static final String DISABLEDTD = "http://apache.org/xml/features/disallow-doctype-decl"; + private static final String DISABLEEXTERNALDTD = "http://apache.org/xml/features/nonvalidating/load-external-dtd"; @Override public boolean isRecognizedFormat(BufferedReader reader) throws IOException { @@ -99,20 +99,20 @@ public String getDescription() { * @return If supported, XXE safe DocumentBuilderFactory. Else, returns original builder given */ private DocumentBuilderFactory makeSafeDocBuilderFactory(DocumentBuilderFactory dBuild) { - String FEATURE = null; + String feature = null; + try { - FEATURE = "http://apache.org/xml/features/disallow-doctype-decl"; - dBuild.setFeature(FEATURE, true); + feature = DISABLEDTD; + dBuild.setFeature(feature, true); - FEATURE = "http://apache.org/xml/features/nonvalidating/load-external-dtd"; - dBuild.setFeature(FEATURE, false); + feature = DISABLEEXTERNALDTD; + dBuild.setFeature(feature, false); dBuild.setXIncludeAware(false); dBuild.setExpandEntityReferences(false); } catch (ParserConfigurationException e) { - LOGGER.warn("Builder not fully configured. ParserConfigurationException was thrown. Feature:'" + - FEATURE + "' is probably not supported by current XML processor."); + LOGGER.warn("Builder not fully configured. Feature:'{}' is probably not supported by current XML processor. {}", feature, e); } return dBuild; From 11da233c322d2359f087b9254ef9f4c1aa175328 Mon Sep 17 00:00:00 2001 From: Nicksw Date: Mon, 30 Jul 2018 09:53:59 -0400 Subject: [PATCH 3/4] Rearrange import statments for project compatibility --- CHANGELOG.md | 2 +- .../org/jabref/logic/importer/fileformat/MsBibImporter.java | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 33496a2ca11..073333caa15 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -63,7 +63,7 @@ We refer to [GitHub issues](/~https://github.com/JabRef/jabref/issues) by using `# <<<<<<< HEAD - We fixed an issue where users were vulnerable to XXE attacks during parsing [#4229](/~https://github.com/JabRef/jabref/issues/4229) ======= -- We an issue where users were vulnerable to XXE attacks during parsing [#4229](/~https://github.com/JabRef/jabref/issues/4229) +- We fixed an issue where users were vulnerable to XXE attacks during parsing [#4229](/~https://github.com/JabRef/jabref/issues/4229) - We fixed an issue where files added via the "Attach file" contextmenu of an entry were not made relative. [#4201](/~https://github.com/JabRef/jabref/issues/4201) >>>>>>> 72264836363b0cd55df49bd171ea15b4c6e774c9 diff --git a/src/main/java/org/jabref/logic/importer/fileformat/MsBibImporter.java b/src/main/java/org/jabref/logic/importer/fileformat/MsBibImporter.java index 40f2a6fc7b1..db68e7958b7 100644 --- a/src/main/java/org/jabref/logic/importer/fileformat/MsBibImporter.java +++ b/src/main/java/org/jabref/logic/importer/fileformat/MsBibImporter.java @@ -13,14 +13,14 @@ import org.jabref.logic.msbib.MSBibDatabase; import org.jabref.logic.util.StandardFileType; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.w3c.dom.Document; import org.xml.sax.ErrorHandler; import org.xml.sax.InputSource; import org.xml.sax.SAXException; import org.xml.sax.SAXParseException; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; /** * Importer for the MS Office 2007 XML bibliography format * From cd3dd60431f8f03d0ae8bba96067a787aeedb22d Mon Sep 17 00:00:00 2001 From: Tobias Diez Date: Mon, 30 Jul 2018 18:05:21 +0200 Subject: [PATCH 4/4] Remove merge artefacts from changelog --- CHANGELOG.md | 4 ---- 1 file changed, 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 073333caa15..eac14594f4e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -60,12 +60,8 @@ We refer to [GitHub issues](/~https://github.com/JabRef/jabref/issues) by using `# - We fixed an issue where the "Convert to BibTeX-Cleanup" moved the content of the `file` field to the `pdf` field [#4120](/~https://github.com/JabRef/jabref/issues/4120) - We fixed an issue where the preview pane in entry preview in preferences wasn't showing the citation style selected [#3849](/~https://github.com/JabRef/jabref/issues/3849) - We fixed an issue where the default entry preview style still contained the field `review`. The field `review` in the style is now replaced with comment to be consistent with the entry editor [#4098](/~https://github.com/JabRef/jabref/issues/4098) -<<<<<<< HEAD -- We fixed an issue where users were vulnerable to XXE attacks during parsing [#4229](/~https://github.com/JabRef/jabref/issues/4229) -======= - We fixed an issue where users were vulnerable to XXE attacks during parsing [#4229](/~https://github.com/JabRef/jabref/issues/4229) - We fixed an issue where files added via the "Attach file" contextmenu of an entry were not made relative. [#4201](/~https://github.com/JabRef/jabref/issues/4201) ->>>>>>> 72264836363b0cd55df49bd171ea15b4c6e774c9