From 2bc58a0d1aac2feb68f30051f2a0ccace94b94d0 Mon Sep 17 00:00:00 2001 From: Patrick Gotthard Date: Sat, 27 Jun 2015 08:53:00 +0200 Subject: [PATCH] Fixed #203 - XML bomb vulnerability The current configuration of the SAXParser is vulnerable for an XML bomb attack. The feature "http://apache.org/xml/features/disallow-doctype-decl" of the SAXBuilder must be activated in order to fix the vulnerability. --- README.md | 31 +++++---- .../com/rometools/rome/io/SyndFeedInput.java | 21 ++++++ .../com/rometools/rome/io/WireFeedInput.java | 68 ++++++++++++------- .../rometools/rome/unittest/FeedOpsTest.java | 4 ++ .../com/rometools/rome/unittest/FeedTest.java | 9 +++ .../rometools/rome/unittest/SyndFeedTest.java | 6 +- .../rome/unittest/TestOpsRSS091N.java | 2 +- .../rome/unittest/TestSyndFeedRSS091N.java | 2 +- .../rometools/rome/unittest/TestXmlBomb.java | 54 +++++++++++++++ 9 files changed, 157 insertions(+), 40 deletions(-) create mode 100644 src/test/java/com/rometools/rome/unittest/TestXmlBomb.java diff --git a/README.md b/README.md index 1d3cf76..b1c6816 100644 --- a/README.md +++ b/README.md @@ -5,16 +5,23 @@ RSS 0.91 Userland, RSS 0.92, RSS 0.93, RSS 0.94, RSS 1.0, RSS 2.0, Atom 0.3, Ato More Information: http://rometools.github.io/rome/ -## Buildstatus +## Changelog -| Project | Status | -| --------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| rome | [![Build Status](http://jenkins.patrick-gotthard.de/buildStatus/icon?job=rome)](http://jenkins.patrick-gotthard.de/job/rome/) | -| rome-certiorem | [![Build Status](http://jenkins.patrick-gotthard.de/buildStatus/icon?job=rome-certiorem)](http://jenkins.patrick-gotthard.de/job/rome-certiorem/) | -| rome-certiorem-webapp | [![Build Status](http://jenkins.patrick-gotthard.de/buildStatus/icon?job=rome-certiorem-webapp)](http://jenkins.patrick-gotthard.de/job/rome-certiorem-webapp/) | -| rome-fetcher | [![Build Status](http://jenkins.patrick-gotthard.de/buildStatus/icon?job=rome-fetcher)](http://jenkins.patrick-gotthard.de/job/rome-fetcher/) | -| rome-modules | [![Build Status](http://jenkins.patrick-gotthard.de/buildStatus/icon?job=rome-modules)](http://jenkins.patrick-gotthard.de/job/rome-modules/) | -| rome-opml | [![Build Status](http://jenkins.patrick-gotthard.de/buildStatus/icon?job=rome-opml)](http://jenkins.patrick-gotthard.de/job/rome-opml/) | -| rome-parent | [![Build Status](http://jenkins.patrick-gotthard.de/buildStatus/icon?job=rome-parent)](http://jenkins.patrick-gotthard.de/job/rome-parent/) | -| rome-propono | [![Build Status](http://jenkins.patrick-gotthard.de/buildStatus/icon?job=rome-propono)](http://jenkins.patrick-gotthard.de/job/rome-propono/) | -| rome-utils | [![Build Status](http://jenkins.patrick-gotthard.de/buildStatus/icon?job=rome-utils)](http://jenkins.patrick-gotthard.de/job/rome-utils/) | \ No newline at end of file +### 1.5.1 + +- solved an [XML bomb](https://en.wikipedia.org/wiki/Billion_laughs) vulnerability + +Important note: due to the security fix ROME now forbids all Doctype declarations by default. This will break compatibility with RSS 0.91 Netscape +because it requires a Doctype declaration. When you experience problems you have to activate the property **allowDoctypes** on the SyndFeedInput object. You +should only use this possibility when the feeds that you process are absolutely trustful. + +### 1.5.0 + +- many (untracked) enhancements +- code cleanup +- renamed packages (was required to be able to push to Maven Central after years again) +- updated sourcecode to Java 1.6 + +### Prior to 1.5.0 + +- see [http://rometools.github.io/rome/ROMEReleases](http://rometools.github.io/rome/ROMEReleases) \ No newline at end of file diff --git a/src/main/java/com/rometools/rome/io/SyndFeedInput.java b/src/main/java/com/rometools/rome/io/SyndFeedInput.java index dc68ce4..4acac3f 100644 --- a/src/main/java/com/rometools/rome/io/SyndFeedInput.java +++ b/src/main/java/com/rometools/rome/io/SyndFeedInput.java @@ -100,6 +100,27 @@ public class SyndFeedInput { public boolean getXmlHealerOn() { return feedInput.getXmlHealerOn(); } + + /** + * Indicates whether Doctype declarations are allowed. + * + * @return true when Doctype declarations are allowed, false otherwise + */ + public boolean isAllowDoctypes() { + return feedInput.isAllowDoctypes(); + } + + /** + * Since ROME 1.5.1 we fixed a security vulnerability by disallowing Doctype declarations by default. + * This change breaks the compatibility with at least RSS 0.91N because it requires a Doctype declaration. + * You are able to allow Doctype declarations again with this property. You should only activate it + * when the feeds that you process are absolutely trustful. + * + * @param allowDoctypes true when Doctype declarations should be allowed again, false otherwise + */ + public void setAllowDoctypes(boolean allowDoctypes) { + feedInput.setAllowDoctypes(allowDoctypes); + } /** * Builds SyndFeedImpl from a file. diff --git a/src/main/java/com/rometools/rome/io/WireFeedInput.java b/src/main/java/com/rometools/rome/io/WireFeedInput.java index 6825309..3f88be1 100644 --- a/src/main/java/com/rometools/rome/io/WireFeedInput.java +++ b/src/main/java/com/rometools/rome/io/WireFeedInput.java @@ -67,6 +67,7 @@ public class WireFeedInput { private final Locale locale; private boolean xmlHealerOn; + private boolean allowDoctypes = false; private static FeedParsers getFeedParsers() { synchronized (WireFeedInput.class) { @@ -163,6 +164,27 @@ public class WireFeedInput { public boolean getXmlHealerOn() { return xmlHealerOn; } + + /** + * Indicates whether Doctype declarations are allowed. + * + * @return true when Doctype declarations are allowed, false otherwise + */ + public boolean isAllowDoctypes() { + return allowDoctypes; + } + + /** + * Since ROME 1.5.1 we fixed a security vulnerability by disallowing Doctype declarations by default. + * This change breaks the compatibility with at least RSS 0.91N because it requires a Doctype declaration. + * You are able to allow Doctype declarations again with this property. You should only activate it + * when the feeds that you process are absolutely trustful. + * + * @param allowDoctypes true when Doctype declarations should be allowed again, false otherwise + */ + public void setAllowDoctypes(boolean allowDoctypes) { + this.allowDoctypes = allowDoctypes; + } /** * Builds an WireFeed (RSS or Atom) from a file. @@ -322,40 +344,36 @@ public class WireFeedInput { // // Crimson is one parser which is known not to support these features. try { + final XMLReader parser = saxBuilder.createParser(); - try { - parser.setFeature("http://xml.org/sax/features/external-general-entities", false); - saxBuilder.setFeature("http://xml.org/sax/features/external-general-entities", false); - } catch (final SAXNotRecognizedException e) { - // ignore - } catch (final SAXNotSupportedException e) { - // ignore - } + + setFeature(saxBuilder, parser, "http://xml.org/sax/features/external-general-entities", false); + setFeature(saxBuilder, parser, "http://xml.org/sax/features/external-parameter-entities", false); + setFeature(saxBuilder, parser, "http://apache.org/xml/features/nonvalidating/load-external-dtd", false); - try { - parser.setFeature("http://xml.org/sax/features/external-parameter-entities", false); - saxBuilder.setFeature("http://xml.org/sax/features/external-parameter-entities", false); - } catch (final SAXNotRecognizedException e) { - // ignore - } catch (final SAXNotSupportedException e) { - // ignore - } - - try { - parser.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false); - saxBuilder.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false); - } catch (final SAXNotRecognizedException e) { - // ignore - } catch (final SAXNotSupportedException e) { - // ignore + if(!allowDoctypes) { + setFeature(saxBuilder, parser, "http://apache.org/xml/features/disallow-doctype-decl", true); } } catch (final JDOMException e) { throw new IllegalStateException("JDOM could not create a SAX parser"); } - + saxBuilder.setExpandEntities(false); + return saxBuilder; + + } + + private void setFeature(SAXBuilder saxBuilder, XMLReader parser, String feature, boolean value) { + try { + saxBuilder.setFeature(feature, true); + parser.setFeature(feature, true); + } catch (final SAXNotRecognizedException e) { + // ignore + } catch (final SAXNotSupportedException e) { + // ignore + } } } diff --git a/src/test/java/com/rometools/rome/unittest/FeedOpsTest.java b/src/test/java/com/rometools/rome/unittest/FeedOpsTest.java index be0b2e5..9354686 100644 --- a/src/test/java/com/rometools/rome/unittest/FeedOpsTest.java +++ b/src/test/java/com/rometools/rome/unittest/FeedOpsTest.java @@ -21,6 +21,10 @@ public abstract class FeedOpsTest extends FeedTest { protected FeedOpsTest(final String feedType) { super(feedType + ".xml"); } + + protected FeedOpsTest(final String feedType, boolean allowDoctypes) { + super(feedType + ".xml", allowDoctypes); + } // 1.2a public void testWireFeedEquals() throws Exception { diff --git a/src/test/java/com/rometools/rome/unittest/FeedTest.java b/src/test/java/com/rometools/rome/unittest/FeedTest.java index 5cbd914..d7bbb9b 100644 --- a/src/test/java/com/rometools/rome/unittest/FeedTest.java +++ b/src/test/java/com/rometools/rome/unittest/FeedTest.java @@ -21,7 +21,9 @@ import com.rometools.rome.io.WireFeedInput; * */ public abstract class FeedTest extends TestCase { + private final String feedFileName; + private final boolean allowDoctypes; private Document jDomDoc = null; private WireFeed wireFeed = null; private SyndFeed syndFeed = null; @@ -29,7 +31,12 @@ public abstract class FeedTest extends TestCase { private boolean preservingWireFeed = false; protected FeedTest(final String feedFileName) { + this(feedFileName, false); + } + + protected FeedTest(final String feedFileName, boolean allowDoctypes) { this.feedFileName = feedFileName; + this.allowDoctypes = allowDoctypes; } protected String getFeedFileName() { @@ -50,12 +57,14 @@ public abstract class FeedTest extends TestCase { protected WireFeed getWireFeed() throws Exception { final WireFeedInput in = new WireFeedInput(); + in.setAllowDoctypes(allowDoctypes); return in.build(getFeedReader()); } protected SyndFeed getSyndFeed(final boolean preserveWireFeed) throws Exception { final SyndFeedInput in = new SyndFeedInput(); in.setPreserveWireFeed(preserveWireFeed); + in.setAllowDoctypes(allowDoctypes); return in.build(getFeedReader()); } diff --git a/src/test/java/com/rometools/rome/unittest/SyndFeedTest.java b/src/test/java/com/rometools/rome/unittest/SyndFeedTest.java index 2453268..be867ef 100644 --- a/src/test/java/com/rometools/rome/unittest/SyndFeedTest.java +++ b/src/test/java/com/rometools/rome/unittest/SyndFeedTest.java @@ -19,7 +19,11 @@ public abstract class SyndFeedTest extends FeedTest { } protected SyndFeedTest(final String feedType, final String feedFileName) { - super(feedFileName); + this(feedType, feedFileName, false); + } + + protected SyndFeedTest(final String feedType, final String feedFileName, boolean allowDoctypes) { + super(feedFileName, allowDoctypes); prefix = feedType; } diff --git a/src/test/java/com/rometools/rome/unittest/TestOpsRSS091N.java b/src/test/java/com/rometools/rome/unittest/TestOpsRSS091N.java index 58cb731..024edd5 100644 --- a/src/test/java/com/rometools/rome/unittest/TestOpsRSS091N.java +++ b/src/test/java/com/rometools/rome/unittest/TestOpsRSS091N.java @@ -15,7 +15,7 @@ public class TestOpsRSS091N extends FeedOpsTest { } public TestOpsRSS091N() { - super("rss_0.91N"); + super("rss_0.91N", true); } } diff --git a/src/test/java/com/rometools/rome/unittest/TestSyndFeedRSS091N.java b/src/test/java/com/rometools/rome/unittest/TestSyndFeedRSS091N.java index 2ba777a..d3b2a36 100644 --- a/src/test/java/com/rometools/rome/unittest/TestSyndFeedRSS091N.java +++ b/src/test/java/com/rometools/rome/unittest/TestSyndFeedRSS091N.java @@ -18,7 +18,7 @@ import com.rometools.rome.io.impl.DateParser; public class TestSyndFeedRSS091N extends SyndFeedTest { public TestSyndFeedRSS091N() { - super("rss_0.91N", "rss_0.91N.xml"); + super("rss_0.91N", "rss_0.91N.xml", true); } protected TestSyndFeedRSS091N(final String type) { diff --git a/src/test/java/com/rometools/rome/unittest/TestXmlBomb.java b/src/test/java/com/rometools/rome/unittest/TestXmlBomb.java new file mode 100644 index 0000000..ce15c1e --- /dev/null +++ b/src/test/java/com/rometools/rome/unittest/TestXmlBomb.java @@ -0,0 +1,54 @@ +package com.rometools.rome.unittest; + + +import java.io.ByteArrayInputStream; +import java.io.InputStream; +import java.io.InputStreamReader; +import java.io.Reader; +import java.nio.charset.Charset; + +import com.rometools.rome.io.ParsingFeedException; +import com.rometools.rome.io.WireFeedInput; +import junit.framework.TestCase; + +public class TestXmlBomb extends TestCase { + + public void testXmlBomb() throws Exception { + + // https://en.wikipedia.org/wiki/Billion_laughs + // https://msdn.microsoft.com/en-us/magazine/ee335713.aspx + String content = "\n" + + "\n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + "]>\n" + + "\n" + + " &lol9;\n" + + " subtitle\n" + + " \n" + + " id1\n" + + " title1\n" + + " \n" + + ""; + + InputStream is = new ByteArrayInputStream(content.getBytes("UTF-8")); + WireFeedInput feedInput = new WireFeedInput(); + Reader reader = new InputStreamReader(is, Charset.forName("UTF-8")); + try { + feedInput.build(reader); + fail("Expected exception"); + } + catch (ParsingFeedException ex) { + } + } + +} \ No newline at end of file