Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 5.3
    • Fix Version/s: 5.4, 6.0
    • Component/s: UI
    • Labels:
      None

      Description

      Use the drop-down in the left menu to select a core. Use the “Watch Changes” feature under the “Plugins / Stats” option. When submitting the changes, XML is passed in the “stream.body” parameter and is vulnerable to XXE.

      1. SOLR-8307.patch
        8 kB
        Erik Hatcher
      2. SOLR-8307.patch
        1 kB
        Shawn Heisey

        Activity

        Hide
        Shawn Heisey added a comment -

        I looked up how to fix this. I think this patch might do it.

        Show
        Shawn Heisey added a comment - I looked up how to fix this. I think this patch might do it.
        Hide
        Shawn Heisey added a comment -

        I'm wondering whether my patch might disable xinclude in Solr's config files. My solrconfig.xml file uses xinclude extensively.

        Show
        Shawn Heisey added a comment - I'm wondering whether my patch might disable xinclude in Solr's config files. My solrconfig.xml file uses xinclude extensively.
        Hide
        Shawn Heisey added a comment -

        I patched the 5.3.2 snapshot I'm trying out, and my solr install is still handling my config with no problem, so it appears that this doesn't break the xlmns for xinclude.

        Show
        Shawn Heisey added a comment - I patched the 5.3.2 snapshot I'm trying out, and my solr install is still handling my config with no problem, so it appears that this doesn't break the xlmns for xinclude.
        Hide
        Shawn Heisey added a comment -

        I found more instances of XMLInputFactory in the codebase. Here are the classes:

        org.apache.solr.handler.DocumentAnalysisRequestHandler
        org.apache.solr.handler.XmlUpdateRequestHandlerTest
        org.apache.solr.handler.dataimport.XPathRecordReader
        org.apache.solr.handler.loader.XMLLoader
        org.apache.solr.update.AddBlockUpdateTest
        org.apache.solr.util.EmptyEntityResolver

        I do not know if all of these need attention or if some could be left alone.

        It also appear that other XML parsers are in use. I found SAXParserFactory and DocumentBuilderFactory with grep. The xerces library is in the ivy config. My little patch isn't going to be enough. This is the document I'm using for information:

        https://www.owasp.org/index.php/XML_External_Entity_%28XXE%29_Processing

        Show
        Shawn Heisey added a comment - I found more instances of XMLInputFactory in the codebase. Here are the classes: org.apache.solr.handler.DocumentAnalysisRequestHandler org.apache.solr.handler.XmlUpdateRequestHandlerTest org.apache.solr.handler.dataimport.XPathRecordReader org.apache.solr.handler.loader.XMLLoader org.apache.solr.update.AddBlockUpdateTest org.apache.solr.util.EmptyEntityResolver I do not know if all of these need attention or if some could be left alone. It also appear that other XML parsers are in use. I found SAXParserFactory and DocumentBuilderFactory with grep. The xerces library is in the ivy config. My little patch isn't going to be enough. This is the document I'm using for information: https://www.owasp.org/index.php/XML_External_Entity_%28XXE%29_Processing
        Hide
        Erik Hatcher added a comment -

        At a quick glance, it looks like XMLResponseParser ought to

        EmptyEntityResolver.configureXMLInputFactory(factory);
        

        That's something that Uwe Schindler probably put in to prevent this issue in other places.

        Show
        Erik Hatcher added a comment - At a quick glance, it looks like XMLResponseParser ought to EmptyEntityResolver.configureXMLInputFactory(factory); That's something that Uwe Schindler probably put in to prevent this issue in other places.
        Hide
        Uwe Schindler added a comment -

        Hi,
        it should use the code pattern as Erik told. Disabling DTDs completly is not a good idea.

        In general all XML parsing of resources coming from network should follow the same pattern. The EmptyEntityResolver has methods for all types of XML parsers to disable external entities, so use it's methods to configure. Grep on EmptyEntityResolver and you will see that all of the above listed parsers are fine (unless somebody broke them again).

        Please note: This only affects XML coming from the network. Please don't disable xinclude or external entities in Solr's config files. Those should not be accessible through internet anyways, if they are you have bigger problems. It is a officially documented feature that you can ue xinclude and external entities to split your solr config files (I generally place the field types and fields each in a separate XML file and include them into the schema).

        Show
        Uwe Schindler added a comment - Hi, it should use the code pattern as Erik told. Disabling DTDs completly is not a good idea. In general all XML parsing of resources coming from network should follow the same pattern. The EmptyEntityResolver has methods for all types of XML parsers to disable external entities, so use it's methods to configure. Grep on EmptyEntityResolver and you will see that all of the above listed parsers are fine (unless somebody broke them again). Please note: This only affects XML coming from the network. Please don't disable xinclude or external entities in Solr's config files. Those should not be accessible through internet anyways, if they are you have bigger problems. It is a officially documented feature that you can ue xinclude and external entities to split your solr config files (I generally place the field types and fields each in a separate XML file and include them into the schema).
        Hide
        Uwe Schindler added a comment -

        The patch attached here just modifies SolrJ. How is this related to config file parsing?

        Show
        Uwe Schindler added a comment - The patch attached here just modifies SolrJ. How is this related to config file parsing?
        Hide
        Uwe Schindler added a comment - - edited

        I checked the code: Where is the XXE risk? The stream.body is going through a safe parser. So do you have a testcase? How did you find out that there is an XXE issue? I spent a whole week 2 years ago on fixing all this problems, so how could they reappear? There are also tests that check to prevent XXE at some places!

        The attached patch only fixes SolrJ, but this is not really a security issue, because it is used to connect to Solr and not arbitrary web sites.

        Show
        Uwe Schindler added a comment - - edited I checked the code: Where is the XXE risk? The stream.body is going through a safe parser. So do you have a testcase? How did you find out that there is an XXE issue? I spent a whole week 2 years ago on fixing all this problems, so how could they reappear? There are also tests that check to prevent XXE at some places! The attached patch only fixes SolrJ, but this is not really a security issue, because it is used to connect to Solr and not arbitrary web sites.
        Hide
        Shawn Heisey added a comment -

        The patch attached here just modifies SolrJ. How is this related to config file parsing?

        I'm flailing in the dark here, and obviously do not really understand the implications of the code examples I found. The mbeans handler is what was mentioned in the bug report, so I followed that, and it uses XMLResponseParser, so that's what I modified. I'm not at all surprised that there's a better way.

        Show
        Shawn Heisey added a comment - The patch attached here just modifies SolrJ. How is this related to config file parsing? I'm flailing in the dark here, and obviously do not really understand the implications of the code examples I found. The mbeans handler is what was mentioned in the bug report, so I followed that, and it uses XMLResponseParser, so that's what I modified. I'm not at all surprised that there's a better way.
        Hide
        Erik Hatcher added a comment -

        Uwe Schindler looks like the diff feature of the admin UI sends XML (it got from Solr) to SolrInfoMBeanHandler with diff=true. And then that XML is parsed by XMLResponseParser. Looks like a legit vector.

        Show
        Erik Hatcher added a comment - Uwe Schindler looks like the diff feature of the admin UI sends XML (it got from Solr) to SolrInfoMBeanHandler with diff=true. And then that XML is parsed by XMLResponseParser. Looks like a legit vector.
        Hide
        Shawn Heisey added a comment -

        Thank you for taking a look and rescuing me from my lack of knowledge. I appreciate the things I learn from my colleagues here.

        Show
        Shawn Heisey added a comment - Thank you for taking a look and rescuing me from my lack of knowledge. I appreciate the things I learn from my colleagues here.
        Hide
        Uwe Schindler added a comment -

        OK. So a misuse of response parser. This is why it is a Problem. Thanks. I would fix this with the entity resolver. Nothing more to do.

        Show
        Uwe Schindler added a comment - OK. So a misuse of response parser. This is why it is a Problem. Thanks. I would fix this with the entity resolver. Nothing more to do.
        Hide
        Erik Hatcher added a comment - - edited

        Here's a patch that does `EmptyEntityResolver.configureXMLInputFactory` for the SolrInfoMBeanHandler diff feature, including test case that fails without the fix.

        My patch includes a move of EmptyEntityResolver from solr-core to solrj too

        Show
        Erik Hatcher added a comment - - edited Here's a patch that does `EmptyEntityResolver.configureXMLInputFactory` for the SolrInfoMBeanHandler diff feature, including test case that fails without the fix. My patch includes a move of EmptyEntityResolver from solr-core to solrj too
        Hide
        Erik Hatcher added a comment -

        Addressing Shawn Heisey's list above:

        • org.apache.solr.handler.DocumentAnalysisRequestHandler: uses EmptyEntityResolver appropriately
        • org.apache.solr.handler.XmlUpdateRequestHandlerTest: this is a test, so no concern (but it does not use EmptyEntityResolver)
        • org.apache.solr.handler.dataimport.XPathRecordReader: uses EmptyEntityResolver
        • org.apache.solr.handler.loader.XMLLoader: uses EmptyEntityResolver
        • org.apache.solr.update.AddBlockUpdateTest:another test, so no concern, but it also does not use EmptyEntityResolver
        • org.apache.solr.util.EmptyEntityResolver: this is the fix to potentially evil external entity references

        So all those look fine.

        Show
        Erik Hatcher added a comment - Addressing Shawn Heisey 's list above: org.apache.solr.handler.DocumentAnalysisRequestHandler: uses EmptyEntityResolver appropriately org.apache.solr.handler.XmlUpdateRequestHandlerTest: this is a test, so no concern (but it does not use EmptyEntityResolver) org.apache.solr.handler.dataimport.XPathRecordReader: uses EmptyEntityResolver org.apache.solr.handler.loader.XMLLoader: uses EmptyEntityResolver org.apache.solr.update.AddBlockUpdateTest:another test, so no concern, but it also does not use EmptyEntityResolver org.apache.solr.util.EmptyEntityResolver: this is the fix to potentially evil external entity references So all those look fine.
        Hide
        Erik Hatcher added a comment -

        Solr's "ant test" passed locally. I'll commit to trunk and branch_5x in the next day or two, barring any objections.

        Show
        Erik Hatcher added a comment - Solr's "ant test" passed locally. I'll commit to trunk and branch_5x in the next day or two, barring any objections.
        Hide
        Erik Hatcher added a comment -

        Forgot to mention JIRA ticket on commit message. Committed:

        Show
        Erik Hatcher added a comment - Forgot to mention JIRA ticket on commit message. Committed: trunk r1715863: http://svn.apache.org/viewvc?rev=1715863&view=rev branch_5x r1715864: http://svn.apache.org/viewvc?rev=1715864&view=rev
        Hide
        Hoss Man added a comment -

        the commits made for this issue have broken trunk and 5x builds due to causing the javadocs to produce broken links.

        some of the affected classes have fundamental problems that can/should be fixed in SOLR-8333, but independent of that this commit – and the creation of solr/solrj/src/java/org/apache/solr/util/ which competes with solr/core/src/java/org/apache/solr/util/ – are breaking the build.

        erik: can you please revert this until a better solution is found?(i thought you mentioned earlier today that you would do this – but that was ~8 hours ago and i've seen you make several other commits & jira comments since then w/o actually addressing the immediate problem, so i'm asking you now explicitly: please revert until this issue can be fixed in a way that does not break the build.

        Show
        Hoss Man added a comment - the commits made for this issue have broken trunk and 5x builds due to causing the javadocs to produce broken links. some of the affected classes have fundamental problems that can/should be fixed in SOLR-8333 , but independent of that this commit – and the creation of solr/solrj/src/java/org/apache/solr/util/ which competes with solr/core/src/java/org/apache/solr/util/ – are breaking the build. erik: can you please revert this until a better solution is found?(i thought you mentioned earlier today that you would do this – but that was ~8 hours ago and i've seen you make several other commits & jira comments since then w/o actually addressing the immediate problem, so i'm asking you now explicitly: please revert until this issue can be fixed in a way that does not break the build.
        Hide
        ASF subversion and git services added a comment -

        Commit 1716007 from Erik Hatcher in branch 'dev/trunk'
        [ https://svn.apache.org/r1716007 ]

        SOLR-8307: move EmptyEntityResolver to another package to avoid conflict with solr-core and javadoc generation

        Show
        ASF subversion and git services added a comment - Commit 1716007 from Erik Hatcher in branch 'dev/trunk' [ https://svn.apache.org/r1716007 ] SOLR-8307 : move EmptyEntityResolver to another package to avoid conflict with solr-core and javadoc generation
        Hide
        ASF subversion and git services added a comment -

        Commit 1716008 from Erik Hatcher in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1716008 ]

        SOLR-8307: move EmptyEntityResolver to another package to avoid conflict with solr-core and javadoc generation (merged from trunk r1716007)

        Show
        ASF subversion and git services added a comment - Commit 1716008 from Erik Hatcher in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1716008 ] SOLR-8307 : move EmptyEntityResolver to another package to avoid conflict with solr-core and javadoc generation (merged from trunk r1716007)
        Hide
        Erik Hatcher added a comment -

        Chris Hostetter (Unused) - should be fixed now. I moved EmptyEntityResolver to the common package to as to not overlap.

        Do we need to create a solr-core version of this class (in the util) package to keep the same fully qualified classname for this public class? I'm ok with it changing, and documenting it in CHANGES. Objections or suggestions?

        Show
        Erik Hatcher added a comment - Chris Hostetter (Unused) - should be fixed now. I moved EmptyEntityResolver to the common package to as to not overlap. Do we need to create a solr-core version of this class (in the util) package to keep the same fully qualified classname for this public class? I'm ok with it changing, and documenting it in CHANGES. Objections or suggestions?
        Hide
        Erik Hatcher added a comment -

        Uwe Schindler - what do you think about the public EmptyEntityResolver moving to another package? Do you think we should create a back compatible deprecated one in the same place? I can't imagine it is being used externally. I'll re-open this issue and document the change at least, and add a copy of it back if desired.

        Show
        Erik Hatcher added a comment - Uwe Schindler - what do you think about the public EmptyEntityResolver moving to another package? Do you think we should create a back compatible deprecated one in the same place? I can't imagine it is being used externally. I'll re-open this issue and document the change at least, and add a copy of it back if desired.
        Hide
        Uwe Schindler added a comment -

        I am fine with that. I don't think we need backwards compatibility.

        Show
        Uwe Schindler added a comment - I am fine with that. I don't think we need backwards compatibility.
        Hide
        Erik Hatcher added a comment - - edited

        Thanks Uwe Schindler. Documented in CHANGES and committed:

        Show
        Erik Hatcher added a comment - - edited Thanks Uwe Schindler . Documented in CHANGES and committed: branch_5x: r1716161 https://svn.apache.org/r1716161 trunk: r1716160 https://svn.apache.org/r1716160

          People

          • Assignee:
            Erik Hatcher
            Reporter:
            Adam Johnson
          • Votes:
            1 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development