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

@@ -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                                                                                                                                                            |
+### 1.5.1
-| --------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------- |
+
-| rome                  | [![Build Status](http://jenkins.patrick-gotthard.de/buildStatus/icon?job=rome)](http://jenkins.patrick-gotthard.de/job/rome/)                                     |
+- solved an [XML bomb](https://en.wikipedia.org/wiki/Billion_laughs) vulnerability
-| 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/)   |
+Important note: due to the security fix ROME now forbids all Doctype declarations by default. This will break compatibility with RSS 0.91 Netscape
-| rome-fetcher          | [![Build Status](http://jenkins.patrick-gotthard.de/buildStatus/icon?job=rome-fetcher)](http://jenkins.patrick-gotthard.de/job/rome-fetcher/)                     |
+because it requires a Doctype declaration. When you experience problems you have to activate the property **allowDoctypes** on the SyndFeedInput object. You 
-| rome-modules          | [![Build Status](http://jenkins.patrick-gotthard.de/buildStatus/icon?job=rome-modules)](http://jenkins.patrick-gotthard.de/job/rome-modules/)                     |
+should only use this possibility when the feeds that you process are absolutely trustful.
-| 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/)                       |
+### 1.5.0
-| 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/)                         |
+- 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) 

src/main/java/com/rometools/rome/io/SyndFeedInput.java

@@ -101,6 +101,27 @@ public class SyndFeedInput {
         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.
      * <p>

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) {
@@ -164,6 +165,27 @@ public class WireFeedInput {
         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.
      * <p>
@@ -322,32 +344,15 @@ 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
-            }
             
-            try {
+            setFeature(saxBuilder, parser, "http://xml.org/sax/features/external-general-entities", false);
-                parser.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
+            setFeature(saxBuilder, parser, "http://xml.org/sax/features/external-parameter-entities", false);
-                saxBuilder.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
+            setFeature(saxBuilder, parser, "http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
-            } catch (final SAXNotRecognizedException e) {
-                // ignore
-            } catch (final SAXNotSupportedException e) {
-                // ignore
-            }
 
-            try {
+            if(!allowDoctypes) {
-                parser.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
+                setFeature(saxBuilder, parser, "http://apache.org/xml/features/disallow-doctype-decl", true);
-                saxBuilder.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
-            } catch (final SAXNotRecognizedException e) {
-                // ignore
-            } catch (final SAXNotSupportedException e) {
-                // ignore
             }
 
         } catch (final JDOMException e) {
@@ -355,7 +360,20 @@ public class WireFeedInput {
         }
         
         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
+        }
     }
 
 }

src/test/java/com/rometools/rome/unittest/FeedOpsTest.java

@@ -22,6 +22,10 @@ public abstract class FeedOpsTest extends FeedTest {
         super(feedType + ".xml");
     }
     
+    protected FeedOpsTest(final String feedType, boolean allowDoctypes) {
+        super(feedType + ".xml", allowDoctypes);
+    }
+
     // 1.2a
     public void testWireFeedEquals() throws Exception {
         final WireFeed feed1 = getCachedWireFeed();

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());
     }
 

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;
     }
 

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);
     }
 
 }

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

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 = "<?xml version=\"1.0\"?>\n" +
+				"<!DOCTYPE lolz [\n" +
+				" <!ENTITY lol \"lol\">\n" +
+				" <!ELEMENT lolz (#PCDATA)>\n" +
+				" <!ENTITY lol1 \"&lol;&lol;&lol;&lol;&lol;&lol;&lol;&lol;&lol;&lol;\">\n" +
+				" <!ENTITY lol2 \"&lol1;&lol1;&lol1;&lol1;&lol1;&lol1;&lol1;&lol1;&lol1;&lol1;\">\n" +
+				" <!ENTITY lol3 \"&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;\">\n" +
+				" <!ENTITY lol4 \"&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;\">\n" +
+				" <!ENTITY lol5 \"&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;\">\n" +
+				" <!ENTITY lol6 \"&lol5;&lol5;&lol5;&lol5;&lol5;&lol5;&lol5;&lol5;&lol5;&lol5;\">\n" +
+				" <!ENTITY lol7 \"&lol6;&lol6;&lol6;&lol6;&lol6;&lol6;&lol6;&lol6;&lol6;&lol6;\">\n" +
+				" <!ENTITY lol8 \"&lol7;&lol7;&lol7;&lol7;&lol7;&lol7;&lol7;&lol7;&lol7;&lol7;\">\n" +
+				" <!ENTITY lol9 \"&lol8;&lol8;&lol8;&lol8;&lol8;&lol8;&lol8;&lol8;&lol8;&lol8;\">\n" +
+				"]>\n" +
+				"<feed>\n" +
+				"    <title>&lol9;</title>\n" +
+				"    <subtitle>subtitle</subtitle>\n" +
+				"    <entry>\n" +
+				"        <id>id1</id>\n" +
+				"        <title>title1</title>\n" +
+				"    </entry>\n" +
+				"</feed>";
+
+		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) {
+		}
+	}
+
+}