SOLR-11477: Disallow resolving of external entities in Lucene querypa…

…rser/xml/CoreParser and SolrCoreParser (defType=xmlparser or {!xmlparser ...}) by default. (Michael Stepankin, Olga Barinova, Uwe Schindler, Christine Poerschke) This closes #263.
apache · Oct 17, 2017 · d8000be · d8000be
1 parent 2f5ecbc
commit d8000be
Show file tree

Hide file tree

Showing 7 changed files with 160 additions and 16 deletions.
diff --git a/lucene/CHANGES.txt b/lucene/CHANGES.txt
@@ -5,11 +5,19 @@ http://s.apache.org/luceneversions
 
 ======================= Lucene 5.5.5 =======================
 
+Changes in Runtime Behavior
+
+* Resolving of external entities in queryparser/xml/CoreParser is disallowed
+  by default. See SOLR-11477 for details.
+
 Bug Fixes
 
 * LUCENE-7419: Fix performance bug with TokenStream.end(), where it would lookup
   PositionIncrementAttribute every time. (Mike McCandless, Robert Muir)
 
+* SOLR-11477: Disallow resolving of external entities in queryparser/xml/CoreParser
+  by default. (Michael Stepankin, Olga Barinova, Uwe Schindler, Christine Poerschke)
+
 ======================= Lucene 5.5.4 =======================
 
 Bug Fixes

diff --git a/lucene/queryparser/src/java/org/apache/lucene/queryparser/xml/CoreParser.java b/lucene/queryparser/src/java/org/apache/lucene/queryparser/xml/CoreParser.java
@@ -22,11 +22,19 @@
 import org.apache.lucene.search.Query;
 import org.w3c.dom.Document;
 import org.w3c.dom.Element;
+import org.xml.sax.EntityResolver;
+import org.xml.sax.ErrorHandler;
+import org.xml.sax.InputSource;
+import org.xml.sax.SAXException;
 
+import javax.xml.XMLConstants;
 import javax.xml.parsers.DocumentBuilder;
 import javax.xml.parsers.DocumentBuilderFactory;
+import javax.xml.parsers.ParserConfigurationException;
 
+import java.io.IOException;
 import java.io.InputStream;
+import java.util.Locale;
 
 /**
  * Assembles a QueryBuilder which uses only core Lucene Query objects
@@ -118,6 +126,10 @@ protected CoreParser(String defaultField, Analyzer analyzer, QueryParser parser)
     queryFactory.addBuilder("SpanNot", snot);
   }
 
+  /**
+   * Parses the given stream as XML file and returns a {@link Query}.
+   * By default this disallows external entities for security reasons.
+   */
   public Query parse(InputStream xmlStream) throws ParserException {
     return getQuery(parseXML(xmlStream).getDocumentElement());
   }
@@ -130,28 +142,63 @@ public void addFilterBuilder(String nodeName, FilterBuilder builder) {
     filterFactory.addBuilder(nodeName, builder);
   }
 
-  private static Document parseXML(InputStream pXmlFile) throws ParserException {
-    DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance();
-    DocumentBuilder db = null;
+  /**
+   * Returns a SAX {@link EntityResolver} to be used by {@link DocumentBuilder}.
+   * By default this returns {@link #DISALLOW_EXTERNAL_ENTITY_RESOLVER}, which disallows the
+   * expansion of external entities (for security reasons). To restore legacy behavior,
+   * override this method to return {@code null}.
+   */
+  protected EntityResolver getEntityResolver() {
+    return DISALLOW_EXTERNAL_ENTITY_RESOLVER;
+  }
+
+  /**
+   * Subclass and override to return a SAX {@link ErrorHandler} to be used by {@link DocumentBuilder}.
+   * By default this returns {@code null} so no error handler is used.
+   * This method can be used to redirect XML parse errors/warnings to a custom logger.
+   */
+  protected ErrorHandler getErrorHandler() {
+    return null;
+  }
+
+  private Document parseXML(InputStream pXmlFile) throws ParserException {
+    final DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance();
+    dbf.setValidating(false);
     try {
-      db = dbf.newDocumentBuilder();
-    }
-    catch (Exception se) {
-      throw new ParserException("XML Parser configuration error", se);
+      dbf.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
+    } catch (ParserConfigurationException e) {
+      // ignore since all implementations are required to support the
+      // {@link javax.xml.XMLConstants#FEATURE_SECURE_PROCESSING} feature
     }
-    org.w3c.dom.Document doc = null;
+    final DocumentBuilder db;
     try {
-      doc = db.parse(pXmlFile);
+      db = dbf.newDocumentBuilder();
+    } catch (Exception se) {
+      throw new ParserException("XML Parser configuration error.", se);
     }
-    catch (Exception se) {
-      throw new ParserException("Error parsing XML stream:" + se, se);
+    try {
+      db.setEntityResolver(getEntityResolver());
+      db.setErrorHandler(getErrorHandler());
+      return db.parse(pXmlFile);
+    } catch (Exception se) {
+      throw new ParserException("Error parsing XML stream: " + se, se);
     }
-    return doc;
   }
 
 
   @Override
   public Query getQuery(Element e) throws ParserException {
     return queryFactory.getQuery(e);
   }
+
+  public static final EntityResolver DISALLOW_EXTERNAL_ENTITY_RESOLVER =
+      new EntityResolver() {
+        @Override
+        public InputSource resolveEntity(String publicId, String systemId) throws SAXException, IOException {
+          throw new SAXException(String.format(Locale.ENGLISH,
+              "External Entity resolving unsupported:  publicId=\"%s\" systemId=\"%s\"",
+              publicId, systemId));
+        }
+      };
+
 }
diff --git a/lucene/queryparser/src/test/org/apache/lucene/queryparser/xml/DOCTYPE_TermQuery.xml b/lucene/queryparser/src/test/org/apache/lucene/queryparser/xml/DOCTYPE_TermQuery.xml
@@ -0,0 +1,19 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE TermQuery SYSTEM "foo://bar.xyz/mydtd">
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one or more
+ contributor license agreements.  See the NOTICE file distributed with
+ this work for additional information regarding copyright ownership.
+ The ASF licenses this file to You under the Apache License, Version 2.0
+ (the "License"); you may not use this file except in compliance with
+ the License.  You may obtain a copy of the License at
+
+     http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+-->
+<TermQuery fieldName="contents">sumitomo</TermQuery>
diff --git a/lucene/queryparser/src/test/org/apache/lucene/queryparser/xml/ENTITY_TermQuery.xml b/lucene/queryparser/src/test/org/apache/lucene/queryparser/xml/ENTITY_TermQuery.xml
@@ -0,0 +1,23 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE TermQuery [
+<!ENTITY internalTerm "sumitomo">
+<!ENTITY externalTerm SYSTEM "foo://bar.xyz/external">
+<!ENTITY % myParameterEntity "foo://bar.xyz/param">
+]>
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one or more
+ contributor license agreements.  See the NOTICE file distributed with
+ this work for additional information regarding copyright ownership.
+ The ASF licenses this file to You under the Apache License, Version 2.0
+ (the "License"); you may not use this file except in compliance with
+ the License.  You may obtain a copy of the License at
+
+     http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+-->
+<TermQuery fieldName="contents">&internalTerm;&externalTerm;</TermQuery>
diff --git a/lucene/queryparser/src/test/org/apache/lucene/queryparser/xml/TestCoreParser.java b/lucene/queryparser/src/test/org/apache/lucene/queryparser/xml/TestCoreParser.java
@@ -23,7 +23,6 @@
 import org.apache.lucene.document.Document;
 import org.apache.lucene.document.Field;
 import org.apache.lucene.document.IntField;
-import org.apache.lucene.index.LeafReaderContext;
 import org.apache.lucene.index.DirectoryReader;
 import org.apache.lucene.index.IndexReader;
 import org.apache.lucene.index.IndexWriter;
@@ -35,15 +34,13 @@
 import org.apache.lucene.store.Directory;
 import org.apache.lucene.util.LuceneTestCase;
 import org.junit.AfterClass;
-import org.junit.Assume;
 import org.junit.BeforeClass;
 
 import java.io.BufferedReader;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.InputStreamReader;
 import java.nio.charset.StandardCharsets;
-import java.util.List;
 
 
 public class TestCoreParser extends LuceneTestCase {
@@ -101,6 +98,32 @@ public void testSimpleXML() throws ParserException, IOException {
     dumpResults("TermQuery", q, 5);
   }
 
+  public void test_DOCTYPE_TermQueryXML() throws ParserException, IOException {
+    try {
+      parse("DOCTYPE_TermQuery.xml");
+      fail("should have thrown a ParserException!");
+    }
+    catch (ParserException saxe) {
+      assertTrue(saxe.getMessage().equals(
+          "Error parsing XML stream: org.xml.sax.SAXException: External Entity resolving unsupported:  publicId=\"null\" systemId=\"foo://bar.xyz/mydtd\""));
+      return;
+    }
+    fail("should have thrown a ParserException!");
+  }
+
+  public void test_ENTITY_TermQueryXML() throws ParserException, IOException {
+    try {
+      parse("ENTITY_TermQuery.xml");
+      fail("should have thrown a ParserException!");
+    }
+    catch (ParserException saxe) {
+      assertTrue(saxe.getMessage().equals(
+          "Error parsing XML stream: org.xml.sax.SAXException: External Entity resolving unsupported:  publicId=\"null\" systemId=\"foo://bar.xyz/external\""));
+      return;
+    }
+    fail("should have thrown a ParserException!");
+  }
+
   public void testSimpleTermsQueryXML() throws ParserException, IOException {
     Query q = parse("TermsQuery.xml");
     dumpResults("TermsQuery", q, 5);

diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
@@ -21,10 +21,20 @@ Apache UIMA 2.3.1
 Apache ZooKeeper 3.4.6
 Jetty 9.2.13.v20150730
 
+Upgrade Notes
+----------------------
+
+* SOLR-11477: in the XML query parser (defType=xmlparser or {!xmlparser ... })
+  the resolving of external entities is now disallowed by default.
+
 Bug Fixes
 ----------------------
 
-* SOLR-10420: Solr 6.x leaking one SolrZkClient instance per second (Scott Blum, Cao Manh Dat, Markus Jelsma, Steve Rowe)
+* SOLR-10420: Leaking one SolrZkClient instance per second (Scott Blum, Cao Manh Dat, Markus Jelsma, Steve Rowe)
+
+* SOLR-11477: Disallow resolving of external entities in the XML query parser (defType=xmlparser).
+  (Michael Stepankin, Olga Barinova, Uwe Schindler, Christine Poerschke)
+
 
 ======================= 5.5.4 =======================
 

diff --git a/solr/core/src/java/org/apache/solr/search/SolrCoreParser.java b/solr/core/src/java/org/apache/solr/search/SolrCoreParser.java
@@ -16,17 +16,26 @@
  */
 package org.apache.solr.search;
 
+import java.lang.invoke.MethodHandles;
+
 import org.apache.lucene.analysis.Analyzer;
 import org.apache.lucene.queryparser.xml.CoreParser;
 
 import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.common.util.XMLErrorLogger;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.xml.sax.ErrorHandler;
 
 /**
  * Assembles a QueryBuilder which uses Query objects from Solr's <code>search</code> module
  * in addition to Query objects supported by the Lucene <code>CoreParser</code>.
  */
 public class SolrCoreParser extends CoreParser {
 
+  private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+  private static final XMLErrorLogger xmllog = new XMLErrorLogger(log);
+
   public SolrCoreParser(String defaultField, Analyzer analyzer,
       SolrQueryRequest req) {
     super(defaultField, analyzer);
@@ -35,4 +44,9 @@ public SolrCoreParser(String defaultField, Analyzer analyzer,
     // lucene_parser.addQueryBuilder("SomeOtherQuery", new SomeOtherQueryBuilder(schema));
   }
 
+  @Override
+  protected ErrorHandler getErrorHandler() {
+    return xmllog;
+  }
+
 }