Provide a secure XML parser in SecurityHelpers, use it throughout.

The secure XML parser does not allow entity references to refer to external entities; allowing this exposes an application to XXE (XML External Entity) attacks, where the external reference can be to a local file with sensitive data, whose contents will then appear in the resulting parse error messages. External entities are ignored and will not appear in the parsed or reserialized XML. All of Lift's built-in XML parsing now uses Helpers.secureXML instead of directly using scala.xml.XML, including in tests. More at https://www.owasp.org/index.php/XML_External_Entity_(XXE)_Processing . Signed-off-by: Diego Medina <diego@fmpwizard.com>
lift · Mar 15, 2015 · fb6acf6 · fb6acf6
1 parent 5c54df3
commit fb6acf6
Show file tree

Hide file tree

Showing 7 changed files with 56 additions and 18 deletions.
diff --git a/core/util/src/main/scala/net/liftweb/util/SecurityHelpers.scala b/core/util/src/main/scala/net/liftweb/util/SecurityHelpers.scala
@@ -17,12 +17,17 @@
 package net.liftweb
 package util
 
+import javax.xml.parsers.SAXParserFactory
+
 import org.apache.commons.codec.binary.Base64
-import java.io.{InputStream, ByteArrayOutputStream, ByteArrayInputStream, Reader, File, FileInputStream, BufferedReader}
-import java.security.{SecureRandom, MessageDigest}
+import java.io._
+import java.security._
 import javax.crypto._
 import javax.crypto.spec._
-import scala.xml.{Node, XML}
+
+import scala.xml.{Elem, XML}
+import scala.xml.factory.XMLLoader
+
 import common._
 
 object SecurityHelpers extends StringHelpers with IoHelpers with SecurityHelpers
@@ -197,5 +202,21 @@ trait SecurityHelpers {
     sb.toString
   }
 
+  /**
+   * Provides a secure XML parser, similar to the one provided by
+   * `scala.xml.XML`, but with external entities disabled. This prevents
+   * prevents XXE (XML External Entities) attacks. It is used internally
+   * throughout Lift, and should be used by anyone who is parsing XML from
+   * an untrusted source.
+   */
+  def secureXML: XMLLoader[Elem] = {
+    val parserFactory = SAXParserFactory.newInstance()
+    parserFactory.setNamespaceAware(false)
+    parserFactory.setFeature("http://xml.org/sax/features/external-general-entities", false);
+    parserFactory.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
+
+    val saxParser = parserFactory.newSAXParser();
+    XML.withSAXParser(saxParser)
+  }
 }
 
diff --git a/core/util/src/test/scala/net/liftweb/util/SecurityHelpersSpec.scala b/core/util/src/test/scala/net/liftweb/util/SecurityHelpersSpec.scala
@@ -30,6 +30,24 @@ object SecurityHelpersSpec extends Specification  {
   "SecurityHelpers Specification".title
 
   "Security Helpers" should {
+    "Parse XML with nasty entity references" in {
+      secureXML.loadString("""<?xml version="1.0" encoding="ISO-8859-1"?>
+        <!DOCTYPE foo [
+          <!ELEMENT foo ANY >
+          <!ENTITY xxe SYSTEM "file:///etc/passwd" >]>
+        <foo>&xxe;</foo>"""
+      ) must_== <foo></foo>
+    }
+
+    "Parse XML with deal with nice entity references" in {
+      secureXML.loadString("""<?xml version="1.0" encoding="ISO-8859-1"?>
+        <!DOCTYPE foo [
+          <!ELEMENT foo ANY >
+          <!ENTITY xxe SYSTEM "file:///etc/passwd" >]>
+        <foo>&amp;</foo>
+      """).toString must_== "<foo>&amp;</foo>"
+    }
+
     "provide a randomLong method returning a random Long modulo a number" in {
       randomLong(7L) must be_<(7L)
     }

diff --git a/core/util/src/test/scala/net/liftweb/util/ToHeadSpec.scala b/core/util/src/test/scala/net/liftweb/util/ToHeadSpec.scala
@@ -17,15 +17,13 @@
 package net.liftweb
 package util
 
-import xml.XML._
-
 import org.specs2.matcher.XmlMatchers
 import org.specs2.mutable.Specification
 
 import common._
 import ControlHelpers._
 import HeadHelper._
-
+import Helpers.secureXML
 
 /**
  * Systems under specification for ToHead.
@@ -42,8 +40,8 @@ object ToHeadSpec extends Specification with XmlMatchers {
 
       susfiles must beLike {
         case Full(sus) =>
-          val actual = load(sus._1)
-          val expected = load(sus._2)
+          val actual = secureXML.load(sus._1)
+          val expected = secureXML.load(sus._2)
           mergeToHtmlHead(actual).toString.replaceAll("\\s", "") must_==
           (expected.toString.replaceAll("\\s", ""))
       }
@@ -57,8 +55,8 @@ object ToHeadSpec extends Specification with XmlMatchers {
 
       susfiles must beLike {
         case Full(sus) =>
-          val actual = load(sus._1)
-          val expected = load(sus._2)
+          val actual = secureXML.load(sus._1)
+          val expected = secureXML.load(sus._2)
           mergeToHtmlHead(actual) must ==/(expected)
       }
     }
@@ -71,8 +69,8 @@ object ToHeadSpec extends Specification with XmlMatchers {
 
       susfiles must beLike {
         case Full(sus) =>
-          val actual = load(sus._1)
-          val expected = load(sus._2)
+          val actual = secureXML.load(sus._1)
+          val expected = secureXML.load(sus._2)
           mergeToHtmlHead(actual).toString.replaceAll("\\s", "") must_==
           (expected.toString.replaceAll("\\s", ""))
       }

diff --git a/web/webkit/src/main/scala/net/liftweb/http/Req.scala b/web/webkit/src/main/scala/net/liftweb/http/Req.scala
@@ -1067,7 +1067,7 @@ class Req(val path: ParsePath,
   lazy val forcedBodyAsXml: Box[Elem] = {
     try {
       import java.io._
-      body.map(b => XML.load(new ByteArrayInputStream(b)))
+      body.map(b => secureXML.load(new ByteArrayInputStream(b)))
     } catch {
       case e: LiftFlowOfControlException => throw e
       case e: Exception => Failure(e.getMessage, Full(e), Empty)

diff --git a/web/webkit/src/test/scala/net/liftweb/builtin/snippet/MsgSpec.scala b/web/webkit/src/test/scala/net/liftweb/builtin/snippet/MsgSpec.scala
@@ -23,6 +23,7 @@ import org.specs2.mutable.Specification
 
 import common._
 import http._
+import util.Helpers.secureXML
 
 /**
  * System under specification for Msg.
@@ -43,7 +44,7 @@ object MsgSpec extends Specification with XmlMatchers {
 
         // We reparse due to inconsistencies with UnparsedAttributes
         val result = S.withAttrs(new UnprefixedAttribute("id", Text("foo"), new UnprefixedAttribute("noticeClass", Text("funky"), Null))) {
-          XML.loadString(Msg.render(<div/>).toString)
+          secureXML.loadString(Msg.render(<div/>).toString)
         }
 
         result must ==/(<span id="foo">Error, <span class="funky">Notice</span></span>)

diff --git a/web/webkit/src/test/scala/net/liftweb/builtin/snippet/MsgsSpec.scala b/web/webkit/src/test/scala/net/liftweb/builtin/snippet/MsgsSpec.scala
@@ -17,13 +17,12 @@
 package net.liftweb
 package builtin.snippet
 
-import xml.XML
 import org.specs2.matcher.XmlMatchers
 import org.specs2.mutable.Specification
 
 import common._
 import http._
-
+import util.Helpers.secureXML
 
 /**
  * System under specification for Msgs.
@@ -43,7 +42,7 @@ object MsgsSpec extends Specification with XmlMatchers {
         S.notice("Notice")
 
         // We reparse due to inconsistencies with UnparsedAttributes
-        XML.loadString(Msgs.render(
+        secureXML.loadString(Msgs.render(
           <lift:warning_msg>Warning:</lift:warning_msg><lift:notice_class>funky</lift:notice_class>
         ).toString)
       }

diff --git a/web/webkit/src/test/scala/net/liftweb/http/rest/XMLApiSpec.scala b/web/webkit/src/test/scala/net/liftweb/http/rest/XMLApiSpec.scala
@@ -24,6 +24,7 @@ import org.specs2.mutable.Specification
 import org.specs2.matcher.Matcher
 
 import common._
+import util.Helpers.secureXML
 import util.ControlHelpers.tryo
 
 /**
@@ -89,7 +90,7 @@ object XmlApiSpec extends Specification  {
         /* For some reason, the UnprefixedAttributes that Lift uses to merge in
          * new attributes makes comparison fail. Instead, we simply stringify and
          * reparse the response contents and that seems to fix the issue. */
-        val converted = XML.loadString(x.xml.toString)
+        val converted = secureXML.loadString(x.xml.toString)
         result(converted == expected, 
          "%s matches %s".format(converted,expected),
          "%s does not match %s".format(converted, expected),