search

cc-d / ieddit

ieddit.com - A minimalist reddit-like with anonymous posting.
https://ieddit.com
Other
151 stars 20 forks source link

CSRF in post requests #1

Open cc-d opened 5 years ago

cc-d commented 5 years ago

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.

stets commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

https://github.com/cc-d/ieddit/commit/cde64e3f0f9c7f3d9f8d86b4b3066a1b7159ba84

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

stets commented 5 years ago

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 commented 5 years ago

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.