[SPARK-20393][Webu UI] Strengthen Spark to prevent XSS vulnerabilities #17686

n-marion · 2017-04-19T14:33:14Z

What changes were proposed in this pull request?

Add stripXSS and stripXSSMap to Spark Core's UIUtils. Calling these functions at any point that getParameter is called against a HttpServletRequest.

How was this patch tested?

Unit tests, IBM Security AppScan Standard no longer showing vulnerabilities, manual verification of WebUI pages.

srowen

This is probably fine, but, is this all the calls to request.getParameter? and how real is the threat vs theoretical? anyone who can access Spark can already run arbitrary code on the cluster.

srowen · 2017-04-19T14:54:11Z

core/src/main/scala/org/apache/spark/ui/UIUtils.scala

+    strippedXSSUrl
+  }
+
+  def stripXSSMap(url: Array[String]): Array[String] = {


Not sure what this method is trying to do but it just returns its argument. Do you just mean url.map(stripXSS)? why Map, why url as names?

I appreciate the feedback. The method returned the array of strings stripped of possible XSS issues. I have used your recommendation in the next commit. Map was changed to Array. url is now requestParameter.

srowen · 2017-04-19T14:54:33Z

core/src/main/scala/org/apache/spark/ui/jobs/AllJobsPage.scala

@@ -220,18 +220,20 @@ private[ui] class AllJobsPage(parent: JobsTab) extends WebUIPage("") {
      jobTag: String,
      jobs: Seq[JobUIData],
      killEnabled: Boolean): Seq[Node] = {
-    val allParameters = request.getParameterMap.asScala.toMap
+    //stripXSS is called to remove suspicious characters used in XSS attacks
+    val allParameters = request.getParameterMap.asScala.toMap.mapValues(UIUtils.stripXSSMap(_))


(_) is redundant here

… scala style.

srowen · 2017-04-21T13:15:48Z

core/src/main/scala/org/apache/spark/ui/jobs/AllJobsPage.scala

@@ -220,18 +220,20 @@ private[ui] class AllJobsPage(parent: JobsTab) extends WebUIPage("") {
      jobTag: String,
      jobs: Seq[JobUIData],
      killEnabled: Boolean): Seq[Node] = {
-    val allParameters = request.getParameterMap.asScala.toMap
+    // stripXSS is called to remove suspicious characters used in XSS attacks
+    val allParameters = request.getParameterMap.asScala.toMap.mapValues(UIUtils.stripXSSArray(_))


Nit: omit (_)

srowen · 2017-04-21T13:18:04Z

core/src/main/scala/org/apache/spark/ui/UIUtils.scala

+   * https://www.owasp.org/index.php/Testing_for_Reflected_Cross_site_scripting_(OTG-INPVAL-001)
+   */
+  def stripXSS(requestParameter: String): String = {
+    var requestParameterStripped = requestParameter


I think the body is simpler as

if (requestParameter == null) { null } else { StringEscapeUtils.escapeHtml4( requestParameter.replaceAll("(\r\n|\n|\r|%0D%0A|%0A|%0D|'|%27)", "")) }

I'm not sure if it will matter for performance but you could pull out the regex as a saved instance variable instead of compiling it each time.

Followed your suggestion.

srowen

These are all the calls to request.getParameter or similar methods right?

srowen · 2017-04-25T08:26:41Z

core/src/main/scala/org/apache/spark/ui/UIUtils.scala


 /** Utility functions for generating XML pages with spark content. */
 private[spark] object UIUtils extends Logging {
  val TABLE_CLASS_NOT_STRIPED = "table table-bordered table-condensed"
  val TABLE_CLASS_STRIPED = TABLE_CLASS_NOT_STRIPED + " table-striped"
  val TABLE_CLASS_STRIPED_SORTABLE = TABLE_CLASS_STRIPED + " sortable"

+  val NEWLINE_AND_SINGLE_QUOTE_REGEX = "(\r\n|\n|\r|%0D%0A|%0A|%0D|'|%27)"


This is just a string constant rather than regex (with "...".r). If you're going to do it you can make this faster by reusing the Regex object. (Can these members be private while you're at it?)

I intended to make it a regex object; but I did miss the .r; By making these members private are you referring to all of the: TABLE_*?

replaceAll requires (String, String) so I got a compile error with the regex object.

srowen · 2017-04-25T08:27:37Z

core/src/main/scala/org/apache/spark/ui/UIUtils.scala

+        requestParameter.replaceAll(NEWLINE_AND_SINGLE_QUOTE_REGEX, ""))
+  }
+
+  def stripXSSArray(requestParameter: Array[String]): Array[String] = {


I'm on the fence about whether it's worth including this as you can just write _.map(stripXSS) in the two places it's used.

I'll check this again; I had a problem with _.map(stripXSS) compiling; but maybe I did something wrong with it.

And in regards to all the calls; I checked for any type of getParameter call; and also looked for HttpServletRequest with other types of calls. Doesn't appear to be any I missed.

srowen · 2017-04-25T08:28:15Z

core/src/main/scala/org/apache/spark/ui/UIUtils.scala

+   * https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet and
+   * https://www.owasp.org/index.php/Testing_for_Reflected_Cross_site_scripting_(OTG-INPVAL-001)
+   */
+  def stripXSS(requestParameter: String): String = {


If you would, add a couple brief tests of this to UIUtilsSuite

I'll work on some.

srowen · 2017-05-02T08:56:17Z

core/src/main/scala/org/apache/spark/ui/UIUtils.scala


 /** Utility functions for generating XML pages with spark content. */
 private[spark] object UIUtils extends Logging {
  val TABLE_CLASS_NOT_STRIPED = "table table-bordered table-condensed"
  val TABLE_CLASS_STRIPED = TABLE_CLASS_NOT_STRIPED + " table-striped"
  val TABLE_CLASS_STRIPED_SORTABLE = TABLE_CLASS_STRIPED + " sortable"

+  private val NEWLINE_AND_SINGLE_QUOTE_REGEX = "(\r\n|\n|\r|%0D%0A|%0A|%0D|'|%27)"


I still think it's worth making this a regex object if you're going to bother, but otherwise LGTM

Will attempt. Thanks.

Regex created. Changed replaceAll string method to replaceAllIn regex method. Added new test case for new line in parameter.

… for newline in parameter.

SparkQA · 2017-05-04T14:01:21Z

Test build #3688 has finished for PR 17686 at commit 18dbd6f.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-05-06T10:08:41Z

Test build #3694 has finished for PR 17686 at commit 39824a8.

This patch fails Scala style tests.
This patch does not merge cleanly.
This patch adds no public classes.

srowen · 2017-05-08T09:02:42Z

@n-marion looks like still some style issues from the checker

n-marion · 2017-05-08T13:06:51Z

Missed the import organization. Just ran it locally. Didn't hit same problem.

SparkQA · 2017-05-09T08:57:57Z

Test build #3702 has finished for PR 17686 at commit ef1cc25.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

n-marion · 2017-05-09T11:41:27Z

Adjusted test-case to shorten string. Rerunning to verify passing as well as scalastyle pass for tests.

SparkQA · 2017-05-09T16:00:03Z

Test build #3704 has finished for PR 17686 at commit 8ed9c76.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

srowen · 2017-05-10T10:00:08Z

Merged to master/2.2/2.1

## What changes were proposed in this pull request? Add stripXSS and stripXSSMap to Spark Core's UIUtils. Calling these functions at any point that getParameter is called against a HttpServletRequest. ## How was this patch tested? Unit tests, IBM Security AppScan Standard no longer showing vulnerabilities, manual verification of WebUI pages. Author: NICHOLAS T. MARION <nmarion@us.ibm.com> Closes apache#17686 from n-marion/xss-fix.

## What changes were proposed in this pull request? Add stripXSS and stripXSSMap to Spark Core's UIUtils. Calling these functions at any point that getParameter is called against a HttpServletRequest. ## How was this patch tested? Unit tests, IBM Security AppScan Standard no longer showing vulnerabilities, manual verification of WebUI pages. Author: NICHOLAS T. MARION <nmarion@us.ibm.com> Closes #17686 from n-marion/xss-fix. (cherry picked from commit b512233) Signed-off-by: Sean Owen <sowen@cloudera.com>

Add stripXSS and stripXSSMap to Spark Core's UIUtils. Calling these functions at any point that getParameter is called against a HttpServletRequest. Unit tests, IBM Security AppScan Standard no longer showing vulnerabilities, manual verification of WebUI pages. Author: NICHOLAS T. MARION <nmarion@us.ibm.com> Closes #17686 from n-marion/xss-fix. (cherry picked from commit b512233) Signed-off-by: Sean Owen <sowen@cloudera.com>

Add stripXSS and stripXSSMap to Spark Core's UIUtils. Calling these functions at any point that getParameter is called against a HttpServletRequest. Unit tests, IBM Security AppScan Standard no longer showing vulnerabilities, manual verification of WebUI pages. Author: NICHOLAS T. MARION <nmarion@us.ibm.com> Closes apache#17686 from n-marion/xss-fix.

n-marion added 3 commits April 19, 2017 10:25

UIUtils.stripXSS added for each page calling request.getParameter.

6bdc629

Perform stripXSS on creation of allParameters mapping

c812f2e

getParameterMap returns Array[String], created new function to handle

06a6791

srowen requested changes Apr 19, 2017

View reviewed changes

Clean up method. Change variable names. Add space in comment to match…

d50c0c2

… scala style.

srowen requested changes Apr 21, 2017

View reviewed changes

Cleanup stripXSS and remove redundant (_)

19d6f86

srowen requested changes Apr 25, 2017

View reviewed changes

Remove additional array function. Add 4 tests to UIUtilsSuite.

ff7be45

srowen reviewed May 2, 2017

View reviewed changes

Change to regex, add case-insensitivity to regex, create new testcase…

18dbd6f

… for newline in parameter.

Fix Scalastyle errors.

39824a8

srowen approved these changes May 6, 2017

View reviewed changes

ScalaStyle fix for imports on UIUtils.

ef1cc25

Fix scalastyle for new tests.

8ed9c76

asfgit closed this in b512233 May 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-20393][Webu UI] Strengthen Spark to prevent XSS vulnerabilities #17686

[SPARK-20393][Webu UI] Strengthen Spark to prevent XSS vulnerabilities #17686

n-marion commented Apr 19, 2017

srowen left a comment

srowen Apr 19, 2017

n-marion Apr 21, 2017

srowen Apr 19, 2017

srowen Apr 21, 2017

n-marion Apr 24, 2017

srowen Apr 21, 2017

n-marion Apr 24, 2017

srowen left a comment

srowen Apr 25, 2017

n-marion Apr 25, 2017

n-marion Apr 25, 2017

srowen Apr 25, 2017

n-marion Apr 25, 2017

srowen Apr 25, 2017

n-marion Apr 25, 2017

srowen May 2, 2017

n-marion May 3, 2017

n-marion May 4, 2017

SparkQA commented May 4, 2017

SparkQA commented May 6, 2017

srowen commented May 8, 2017

n-marion commented May 8, 2017

SparkQA commented May 9, 2017

n-marion commented May 9, 2017 •

edited

Loading

SparkQA commented May 9, 2017

srowen commented May 10, 2017 •

edited

Loading

[SPARK-20393][Webu UI] Strengthen Spark to prevent XSS vulnerabilities #17686

[SPARK-20393][Webu UI] Strengthen Spark to prevent XSS vulnerabilities #17686

Conversation

n-marion commented Apr 19, 2017

What changes were proposed in this pull request?

How was this patch tested?

srowen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srowen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented May 4, 2017

SparkQA commented May 6, 2017

srowen commented May 8, 2017

n-marion commented May 8, 2017

SparkQA commented May 9, 2017

n-marion commented May 9, 2017 • edited Loading

SparkQA commented May 9, 2017

srowen commented May 10, 2017 • edited Loading

n-marion commented May 9, 2017 •

edited

Loading

srowen commented May 10, 2017 •

edited

Loading