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

All security implementations bypassed if 'x-forwarded-host: strapi' header is set #2969

Open
markjlaney opened this issue Mar 12, 2019 · 5 comments

Comments

Projects
None yet
5 participants
@markjlaney
Copy link

commented Mar 12, 2019

Informations

  • Node.js version: 10.0.0
  • NPM version: 6.0.0
  • Strapi version:
  • Database: Mongodb
  • Operating system: Mojave 10.14.1
  • (Optional) Link to your Project:

What is the current behavior?
If a user passes this header: 'x-forwarded-host: strapi', then ctx.request.admin is set to true. Then, in the CORS implementation, if ctx.request.admin is set to true, then the origin is set to *, completely discarding the cors configuration. Therefore, anyone passing this header can send requests because the origin is set to *. Here are the relevant lines of code:

https://github.com/strapi/strapi/blob/master/packages/strapi/lib/middlewares/index.js (line 13 - set to true if that header is passed).

https://github.com/strapi/strapi/blob/master/packages/strapi/lib/middlewares/cors/index.js (line 22 - if the ctx.request.admin is true, then the origin is set to *)

Steps to reproduce the problem

  1. Enable the cors policy in the correct environment and set origin to whatever domain you want:
"cors": {
    "enabled": true,
    "origin": "https://fun-app.com",
    "expose": [
      "WWW-Authenticate",
      "Server-Authorization"
    ],
    "maxAge": 31536000,
    "credentials": false,
    "methods": [
      "GET",
      "POST",
      "PUT",
      "PATCH",
      "DELETE",
      "HEAD"
    ],
    "headers": [
      "Content-Type",
      "Authorization",
      "X-Frame-Options",
      "Origin"
    ]
  }
  1. Run this curl command:
curl -isk -X OPTIONS \
-H 'x-forwarded-host: strapi' \
-H 'Access-Control-Request-Method: GET' \
-H 'Origin: https://fake.com' \
http://localhost:1337/content-manager/explorer/marketingtemplate/\?source\=content-manager

What is the expected behavior?
The expected behavior is that it will say https://fake.com is not a valid origin. Instead it allows the request through the app.

Suggested solutions

Check that strapi.config.currentEnvironment.security.cors.enabled is enabled before checking the ctx.request.admin.

@lauriejim

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

Hello @markjlaney !

Thank you to report this point.

The cors config have to be apply for API request and not for the admin panel stuff.
Cause we can split the front-end stuff of the admin in an other place and it will not match with the front end application that consume the API.
But the admin API will still in the same server and will be affected by by the cors configuration.

@markjlaney

This comment has been minimized.

Copy link
Author

commented Mar 12, 2019

@lauriejim I'm not quite sure I understand what you're saying, but this is reproducible for React app endpoints and admin server endpoints. Or API endpoints. I just tried it against a content type API and an admin API (admin/currentEnvironment) and I'm able to get the origin * every time.

@markjlaney

This comment has been minimized.

Copy link
Author

commented Mar 12, 2019

Ah ok, @lauriejim and I had a discussion in slack. They use that header to disable the CORS setup if the request is coming from the admin panel. The problem is that anyone can set this header and they can disable the CORS policy.

Couldn't you just include your admin panel host in your origin and this header wouldn't be necessary? Like I said, you could check if the CORS policy is enabled first. I mean most people probably aren't running on port 4000 as well so it's not even guaranteed that the hard-coded port value will work. That check itself probably causes people a lot of pain.

@derrickmehaffy

This comment has been minimized.

Copy link
Contributor

commented Mar 25, 2019

Ah ok, @lauriejim and I had a discussion in slack. They use that header to disable the CORS setup if the request is coming from the admin panel. The problem is that anyone can set this header and they can disable the CORS policy.

Couldn't you just include your admin panel host in your origin and this header wouldn't be necessary? Like I said, you could check if the CORS policy is enabled first. I mean most people probably aren't running on port 4000 as well so it's not even guaranteed that the hard-coded port value will work. That check itself probably causes people a lot of pain.

I agree with all of this @lauriejim @soupette @Aurelsicoko using a header for "security" is super bad and is not at all secure. (not to mention it makes us network engineers life a royal headache when you start scaling).

There needs to be a different way to secure this.

@markjlaney

This comment has been minimized.

Copy link
Author

commented Mar 25, 2019

@derrickmehaffy @lauriejim @soupette @Aurelsicoko what's worse is that from what I can tell the CSRF implementation doesn't work independent of the X-FORWARDED header. I've tried removing that X-FORWARDED header myself as a workaround, but the admin app does not send a CSRF token like it should, so I have to come up with my own CSRF solution. Part of this fix should be including the x-csrf-token header in all requests like the koa-lusca library expects.

@markjlaney markjlaney changed the title CORS set to * if 'x-forwarded-host: strapi' header is set All security implementations bypassed if 'x-forwarded-host: strapi' header is set Mar 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.