-
Notifications
You must be signed in to change notification settings - Fork 28.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-20393][Webu UI] Strengthen Spark to prevent XSS vulnerabilities #17686
Changes from 4 commits
6bdc629
c812f2e
06a6791
d50c0c2
19d6f86
ff7be45
18dbd6f
39824a8
ef1cc25
8ed9c76
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,6 +27,7 @@ import scala.xml.transform.{RewriteRule, RuleTransformer} | |
|
||
import org.apache.spark.internal.Logging | ||
import org.apache.spark.ui.scope.RDDOperationGraph | ||
import org.apache.commons.lang3.StringEscapeUtils | ||
|
||
/** Utility functions for generating XML pages with spark content. */ | ||
private[spark] object UIUtils extends Logging { | ||
|
@@ -527,4 +528,25 @@ private[spark] object UIUtils extends Logging { | |
origHref | ||
} | ||
} | ||
|
||
/** | ||
* Remove suspicious characters of user input to prevent Cross-Site scripting (XSS) attacks | ||
* | ||
* For more information about XSS testing: | ||
* 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 = { | ||
var requestParameterStripped = requestParameter | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the body is simpler as
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Followed your suggestion. |
||
if (requestParameterStripped != null) { | ||
// Avoid null characters or single quote | ||
requestParameterStripped = requestParameterStripped.replaceAll("(\r\n|\n|\r|%0D%0A|%0A|%0D|'|%27)", "") | ||
requestParameterStripped = StringEscapeUtils.escapeHtml4(requestParameterStripped) | ||
} | ||
requestParameterStripped | ||
} | ||
|
||
def stripXSSArray(requestParameter: Array[String]): Array[String] = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm on the fence about whether it's worth including this as you can just write There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
requestParameter.map(stripXSS) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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(_)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: omit There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed. |
||
val parameterOtherTable = allParameters.filterNot(_._1.startsWith(jobTag)) | ||
.map(para => para._1 + "=" + para._2(0)) | ||
|
||
val someJobHasJobGroup = jobs.exists(_.jobGroup.isDefined) | ||
val jobIdTitle = if (someJobHasJobGroup) "Job Id (Job Group)" else "Job Id" | ||
|
||
val parameterJobPage = request.getParameter(jobTag + ".page") | ||
val parameterJobSortColumn = request.getParameter(jobTag + ".sort") | ||
val parameterJobSortDesc = request.getParameter(jobTag + ".desc") | ||
val parameterJobPageSize = request.getParameter(jobTag + ".pageSize") | ||
val parameterJobPrevPageSize = request.getParameter(jobTag + ".prevPageSize") | ||
// stripXSS is called first to remove suspicious characters used in XSS attacks | ||
val parameterJobPage = UIUtils.stripXSS(request.getParameter(jobTag + ".page")) | ||
val parameterJobSortColumn = UIUtils.stripXSS(request.getParameter(jobTag + ".sort")) | ||
val parameterJobSortDesc = UIUtils.stripXSS(request.getParameter(jobTag + ".desc")) | ||
val parameterJobPageSize = UIUtils.stripXSS(request.getParameter(jobTag + ".pageSize")) | ||
val parameterJobPrevPageSize = UIUtils.stripXSS(request.getParameter(jobTag + ".prevPageSize")) | ||
|
||
val jobPage = Option(parameterJobPage).map(_.toInt).getOrElse(1) | ||
val jobSortColumn = Option(parameterJobSortColumn).map { sortColumn => | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you would, add a couple brief tests of this to
UIUtilsSuite
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll work on some.