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

Video subs challenge #891

Merged
merged 21 commits into from
Apr 2, 2019
Merged

Conversation

supra08
Copy link
Contributor

@supra08 supra08 commented Mar 23, 2019

In this PR I've added the video subtitles challenge. I had to use jade as the template because the real scenario is understood by seeing the <script> of the rendered html. The basic flow is as follows:

  • The attacker needs to find the /promotion route. (Possibility of implementing the easter egg here!)
  • Once the attacker is in the video page, he observes that there is only a upload subtitle form to perform the xss.
  • He uploads an infected subtitle file containing the prescribed payload <script>alert('xss')</script> to successfully solve the challenge.

juiceShop20190323_133021

Now, here the catch is, simply entering <script>alert('xss')</script> won't work here. The subs are rendered in a special <script type=text/vtt> tag. This practice is found in a few websites to evade cross origin policies. An ideal infected subs file would contain:

  • A closing </script> tag to escape the original <script type=text/vtt>.
  • Then the attack <script>alert('xss')</script>

So the attacker needs to inspect the js of the page first to observe what's going on.

TODO:

  • Convert Upload Subs to Upload Correct Subs and a fake review system to finally upload them.
  • e2e tests and units tests for challenge

Your views on this, @bkimminich , @J12934 , @CaptainFreak

@bkimminich
Copy link
Member

How about we don't add an upload button for the subtitles, but instead put them somewhere on the backend side where they can be overwritten with the same attack used for Arbitrary File Write?

@supra08
Copy link
Contributor Author

supra08 commented Mar 23, 2019

Yep that I believe would be a better idea. Uploading of subs looks a bit unrealistic too 😅 .

@supra08
Copy link
Contributor Author

supra08 commented Mar 24, 2019

@bkimminich, I've made the subs to load from a file in ftp/assets. Since this file will be visible through the localhost:3000/ftp route, the user needs to overwrite the file using the zipper vulnerability. Upon visiting the video page(now with infected subs), the challenge will be solved. I'll try to fix the failing tests asap.

@bkimminich
Copy link
Member

One of the failing tests was on me when I did some refactoring, should be fixed with next build. Can you put the subs into some folder that is not in /ftp? As the Aribtrary File Write challenge is already about overwriting a file in there, it would be nice to show with this that even a seemingly less exposed file can be overwritten with ZipSlip.

@supra08
Copy link
Contributor Author

supra08 commented Mar 26, 2019

Now shifted the subs file to the dist directory in the assets folder, because normally a website would maintain it this way. But now I guess, the user must be given more hints for locating the subs file as it has now become very tough to even guess the location. Sorry for the delay, got some chromedriver related problems that took time.
@bkimminich ^

const challenges = require('../data/datacache').challenges
const utils = require('../lib/utils')

const themes = {
Copy link
Member

Choose a reason for hiding this comment

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

This should be pulled out of userProfile.js and be reused by both Jade pages in order to avoid code duplication.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So should I put this into the data directory in a new file themes.js? @bkimminich

Copy link
Member

Choose a reason for hiding this comment

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

I'd probably put it into /views because it's only for the Jade screens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright 👍

@bkimminich
Copy link
Member

@supra08, are any further changes coming in for this PR? Pulling out the theme for all Jade screens is still open. @J12934, do you have any additional remarks for this PR?

@supra08
Copy link
Contributor Author

supra08 commented Apr 2, 2019

I'm really sorry for the delay. This just slipped out. I've made the changes. Please do tell if more modifications are requires. @bkimminich, @J12934

@bkimminich bkimminich merged commit 547a5e1 into juice-shop:develop Apr 2, 2019
crispy-peppers pushed a commit to crispy-peppers/juice-shop that referenced this pull request Nov 12, 2019
…el (juice-shop#891)

* Closes juice-shop#876
* Fixes overflowing admin panel content by adding the `.text-break` CSS class.
    * This is .text-break cloned from Bootstrap 4.3 with a fix for browsers not supporting break-word. It will be removed from the main CTFd classes when Bootstrap is upgraded internally.
@lock
Copy link

lock bot commented Apr 2, 2020

This thread has been automatically locked because it has not had recent activity after it was closed. 🔒 Please open a new issue for regressions or related bugs.

@lock lock bot locked and limited conversation to collaborators Apr 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants