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

proposal: add first-class Cross Site Request Forgery protection #42168

Closed
empijei opened this issue Oct 23, 2020 · 7 comments
Closed

proposal: add first-class Cross Site Request Forgery protection #42168

empijei opened this issue Oct 23, 2020 · 7 comments
Labels
Projects
Milestone

Comments

@empijei
Copy link
Contributor

@empijei empijei commented Oct 23, 2020

Preface

While for other web vulnerabilities the go stdlib provide some quite hardened packages (html/template with autoescaping, net/http with a decent posture against smuggling, tls being easily enforced and older versions not used etc.) there is a lack of first-class support for CSRF protection, and CSRF is the second most prominent vulnerability in modern web applications.

The only bit of library that offers something related to it is x/net/xsrftoken, which is not well known, it is not advertised and it is in the "x" package.

The ecosystem has a proliferation of external packages that provide some form of protection against this vulnerability, so it looks like the current Go stance on this is that if users want to build a secure web application they have to rely on external libraries and frameworks (thus accepting a whole new kind of issues with supply chain attacks) or they have to understand how to build their own protections.

Proposal

I would like to propose to add a first-class supported XSRF protection mechanism in the standard library to address this issue.

Stretch option:

Since it feels to me like CSRF is not the only issue we might have to take care of, I would deem it appropriate to add a security sub-package to net/http (naming and location is open to discussion) to use to collect middleware and other utilities (e.g. creating servers with safe defaults).
Go as it is right now almost delivers the full toolkit needed to build secure web applications, but it definitely has what is needed to build a web application, which constitutes a quite scary scenario.

@mvdan
Copy link
Member

@mvdan mvdan commented Oct 25, 2020

I would like to propose to add a first-class supported XSRF protection mechanism in the standard library to address this issue.

I think the general answer to "should the standard library be secure by default?" is "yes", but I also don't understand what particular changes you're suggesting :)

Could you outline what changes you're thinking of? I'm ignoring the "stretch option" section for now, because I want to understand the core proposal first.

@empijei
Copy link
Contributor Author

@empijei empijei commented Oct 26, 2020

Some options:

  1. Provide a wrapper/decorator for handlers or Muxes that implements an anti-csrf middleware in the http package
  2. Provide the wrapper above in a separate package, but reference it from the doc in http or find a way to suggest its adoption
  3. Currently http.Request exposes a forest of ways to get form parameters, all types are strings and internal cross-calls are quite confusing. We could add one more way to access form parameters that allows to protect against CSRF and addresses other issues with form parsing, typing and content-type filtering.
  4. Fix the xsrftoken package and link it from the http package
  5. Not really a solution but a decent starting point: document very clearly on the cookie type that people should go and look for anti-CSRF protection if they use cookies for auth.
  6. Change the Cookie type to have a SameSite attribute by default (this has breakage potential and is very risky)
  7. Make the http package suggest protections via logs. This could be on by default (but doesn't have to) and could be toggled with a flag/config. Detecting handlers that need protection can be done by inspecting Fetch Metadata headers to find when handlers are supposed to be only used same-site. We could use heuristics to check if a handler has some sort of CSRF protection but the downside is that we would not detect some of them. This is very invasive if we consider it would log potential security issues to users logs but it would probably detect most vulnerabilities already out there and help people fix them.

I'm also open to other suggestions, these are options that I've been pondering for a while and all have one main issue: people would need to know about them and adopt them.

Protect current users without breakages or behavior changes would be hard and I can't find a reliable way to bring people on board with a new, safe, API without deprecating the old ones or offering a better set of features (e.g. point 3 above, if that offers something better than the other form fields/methods it could help drive adoption).

What do you think?

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Oct 28, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Nov 4, 2020

Doing something in http directly seems like a very significant step.
It would probably be better to focus on "Fix the xsrftoken package and link it from the http package."
By "link it" I assume you mean mention it in the docs.
In general net/http is pretty low-level. Providing a very opinionated approach to XSRF in net/http seems out of place, but doing something great in xsrftoken seems like a good idea.

Over on #42166, what did you have in mind as a new, less error-prone API?

@rsc rsc moved this from Incoming to Active in Proposals Nov 4, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Nov 18, 2020

Ping @empijei. We really need a clearer proposal to evaluate.

@empijei
Copy link
Contributor Author

@empijei empijei commented Nov 23, 2020

I'd be ok with fixing xsrftoken and linking it from the http package docs, it seems like the less invasive solution.

My only concern is that it would not be picked up by many users.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 23, 2020

@rsc
Copy link
Contributor

@rsc rsc commented Dec 2, 2020

Moving this to #42166 then. Thanks.

@rsc rsc closed this Dec 2, 2020
@rsc rsc moved this from Active to Declined in Proposals Dec 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Declined
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants