SOLR-14886 : suppress stack traces in query response #1632

igiguere · 2023-05-08T19:14:59Z

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

Description

Stack traces are considered a security risk. Please refer to OWASP for a full explanation:

https://owasp.org/Top10/A05_2021-Security_Misconfiguration/

Solution

Add a property "hideStackTrace" to solr.xml. In NodeConfig, the default value is "false", for back-compatibility.

Use the new property in ResponseUtils, to print out, or not, the stack trace.

Adapt code that calls ResponseUtils.

Also check if stack traces should be hidden everywhere the "trace" element is added to a response. (I searched for the string"trace" - with quotes - throughout the Solr code. Hope I got everything.)

Tests

org.apache.solr.servlet.HideStackTraceTest : force an exception into the response, and assert that if hideStackTrace=true, the stack trace is not shown.

Checklist

Please review the following and check all that apply:

I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
I have created a Jira issue and added the issue ID to my pull request title.
I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
I have developed this patch against the main branch.
I have run ./gradlew check.
I have added tests for my changes.
I have added documentation for the Reference Guide

solr/core/src/java/org/apache/solr/servlet/ResponseUtils.java

solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java

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

solr/core/src/java/org/apache/solr/rest/BaseSolrResource.java

solr/core/src/java/org/apache/solr/servlet/ResponseUtils.java

solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java

solr/core/src/java/org/apache/solr/servlet/ResponseUtils.java

solr/core/src/test/org/apache/solr/servlet/HideStackTraceTest.java

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

dsmiley

While I can understand that Solr needs NodeConfig / solr.xml, this feels way more like a simple Java system property situation. We certainly have many of the latter.

Maybe someday we would have a unification in a generic way so that we needn't go through all the NodeConfig plumbing just to have some global flag.

stillalex · 2023-08-25T17:41:50Z

@igiguere could you rebase on top of main branch (as suggested on the dev list)?
also what name should I use in the CHANGES.txt entry for the credit?

igiguere · 2023-08-25T18:33:35Z

@stillalex : I'm in CHANGES.txt a couple of times with my full name
Isabelle Giguere

igiguere · 2023-08-25T19:57:37Z

I rebased this branch on the fork's main (up to date with /solr main). There were no changes to push, so I don't know if a build will be triggered.
Hopefully, it will work.

stillalex · 2023-08-25T20:11:20Z

I'm not seeing a new build, I think you have to force push the rebase so the build can apply correctly

dsmiley

See org.apache.solr.search.json.TestJsonRequestWithEdismaxDefType to see a new breed of tests. Notice the SolrClientTestRule. Notice a ConfigRequest is used to manipulate a configuration instead of creating yet another config file. We have too many such config files; it's a maintenance problem. Feedback on this is welcome!

solr/core/src/test/org/apache/solr/servlet/HideStackTraceTest.java

stillalex · 2023-09-11T17:26:49Z

@dsmiley I am attempting to rewrite the test using thew new EmbeddedSolrServerTestRule but I am falling short.
First it seems to shortcut exceptions in the response via checkForExceptions(rsp); method so there is no opportunity for the stacktrace to be populated in the response, what I see now is just the exception being thrown so cannot verify the hideStackTrace flag does anything.
Second it seems to hardcode the response to the binary response (see BinaryResponseWriter.getParsedResponse), so not sure if this might need to change to support other types, for this test it is needed to serialize the error to verify the trace is not there.
please correct if I am missing something obvious here.

mode datapoints (the test and the stacktrace of the exception, left rule init outside for brevity):

  @Test
  public void testHideStackTrace() throws Exception {
    SolrClient client = solrRule.getSolrClient();
    final var req = new QueryRequest(new SolrQuery("qt", "/withError", "q", "*:*" , "wt", "xml"));
    final var rsp = req.process(client); // throws SolrServerException see below
  }

org.apache.solr.client.solrj.SolrServerException: org.apache.solr.client.solrj.SolrServerException: java.lang.RuntimeException: Stack trace should not be populated.
	at __randomizedtesting.SeedInfo.seed([7E5DC4464AD2FF38:1A02C289B04C1233]:0)
	at org.apache.solr.client.solrj.embedded.EmbeddedSolrServer.request(EmbeddedSolrServer.java:277)
	at org.apache.solr.client.solrj.SolrRequest.process(SolrRequest.java:234)
	at org.apache.solr.client.solrj.SolrRequest.process(SolrRequest.java:249)
	at org.apache.solr.servlet.HideStackTraceTest.testHideStackTrace(HideStackTraceTest.java:75)
Caused by: org.apache.solr.client.solrj.SolrServerException: java.lang.RuntimeException: Stack trace should not be populated.
	at org.apache.solr.client.solrj.embedded.EmbeddedSolrServer.checkForExceptions(EmbeddedSolrServer.java:357)
	at org.apache.solr.client.solrj.embedded.EmbeddedSolrServer.request(EmbeddedSolrServer.java:226)
	... 50 more
Caused by: java.lang.RuntimeException: Stack trace should not be populated.
	at org.apache.solr.servlet.HideStackTraceTest$ErrorComponent.process(HideStackTraceTest.java:110)
	at org.apache.solr.handler.component.SearchHandler.handleRequestBody(SearchHandler.java:442)
	at org.apache.solr.handler.RequestHandlerBase.handleRequest(RequestHandlerBase.java:224)
	at org.apache.solr.core.SolrCore.execute(SolrCore.java:2890)
	at org.apache.solr.client.solrj.embedded.EmbeddedSolrServer.request(EmbeddedSolrServer.java:225)
	... 50 more

dsmiley · 2023-09-11T17:43:50Z

Thanks for trying Alex!
I suppose this test is more sensitive to the serialization, in which case you should probably use SolrJettyTestRule.

EmbeddedSolrServer: Ideally, a test (or more likely the test infrastructure behind the scenes) could pick the response format if it wants, say, XML or JSON instead of the parsed NamedList, such as if assertJQ (JSON path custom thing) or assertQ (XPath). The InputStreamResponseParser may be needed, which is a bit of a special thing (see Find-Usages to see for yourself).

stillalex · 2023-09-11T18:36:17Z

@dsmiley I refactored the test as best I could. please take a look and let me know if more cleanup is needed.
I had to rebase+force push to get crave build running again

dsmiley · 2023-09-12T02:37:18Z

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

@@ -301,6 +301,8 @@ && getZkController().getOverseer() != null

  private final AllowListUrlChecker allowListUrlChecker;

+  private final boolean hideStackTrace;


I don't think a simple global boolean needs to be yet another field on CoreContainer. Even the NodeConfig stuff is overkill for what could be a system property.

If we do keep NodeConfig, we could still do away with this field because the NodeConfig stays around as a field on CoreContainer as cfg.

dsmiley · 2023-09-12T02:41:11Z

solr/core/src/java/org/apache/solr/servlet/ResponseUtils.java

@@ -28,6 +28,10 @@
 public class ResponseUtils {
  private ResponseUtils() {}

+  // System property to use if the Solr core does not exist or solr.hideStackTrace is not
+  // configured. (i.e.: a lot of unit test).
+  private static final boolean SYSTEM_HIDE_STACK_TRACES = Boolean.getBoolean("solr.hideStackTrace");


This is all we need for internal config IMO.

dsmiley · 2023-09-12T02:47:40Z

solr/core/src/test/org/apache/solr/servlet/HideStackTraceTest.java

+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.util.EntityUtils;


As a project, we'd like to get away from needless use of Apache HttpClient. We can use JDK HttpClient here.

dsmiley · 2023-09-12T02:55:16Z

It's sad to see so much ceremony in NodeConfig :-( -- an issue for another PR.

Add a property "hideStackTrace" to solr.xml. In NodeConfig, the default value is "false", for back-compatibility. Use the new property in ResponseUtils, to print out, or not, the stack trace. Adapt code that calls ResponseUtils. Add documentation. Add unit test

adapt getTypedErrorInfo to suppress the stack trace

Co-authored-by: Christine Poerschke <[email protected]>

address review comments: - ensure the stack trace is logged while being hidden from the HTTP response - evaluate config setting and system property in a single location Fix unit test

Address review comment: read the system property "solr.hideStackTrace" only once. The system property is used if the Solr core does not exist or solr.hideStackTrace is not configured. (in a lot of unit test). Documentation also mentions that Solr can run without configuring any core (not sure how useful it is then): https://solr.apache.org/guide/solr/latest/configuration-guide/core-discovery.html Pass a Boolean instead of the SolrCore object to methods of ResponseUtils. Search the whole code base for places where "trace" is added, and check for "hideStackTrace" before adding the trace. Refactor the unit test: Using an Http2SolrClient, the test threw the exception that was supposed to demonstrate the fix! Use a CloseableHttpClient, to query Solr from "outside", without SolrJ, and get the plain XMl response.

Code clean-up.

Address review comments: use boolean instead of Boolean. Set the system property as default in NodeConfig. Simplify the initialization of the unit test. Add a logger in HideStackTraceTest, to avoid failures on task "validateSourcePatterns", due to logger name and logger pattern Built and checked locally - gradlew.bat compileJava compileTestJava test - gradlew.bat tidy check -x test

address review comment: fix default hideStackTrace

stillalex · 2023-09-12T20:34:24Z

having to rebase + force commit to get crave build unstuck is becoming a real pain

Added a property "hideStackTrace" to solr.xml or system property 'solr.hideStackTrace' to not include error stack trace in the response. --------- Co-authored-by: Christine Poerschke <[email protected]> Co-authored-by: Alex Deparvu <[email protected]> (cherry picked from commit cf30265)

sonatype-lift bot reviewed May 8, 2023

View reviewed changes

solr/core/src/java/org/apache/solr/servlet/ResponseUtils.java Show resolved Hide resolved

sonatype-lift bot reviewed May 8, 2023

View reviewed changes

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

igiguere changed the title ~~SOLR_14886 : suppress stack traces in query response~~ SOLR-14886 : suppress stack traces in query response May 8, 2023

cpoerschke reviewed May 12, 2023

View reviewed changes

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

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

solr/core/src/java/org/apache/solr/servlet/ResponseUtils.java Show resolved Hide resolved

sonatype-lift bot reviewed May 12, 2023

View reviewed changes

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

stillalex reviewed Aug 16, 2023

View reviewed changes

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

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

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

stillalex reviewed Aug 23, 2023

View reviewed changes

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

stillalex reviewed Aug 23, 2023

View reviewed changes

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

stillalex reviewed Aug 23, 2023

View reviewed changes

solr/core/src/test/org/apache/solr/servlet/HideStackTraceTest.java Outdated Show resolved Hide resolved

stillalex reviewed Aug 23, 2023

View reviewed changes

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

dsmiley reviewed Aug 25, 2023

View reviewed changes

solr/core/src/test/org/apache/solr/servlet/HideStackTraceTest.java Show resolved Hide resolved

stillalex force-pushed the SOLR-14886-Suppress_stack_trace_in_Query_response branch from dc7733e to 80bf80a Compare September 11, 2023 18:31

dsmiley reviewed Sep 12, 2023

View reviewed changes

igiguere and others added 8 commits September 12, 2023 13:32

SOLR-14886 : suppress stack trace in response

8f206b0

adapt getTypedErrorInfo to suppress the stack trace

Update solr/core/src/java/org/apache/solr/core/CoreContainer.java

99ef72e

Co-authored-by: Christine Poerschke <[email protected]>

SOLR-14886 : suppress stack trace in response

4c7e0b6

address review comments: - ensure the stack trace is logged while being hidden from the HTTP response - evaluate config setting and system property in a single location Fix unit test

SOLR-14886 : Suppress stack trace in query response

d7d0b33

Code clean-up.

SOLR-14886: Suppress stack traces in response

abfd756

address review comment: fix default hideStackTrace

stillalex added 5 commits September 12, 2023 13:32

refactored test

240607a

rm conf file, no longer needed

0a57ce3

minor import fix

5eb5a68

added final to solrRule

940d197

removed field from CoreContainer, updated CHANGES.txt file

5b5d907

stillalex force-pushed the SOLR-14886-Suppress_stack_trace_in_Query_response branch from 422925b to 5b5d907 Compare September 12, 2023 20:33

dsmiley approved these changes Sep 12, 2023

View reviewed changes

stillalex merged commit cf30265 into apache:main Sep 12, 2023
2 checks passed

igiguere deleted the SOLR-14886-Suppress_stack_trace_in_Query_response branch September 25, 2023 21:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SOLR-14886 : suppress stack traces in query response #1632

SOLR-14886 : suppress stack traces in query response #1632

igiguere commented May 8, 2023 •

edited

Loading

dsmiley left a comment

stillalex commented Aug 25, 2023

igiguere commented Aug 25, 2023

igiguere commented Aug 25, 2023

stillalex commented Aug 25, 2023

dsmiley left a comment

stillalex commented Sep 11, 2023 •

edited

Loading

dsmiley commented Sep 11, 2023

stillalex commented Sep 11, 2023

dsmiley Sep 12, 2023

dsmiley Sep 12, 2023

dsmiley Sep 12, 2023

dsmiley commented Sep 12, 2023

stillalex commented Sep 12, 2023

		@@ -301,6 +301,8 @@ && getZkController().getOverseer() != null

		private final AllowListUrlChecker allowListUrlChecker;

		private final boolean hideStackTrace;

		import org.apache.http.client.methods.HttpGet;
		import org.apache.http.util.EntityUtils;

SOLR-14886 : suppress stack traces in query response #1632

SOLR-14886 : suppress stack traces in query response #1632

Conversation

igiguere commented May 8, 2023 • edited Loading

Description

Solution

Tests

Checklist

dsmiley left a comment

Choose a reason for hiding this comment

stillalex commented Aug 25, 2023

igiguere commented Aug 25, 2023

igiguere commented Aug 25, 2023

stillalex commented Aug 25, 2023

dsmiley left a comment

Choose a reason for hiding this comment

stillalex commented Sep 11, 2023 • edited Loading

dsmiley commented Sep 11, 2023

stillalex commented Sep 11, 2023

dsmiley Sep 12, 2023

Choose a reason for hiding this comment

dsmiley Sep 12, 2023

Choose a reason for hiding this comment

dsmiley Sep 12, 2023

Choose a reason for hiding this comment

dsmiley commented Sep 12, 2023

stillalex commented Sep 12, 2023

igiguere commented May 8, 2023 •

edited

Loading

stillalex commented Sep 11, 2023 •

edited

Loading