Skip to content
This repository has been archived by the owner on Jul 14, 2023. It is now read-only.

Commit

Permalink
Remove XSS opportunity
Browse files Browse the repository at this point in the history
  • Loading branch information
@rohitdatta
rohitdatta committed Jan 24, 2018
1 parent e89c30c commit 5f18eea
Showing 1 changed file with 0 additions and 3 deletions.
3 changes: 0 additions & 3 deletions formspree/templates/forms/thanks.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,6 @@
<div class="container">
<div class="col-1-1">
<h1>Form submitted successfully</h1>
{% if request.args.get('next') %}
<a href="{{ request.args.get('next') }}" class="button" style="margin-top: 1em;">Return to original site</a>
{% endif %}
</div>
</div>
</div>
Expand Down

8 comments on commit 5f18eea

@colevscode
Copy link
Member

Choose a reason for hiding this comment

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

Hm, think we should revisit this. The return button was a nice feature. Maybe there's a way it can be done that avoids the vulnerability

@rohitdatta
Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, I want to bring it back especially since a few customers have asked for it. This was a temporary fix since I didn't see a way to quickly change how it works. Preferably, we can just set up the button on template render, but that would require a change in how the URL is passed.

@fiatjaf
Copy link
Contributor

Choose a reason for hiding this comment

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

I admint I'm not well-versed in these basic web security stuff, but i don't understand why this is a XSS opportunity. Flask escapes these strings automatically, right? What could an attacker do here?

@rohitdatta
Copy link
Member Author

Choose a reason for hiding this comment

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

If you take a look at ticket 932, it's explained further

@fiatjaf
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Open Redirect - An Attacker is able to redirect the user to any domain he wants.
  2. Cross-Site Scripting - An attack can elevate the open redirect vulnerability to XSS By entering the payload "javascript:alert(document.domain)" and steal the user login session.

I don't see how these can be a problem in this case. The "attacker" in this case is the owner of the website which is hosting a form. The "victim" in this case is someone who's already visiting the attacker's website and spontaneously submitting a form there. What could the attacker do that he couldn't do in his own site?

@kobs0N
Copy link

@kobs0N kobs0N commented on 5f18eea May 19, 2020

Choose a reason for hiding this comment

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

  1. Open Redirect - An Attacker is able to redirect the user to any domain he wants.
  2. Cross-Site Scripting - An attack can elevate the open redirect vulnerability to XSS By entering the payload "javascript:alert(document.domain)" and steal the user login session.

I don't see how these can be a problem in this case. The "attacker" in this case is the owner of the website which is hosting a form. The "victim" in this case is someone who's already visiting the attacker's website and spontaneously submitting a form there. What could the attacker do that he couldn't do in his own site?

Hi @fiatjaf ,
Unfortunately, you are wrong and It is ("was") a big problem and I will explain.
The "attacker" can tamper with the "_next" parameter and add malicious js code which will lead to executing the code in the user browser who visited the link and redirects him to a phishing website.
You may want to read on the subject:
https://owasp.org/www-community/attacks/xss/
https://cwe.mitre.org/data/definitions/601.html

Stay Safe.

@colevscode
Copy link
Member

@colevscode colevscode commented on 5f18eea May 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

There are two vulnerability links you reference. For XSS, the attacker could inject a script on a website containing a formspree form to change the value of the input[name="_next"] element. However in that case, the attacker is able to change more than just the input, they could override the form's onSubmit handler and redirect the form entirely. Also they could inject their own form with an action="attack.com". The solution is to protect against XSS, since once a script is running on the page, it can manipulate the DOM in many ways. I don't think having a hidden input which specifies the redirect following form submission introduces a new vector for XSS.

The second scenario is "open redirect" involving urls of the form https://trusted.site?next=https://redirect.link. An attacker could use a link like https://formspree.io/thanks?next=https://attack.com in a phishing attack. However this is not an open redirect, because the link doesn't actually result in a redirect. Instead, such a link would render a "thanks" page with a "return to original page" link that the user must click on. We should still remove this vector if possible.

Do you agree with my assessment? Thanks for bringing it back to our attention.

@kobs0N
Copy link

@kobs0N kobs0N commented on 5f18eea May 20, 2020

Choose a reason for hiding this comment

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

Hi @colevscode,

Very nice, really liked the XSS overview.
But regarding the open redirect, you actually described the vulnerability. In the end, the user will be redirected.
Read here:
https://www.trustwave.com/en-us/resources/blogs/spiderlabs-blog/understanding-and-discovering-open-redirect-vulnerabilities/

Please sign in to comment.