Skip to content
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][BACKPORT-2.0] Strengthen Spark to prevent XSS vulnerabilities #19538

Closed
wants to merge 4 commits into from

Conversation

ambauma
Copy link

@ambauma ambauma commented Oct 19, 2017

What changes were proposed in this pull request?

This is the fix for the master branch applied to the 2.0 branch. My (unnamed) company will be using Spark 1.6 probably for another year. We have been blocked from having Spark 1.6 on our workstations until CVE-2017-7678 is patched, which SPARK-20393 does. I was told I need to patch branch 2.0 before branch 1.6 could be patched.

How was this patch tested?

The patch came with unit tests. The test build passed. Manual testing on one of the effected screens showed the newline character removed. Screen display was the same regardless (html ignores newline characters).
screenshot from 2017-10-19 12-54-01

n-marion and others added 2 commits October 18, 2017 19:33
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.
@SparkQA
Copy link

SparkQA commented Oct 19, 2017

Test build #3954 has finished for PR 19538 at commit 3e01302.

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

@SparkQA
Copy link

SparkQA commented Oct 20, 2017

Test build #3959 has finished for PR 19538 at commit 30d0514.

  • This patch fails SparkR unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@felixcheung
Copy link
Member

ignore SparkR test failure for now, we are looking into it.

@felixcheung
Copy link
Member

could you update the PR title to say [BACKPORT-2.0] instead of [2.0]. also please type to PR # for the earlier commit to link them here.

you mention there is a discussion, could you link them here. are you looking for an official release for 1.6.x?

@felixcheung
Copy link
Member

link to 1.6 PR #19528

@ambauma ambauma changed the title [SPARK-20393][WEBU UI][2.0] Strengthen Spark to prevent XSS vulnerabilities [SPARK-20393][WEBU UI][BACKPORT-2.0] Strengthen Spark to prevent XSS vulnerabilities Oct 20, 2017
@ambauma
Copy link
Author

ambauma commented Oct 20, 2017

I'm not looking for an official release. My goal is to get the fix into the official branch 1.6 to reduce the number of forks necessary and so that if CVE-2018-XXXX comes and I've moved on my replacement doesn't have to apply this plus that.

* Return the correct Href after checking if master is running in the
* reverse proxy mode or not.
*/
def makeHref(proxy: Boolean, id: String, origHref: String): String = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not in the 2.0 PR?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this method came with the original patch. I don't see anything calling it. I will remove it.

@felixcheung
Copy link
Member

Jenkins, retest this please

@SparkQA
Copy link

SparkQA commented Oct 23, 2017

Test build #82989 has finished for PR 19538 at commit a599d91.

  • This patch fails SparkR unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@jiangxb1987
Copy link
Contributor

retest this please

@HyukjinKwon
Copy link
Member

ok to test

@SparkQA
Copy link

SparkQA commented Jul 16, 2018

Test build #93054 has finished for PR 19538 at commit a599d91.

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

@dongjoon-hyun
Copy link
Member

@ambauma Unfortunately, it seems to be too old and the PR on 1.6 also is closed. Can we close this, too?

My goal is to get the fix into the official branch 1.6 to reduce the number of forks necessary and so that if CVE-2018-XXXX comes and I've moved on my replacement doesn't have to apply this plus that.

@ambauma
Copy link
Author

ambauma commented Sep 13, 2018 via email

@srowen
Copy link
Member

srowen commented Sep 15, 2018

@ambauma could you close it? we can't, directly

@ambauma ambauma closed this Sep 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
8 participants