SOLR-16296: Load elevate.xml in a more secure way #962

heythm · 2022-08-04T01:52:22Z

https://issues.apache.org/jira/browse/SOLR-16296

This removes the use of XmlConfigFile by QueryElevationComponent.

solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java

dsmiley · 2022-08-05T22:19:13Z

solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java

+      inputStream = core.getResourceLoader().openResource(configFileName);
+      xmlDocument = SafeXMLParsing.parseUntrustedXML(log, inputStream);


I think I completely overlooked something (my bad). I was initially concerned about using XmlConfigFile because it did not use Solr's SafeXMLParsing utility, and I could also see it enabling some XInclude & entity resolver stuff, which concerned me in its use for elevate.xml. But this doesn't mean it's not "safe". Upon further inspection, it appears to be doing some of the same stuff that SafeXMLParsing does in the parseConfigXML method, which is relatively safe as it doesn't escape the resource loader. Do you see @HaythemKh ? If you agree, do you think XmlConfigFile could be modified to call SafeXMLParsing.parseConfigXml so that the safe-ness is clear? If not, we could at least leave a comment there.

At this place in QEC, we could replace these two lines with SafeXMLParsing.parseConfigXml and we would in fact continue to support these somewhat exotic XML features in a "safe" way.

About modifying XmlConfigFile with SafeXMLParsing#parseConfigXML; It is indeed possible to apply these changes. However, in XmlConfigFile, we authorize the non-availability of SystemId so as not to activate xinclude for this purpose. Unlike SafeXMLParsing#parseConfigXML where we enable it by default. And, XmlConfigFile is used for reading Solr config files (see SolrConfig#readXml). would that have some impact?

SafeXMLParsing#parseConfigXML enables xinclude by default, whereas SafeXMLParsing#parseUntrustedXML doesn't. It doesn't seem very "safe", is it? I think if we don't need to activate it we don't have to.

@uschindler -- you added the SafeXMLParsing library. My instinct is use use the parseConfigXML method for the elevate.xml because it conveniently takes a SolrResourceLoader. It supports x:include too but so-what; right? It's scoped to the SRL so I don't see how it could be a problem.

Yes, i think that's the way to go. parseConfigXML is for stuff that is part of Solr's config. parseUntrustedXML is for stuff coming over the network like a POSTed document.
As all Solr config files supoort xinclude, it should also be supported for elevantion.xml

About modifying XmlConfigFile with SafeXMLParsing#parseConfigXML; It is indeed possible to apply these changes. However, in XmlConfigFile, we authorize the non-availability of SystemId so as not to activate xinclude for this purpose. Unlike SafeXMLParsing#parseConfigXML where we enable it by default. And, XmlConfigFile is used for reading Solr config files (see SolrConfig#readXml). would that have some impact?

We have a valid system id because the ResourceLoader provides one. XInclude will then go through SystemIdResolver provided.

SafeXMLParsing#parseConfigXML enables xinclude by default, whereas SafeXMLParsing#parseUntrustedXML doesn't. It doesn't seem very "safe", is it? I think if we don't need to activate it we don't have to.

If it is a config file, xinclude should be allowed, as this is the standard for SOlr config files. There's also no security risk in this because the xinclude only allows to load files from resource loader and not somewhere outside (it uses SystemIdResolver to resolve includes, see

solr/solr/core/src/java/org/apache/solr/util/SystemIdResolver.java

Lines 154 to 156 in c99af20

throw new IOException(

"Cannot resolve absolute systemIDs / external entities (only relative paths work): "

+ systemId);

).

Of course if the elevate.xml is coming over the network as part of the request, then it should be parsed with untrusted.

I am not familar with the code, but the rule for config files is:

if the config file is XML in ResourceLoader, it should use SafeXMLParsing#parseConfigXML with xinclude enabled for consistency

if the elevation file is passed as part of request body, then parse it using the request input stream using SafeXMLParsing#parseUntrustedXML

I'd tune XmlConfigFile to use those APIs. Back at that time it was too much work to me, so I left that one out. I just added the SystemIdResolver.

Thanks @uschindler for the detailed context!

solr/CHANGES.txt

solr/core/src/java/org/apache/solr/core/XmlConfigFile.java

solr/core/src/java/org/apache/solr/core/SolrXmlConfig.java

solr/core/src/java/org/apache/solr/schema/IndexSchemaFactory.java

solr/core/src/java/org/apache/solr/core/SolrConfig.java

updated code, needs new review.

heythm · 2022-08-29T20:00:50Z

solr/core/src/test/org/apache/solr/core/TestConfigSets.java

    assertThat(
        config.getConfigSetBaseDirectory().toAbsolutePath(),
        is(Paths.get("/path/to/solr/home/configsets").toAbsolutePath()));

-    NodeConfig absConfig =
-        SolrXmlConfig.fromString(


I think we still need a XmlConfigFile constructor taking an inputstream as input

heythm · 2022-08-29T21:10:54Z

I have done some more local refactorings but I'm thinking about either keeping an XmlConfigFile constructor taking an inputstream as input or continuing the refactorings.

dsmiley

I have done some more local refactorings but I'm thinking about either keeping an XmlConfigFile constructor taking an inputstream as input or continuing the refactorings.

If the PR doesn't get out of hand too much (i.e. increases scope a lot). Your latest change definitely did but maybe it was WIP/draft.

solr/core/src/java/org/apache/solr/core/NodeConfig.java

solr/core/src/java/org/apache/solr/core/SolrConfig.java

solr/core/src/java/org/apache/solr/core/SolrXmlConfig.java

solr/test-framework/src/java/org/apache/solr/util/TestHarness.java

solr/core/src/test/org/apache/solr/metrics/reporters/SolrSlf4jReporterTest.java

solr/core/src/test/org/apache/solr/metrics/reporters/SolrGraphiteReporterTest.java

This reverts commit a820383.

solr/core/src/java/org/apache/solr/core/XmlConfigFile.java

solr/core/src/java/org/apache/solr/util/SafeXMLParsing.java

solr/core/src/java/org/apache/solr/core/XmlConfigFile.java

solr/core/src/java/org/apache/solr/core/SolrConfig.java

dsmiley

Thanks for scaling back the changes to just a few files. Tests pass for me; I think this is ready to merge.

Refactorings to simplify the unwieldily constructor parameters of XmlConfigFile can happen in follow-on work. And it's inconsistent/weird that XmlConfigFile only sets the zkVersion field when it's passed a fileSupplier in the constructor.

solr/CHANGES.txt

dsmiley · 2022-09-02T18:02:38Z

Waiting for some input from @noblepaul to deconflict his recent PRs.

dsmiley · 2022-09-06T06:18:16Z

@noblepaul asked:

BTW can you please take a look at how the work done in #962 can be applied to the other XML files as well

Do you mean, to not use XmlConfigFile at all? It's on a case-by-case basis what work is involved there. I think your XPath removing work could have incorporated the dis-use of XmlConfigFile because the former logic used many methods of XmlConfigFile whereas afterwards it's only used to pass along the XML Document. At that point, there's little need of XmlConfigFile; it could be replaced with some utility method on, say, DOMConfigNode that takes some of the same parameters of XmlConfigFile's constructors. Any way, we could have a new JIRA issue for exactly that.

heythm · 2022-09-06T10:18:50Z

Update: I will limit the scope of this work to only removing the use of XmlConfigFile from QEC following up on this discussion. And once that is merged I will open a new PR for the removal of the use of XPath.

epugh · 2022-09-06T11:48:14Z

Thanks for your persistence on this @heythm....

heythm · 2022-09-06T14:33:55Z

@noblepaul - I want your opinion on merging this PR? This now only includes not using XmlConfigFile in QEC, but also a small cleanup of XmlConfigFile/SafeXMLParsing for handling untrusted/trusted files.

Even if we want to delete XmlConfigFile in the long run, it wouldn't hurt not to lose those changes.

solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java

…ponent (#962) Really a refactoring; no change in behavior or actual safety. Co-authored-by: David Smiley <dsmiley@apache.org>

remove the use of XmlConfigFile by QueryElevationComponent

9af03d2

madrob reviewed Aug 4, 2022

View reviewed changes

solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java Outdated Show resolved Hide resolved

dsmiley reviewed Aug 4, 2022

View reviewed changes

madrob reviewed Aug 4, 2022

View reviewed changes

heythm added 2 commits August 5, 2022 10:27

reduce change noise

c4b173f

upsurge loadElevationProvider's old flow

241cf9a

dsmiley approved these changes Aug 5, 2022

View reviewed changes

dsmiley reviewed Aug 5, 2022

View reviewed changes

heythm added 8 commits August 22, 2022 13:35

remove origDoc as it is not used

1bbcca5

remove not thrown exceptions

a30a608

remove not used constructors

a612674

use #parseConfigXML instead of #parseUntrustedXML

30f2544

revert inputstream changes

099ca65

remove useless import

7ef8572

add CHANGES.txt entry

2cc19f0

Merge branch 'main' into hkhiri/elevate

9b817dd

uschindler previously approved these changes Aug 27, 2022

View reviewed changes

dsmiley reviewed Aug 29, 2022

View reviewed changes

solr/CHANGES.txt Outdated Show resolved Hide resolved

solr/core/src/java/org/apache/solr/core/XmlConfigFile.java Outdated Show resolved Hide resolved

heythm added 4 commits August 29, 2022 15:39

update CHANGES.TXT

7c00f88

refactor XmlConfigFile constructor

9132b22

update XmlConfigFile javadoc

3d8b43e

cleanup and alignment

a820383

uschindler reviewed Aug 29, 2022

View reviewed changes

solr/core/src/java/org/apache/solr/core/SolrXmlConfig.java Outdated Show resolved Hide resolved

solr/core/src/java/org/apache/solr/schema/IndexSchemaFactory.java Outdated Show resolved Hide resolved

solr/core/src/java/org/apache/solr/core/SolrConfig.java Outdated Show resolved Hide resolved

some refactoring

e04f786

heythm commented Aug 29, 2022

View reviewed changes

dsmiley requested changes Aug 29, 2022

View reviewed changes

heythm added 2 commits August 30, 2022 10:14

Revert "cleanup and alignment"

b1ab682

This reverts commit a820383.

add InputSource as input to XmlConfigFile constructor

194a413

docs

926d75d

dsmiley requested changes Aug 30, 2022

View reviewed changes

heythm added 5 commits August 30, 2022 15:11

use Objects.requireNonNull

37c0d6d

bring back XmlConfigFile small part of the doc

8769836

reorder method params

bc77e79

handle UnsupportedOperationException at lowest level

23e25aa

formatting

ae680ce

dsmiley reviewed Aug 30, 2022

View reviewed changes

solr/core/src/java/org/apache/solr/core/XmlConfigFile.java Outdated Show resolved Hide resolved

solr/core/src/java/org/apache/solr/core/SolrConfig.java Outdated Show resolved Hide resolved

heythm added 4 commits August 30, 2022 17:41

update requireNonNull

a6315f7

handle ResourceProvider

30f26d5

cleanup

c6a4bc9

add check on inputsource==null

af80019

dsmiley approved these changes Aug 30, 2022

View reviewed changes

heythm mentioned this pull request Sep 2, 2022

SOLR-16369: avoid xpath in parsing elevate.xml #994

Closed

heythm commented Sep 2, 2022

View reviewed changes

solr/CHANGES.txt Outdated Show resolved Hide resolved

enrich CHANGES.txt

9cc3b6e

remove xpath changes from branch to move them to another

cea0ced

dsmiley reviewed Sep 6, 2022

View reviewed changes

solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java Show resolved Hide resolved

add checke on the <elevate> root tag of elevate.xml

99e3c71

noblepaul approved these changes Sep 7, 2022

View reviewed changes

heythm and others added 2 commits September 7, 2022 06:54

Merge branch 'main' into hkhiri/elevate

73a26c9

Update CHANGES.txt

bdc1181

dsmiley merged commit 66c784f into apache:main Sep 7, 2022

heythm deleted the hkhiri/elevate branch September 7, 2022 16:40

dsmiley added a commit that referenced this pull request Sep 7, 2022

SOLR-16296: Use SafeXMLParsing in XmlConfigFile and QueryElevationCom…

2e55183

…ponent (#962) Really a refactoring; no change in behavior or actual safety. Co-authored-by: David Smiley <dsmiley@apache.org>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SOLR-16296: Load elevate.xml in a more secure way #962

SOLR-16296: Load elevate.xml in a more secure way #962

heythm commented Aug 4, 2022 •

edited

dsmiley Aug 5, 2022

heythm Aug 22, 2022 •

edited

dsmiley Aug 22, 2022

uschindler Aug 22, 2022

uschindler Aug 22, 2022 •

edited

uschindler Aug 22, 2022 •

edited

heythm Aug 27, 2022

heythm Aug 29, 2022 •

edited

heythm commented Aug 29, 2022

dsmiley left a comment

dsmiley left a comment

dsmiley commented Sep 2, 2022

dsmiley commented Sep 6, 2022

heythm commented Sep 6, 2022

epugh commented Sep 6, 2022

heythm commented Sep 6, 2022 •

edited

		inputStream = core.getResourceLoader().openResource(configFileName);
		xmlDocument = SafeXMLParsing.parseUntrustedXML(log, inputStream);

	throw new IOException(
	"Cannot resolve absolute systemIDs / external entities (only relative paths work): "
	+ systemId);

SOLR-16296: Load elevate.xml in a more secure way #962

SOLR-16296: Load elevate.xml in a more secure way #962

Conversation

heythm commented Aug 4, 2022 • edited

dsmiley Aug 5, 2022

Choose a reason for hiding this comment

heythm Aug 22, 2022 • edited

Choose a reason for hiding this comment

dsmiley Aug 22, 2022

Choose a reason for hiding this comment

uschindler Aug 22, 2022

Choose a reason for hiding this comment

uschindler Aug 22, 2022 • edited

Choose a reason for hiding this comment

uschindler Aug 22, 2022 • edited

Choose a reason for hiding this comment

heythm Aug 27, 2022

Choose a reason for hiding this comment

heythm Aug 29, 2022 • edited

Choose a reason for hiding this comment

heythm commented Aug 29, 2022

dsmiley left a comment

Choose a reason for hiding this comment

dsmiley left a comment

Choose a reason for hiding this comment

dsmiley commented Sep 2, 2022

dsmiley commented Sep 6, 2022

heythm commented Sep 6, 2022

epugh commented Sep 6, 2022

heythm commented Sep 6, 2022 • edited

heythm commented Aug 4, 2022 •

edited

heythm Aug 22, 2022 •

edited

uschindler Aug 22, 2022 •

edited

uschindler Aug 22, 2022 •

edited

heythm Aug 29, 2022 •

edited

heythm commented Sep 6, 2022 •

edited