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

CSRF in post requests #1

Open
cc-d opened this issue Sep 25, 2019 · 8 comments
Open

CSRF in post requests #1

cc-d opened this issue Sep 25, 2019 · 8 comments

Comments

@cc-d
Copy link
Owner

cc-d commented Sep 25, 2019

There are currently many possible CSRF vulns involving post requests. Since none of them (as far as I know anyway) involve get requests, it's not that pressing of an issue currently, but eventually I need to install a CSRF library and add unique tokens to every form.

In the meantime, mods/admins should be careful.

@cc-d cc-d added this to the CSRF in post requests milestone Sep 25, 2019
@stets
Copy link
Collaborator

stets commented Oct 2, 2019

involve get requests, it's not that pressing of an issue currently, but eventually I need to install a CSRF library and add unique tokens to every form.

I don't think that matters -- your stuff is open-source so I could find the admin routes and craft a link to send you that hits one of the admin routes and blow away the whole db.

I'd read up a little more, this one is a big deal
https://www.owasp.org/index.php/Testing_for_CSRF_(OTG-SESS-005)

Protection can be done via
https://flask-wtf.readthedocs.io/en/stable/csrf.html

@cc-d
Copy link
Owner Author

cc-d commented Oct 2, 2019

Right, for this to be an issue with how things are constructed currently, somebody would need to be able to execute arbitrary javascript on the site. In which case, an on-page CSRF token will not provide any protection anyway.

As far as I know anyway, there are no XSS vulns currently, but a good place to look for them is in any template that renders as {{ VARIABLE|safe }}. Since markup is supported, I was forced to allow users to submit some forms of html tags, even though I'd really rather not lol.

Every variable that gets rendered as |safe should have pseudo_markup(TEXT) called before it is passed to jinja2. This function first converts text into pure markup, and then runs bleach(TEXT) with the default configuration.

If there were an xss vuln somewhere, this is where it would be. Forgetting to call pseudo_markup() on text before rendering as safe is an understandable mistake, thankfully there hasn't been any issues related to this so far, but I could see it being a problem in the future as it's somewhat easy to overlook.

@cc-d
Copy link
Owner Author

cc-d commented Oct 2, 2019

A link alone to a route would not be sufficient to cause any real damage currently, which is why I wasn't overly concerned.

@cc-d
Copy link
Owner Author

cc-d commented Oct 2, 2019

Even votes are behind post requests. There was a pretty hilarious HN post several years ago that exploited the fact that HN votes were submitted with get requests by users, I believe it is still the highest voted post of all time. "this post upvotes itself" lol

if there is a route that does anything significant based on gets, then that's a serious issue but as far as i know there aren't any

@cc-d
Copy link
Owner Author

cc-d commented Oct 2, 2019

That being said, I'll re-enable some headers which were commented out that will marginally improve site security, since there isn't really a reason to not do so.

@cc-d
Copy link
Owner Author

cc-d commented Oct 2, 2019

cde64e3

Not foolproof by any means, but this shouldn't really hurt anything so ¯\(ツ)

@stets
Copy link
Collaborator

stets commented Oct 2, 2019

Right, for this to be an issue with how things are constructed currently, somebody would need to be able to execute arbitrary javascript on the site

Is that true? I don't think you need javascript for a CSRF attack

@cc-d
Copy link
Owner Author

cc-d commented Oct 3, 2019

For post requests yeah, assuming you don't trick the user into visiting some phishing page which intercepts, and then relays, the traffic.

If you were to enable CORS, completely disregard the same-origin policy, etc it can technically be done, but you would almost have to be intentionally insecure to setup a configuration which allows such a thing.

cc-d pushed a commit that referenced this issue Oct 25, 2019
cc-d pushed a commit that referenced this issue Nov 17, 2019
Merge pull request #110 from docyx/master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants