Prevent XXE attacks by disabling external entities resolution in the …

…default parser

Signed-off-by: Ryan Senior <ryan.senior@puppetlabs.com>

src/main/clojure/clojure/data/xml.clj

@@ -54,7 +54,7 @@
       (= s "")))
 
 (defn emit-cdata [^String cdata-str ^javax.xml.stream.XMLStreamWriter writer]
-  (when-not (str-empty? cdata-str) 
+  (when-not (str-empty? cdata-str)
     (let [idx (.indexOf cdata-str "]]>")]
       (if (= idx -1)
         (.writeCData writer cdata-str )
@@ -97,7 +97,7 @@
     (if-let [r (seq (rest coll))]
       (cons (next-events (first coll) r) next-items)
       (next-events (first coll) next-items)))
-  
+
   String
   (gen-event [s]
     (Event. :chars nil nil s))
@@ -121,13 +121,13 @@
     (Event. :cdata nil nil (:content cdata)))
   (next-events [_ next-items]
     next-items)
-  
+
   Comment
   (gen-event [comment]
     (Event. :comment nil nil (:content comment)))
   (next-events [_ next-items]
     next-items)
-  
+
   nil
   (gen-event [_]
     (Event. :chars nil nil ""))
@@ -279,7 +279,7 @@
 ; Note, sreader is mutable and mutated here in pull-seq, but it's
 ; protected by a lazy-seq so it's thread-safe.
 (defn- pull-seq
-  "Creates a seq of events.  The XMLStreamConstants/SPACE clause below doesn't seem to 
+  "Creates a seq of events.  The XMLStreamConstants/SPACE clause below doesn't seem to
    be triggered by the JDK StAX parser, but is by others.  Leaving in to be more complete."
   [^XMLStreamReader sreader]
   (lazy-seq
@@ -289,7 +289,7 @@
        (cons (event :start-element
                     (keyword (.getLocalName sreader))
                     (attr-hash sreader) nil)
-             (pull-seq sreader)) 
+             (pull-seq sreader))
        XMLStreamConstants/END_ELEMENT
        (cons (event :end-element
                     (keyword (.getLocalName sreader)) nil nil)
@@ -327,9 +327,13 @@
   "Parses the XML InputSource source using a pull-parser. Returns
    a lazy sequence of Event records.  Accepts key pairs
    with XMLInputFactory options, see http://docs.oracle.com/javase/6/docs/api/javax/xml/stream/XMLInputFactory.html
-   and xml-input-factory-props for more information. Defaults coalescing true."
+   and xml-input-factory-props for more information.
+   Defaults coalescing true and supporting-external-entities false."
   [s & {:as props}]
-  (let [fac (new-xml-input-factory (merge {:coalescing true} props))
+  (let [merged-props (merge {:coalescing true
+                             :supporting-external-entities false}
+                            props)
+        fac (new-xml-input-factory merged-props)
         ;; Reflection on following line cannot be eliminated via a
         ;; type hint, because s is advertised by fn parse to be an
         ;; InputStream or Reader, and there are different
@@ -373,7 +377,7 @@
 
     (when (instance? java.io.OutputStreamWriter stream)
       (check-stream-encoding stream (or (:encoding opts) "UTF-8")))
-    
+
     (.writeStartDocument writer (or (:encoding opts) "UTF-8") "1.0")
     (doseq [event (flatten-elements [e])]
       (emit-event event writer))
@@ -395,7 +399,7 @@
 
 (defn indent
   "Emits the XML and indents the result.  WARNING: this is slow
-   it will emit the XML and read it in again to indent it.  Intended for 
+   it will emit the XML and read it in again to indent it.  Intended for
    debugging/testing only."
   [e ^java.io.Writer stream & {:as opts}]
   (let [sw (java.io.StringWriter.)
@@ -410,4 +414,3 @@
   (let [^java.io.StringWriter sw (java.io.StringWriter.)]
     (indent e sw)
     (.toString sw)))
-

src/test/clojure/clojure/data/xml/test_entities.clj

@@ -0,0 +1,49 @@
+;   Copyright (c) Rich Hickey. All rights reserved.
+;   The use and distribution terms for this software are covered by the
+;   Eclipse Public License 1.0 (http://opensource.org/licenses/eclipse-1.0.php)
+;   which can be found in the file epl-v10.html at the root of this distribution.
+;   By using this software in any fashion, you are agreeing to be bound by
+;   the terms of this license.
+;   You must not remove this notice, or any other, from this software.
+
+(ns ^{:doc "Test that external entities are not resolved by default, see https://www.owasp.org/index.php/XML_External_Entity_(XXE)_Processing"
+      :author "Carlo Sciolla"}
+    clojure.data.xml.test-entities
+    (:use clojure.test
+          clojure.data.xml)
+    (:require [clojure.java.io :as io]))
+
+(defn vulnerable-input
+  "Creates an XML with an external entity referring to the given URL"
+  [file-url]
+  (str "<?xml version=\"1.0\" encoding=\"ISO-8859-1\"?>"
+       "<!DOCTYPE foo ["
+       "  <!ELEMENT foo ANY >"
+       "  <!ENTITY xxe SYSTEM \"" file-url  "\" >]>"
+       "<foo>&xxe;</foo>"))
+
+(defn secret-file
+  "Returns the URL to the secret file containing the server root password"
+  []
+  (io/resource "secret.txt"))
+
+(defn parse-vulnerable-file
+  "Parses the vulnerable file, optionally passing the given options to the parser"
+  ([]        (parse-str (vulnerable-input (secret-file))))
+  ([& options] (apply parse-str (vulnerable-input (secret-file)) options)))
+
+(deftest prevent-xxe-by-default
+  (testing "To prevent XXE attacks, exernal entities by default resolve to nil"
+    (let [parsed (parse-vulnerable-file)
+          expected #clojure.data.xml.Element{:tag :foo
+                                             :attrs {}
+                                             :content ()}]
+      (is (= expected parsed)))))
+
+(deftest allow-external-entities-if-required
+  (testing "If explicitly enabled, external entities are property resolved"
+    (let [parsed (parse-vulnerable-file :supporting-external-entities true)
+          expected #clojure.data.xml.Element{:tag :foo
+                                             :attrs {}
+                                             :content ("root_password\n")}]
+      (is (= expected parsed)))))

src/test/resources/secret.txt

@@ -0,0 +1 @@
+root_password