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

fix: plugins cors allow_origins is "**" work does not meet expectations #11498

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kys1230
Copy link

@kys1230 kys1230 commented Aug 13, 2024

Description

Resolved an issue where the cross-origin plugin did not function correctly when the plugin configuration allow_origins = "**" and an exceptional request header Origin contained a comma (",").

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

@dosubot dosubot bot added size:XS This PR changes 0-9 lines, ignoring generated files. bug Something isn't working labels Aug 13, 2024
@kys1230
Copy link
Author

kys1230 commented Aug 13, 2024

Please pay attention to this issue, as it can lead to a malicious request that affects the cross-origin response of other normal requests.

Copy link
Contributor

@shreemaan-abhishek shreemaan-abhishek left a comment

Choose a reason for hiding this comment

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

@kys1230, please add test cases and/or provide steps to reproduce this issue (with route/plugin configurations and request format using curl) thanks <3

@kys1230
Copy link
Author

kys1230 commented Aug 15, 2024

I tried Test::nginx but couldn't get it to run. I'll briefly describe the plugins I used and the reproduction method.

The plugin I used is the CORS plugin with the following configuration.

{
  "uri": "/*",
  "name": "kystest",
  "methods": [
    "GET",
    "POST",
    "PUT",
    "DELETE",
    "PATCH",
    "HEAD",
    "OPTIONS",
    "CONNECT",
    "TRACE",
    "PURGE"
  ],
  "host": "kystest.a.b",
  "plugins": {
    "cors": {
      "_meta": {
        "disable": false
      },
      "allow_credential": true,
      "allow_headers": "**",
      "allow_methods": "**",
      "allow_origins": "**",
      "expose_headers": "**",
      "max_age": 5
    }
  },
  "upstream_id": "438941858735850233",
  "status": 1
}

Normal request:

# once
$ curl -s --resolve kystest.a.b:9080:10.41.0.12 -I -XOPTIONS -H 'Origin: http://a.b.c' http://kystest.a.b:9080
HTTP/1.1 200 OK
Date: Thu, 15 Aug 2024 05:15:19 GMT
Content-Type: text/plain; charset=utf-8
Transfer-Encoding: chunked
Connection: keep-alive
Keep-Alive: timeout=60
Vary: Accept-Encoding
X-Request-ID: b03d213c-0393-4719-8aeb-26a4a6ed910c
Access-Control-Allow-Origin: http://a.b.c
Vary: Origin
Access-Control-Allow-Methods: GET,POST,PUT,DELETE,PATCH,HEAD,OPTIONS,CONNECT,TRACE
Access-Control-Max-Age: 5
Access-Control-Expose-Headers: **
Access-Control-Allow-Credentials: true

# multiple
for i in {0..99}; do curl -s --resolve kystest.a.b:9080:10.41.0.12 -I -XOPTIONS -H 'Origin: http://a.b.c' http://kystest.a.b:9080 | grep -q 'Access-Control' && echo ok || echo no ; done | sort | uniq -c 
 100 ok

malicious request is an erroneous request

If the Origin maliciously passes in content with a ',', it will trigger this issue. When the values of the Origin for subsequent normal requests do not equal the values obtained by splitting the malicious request's Origin by ',', the cross-origin response headers will not be returned

Taking http://a.b.c and http://b.c.d as examples, if the Origin of subsequent requests does not equal http://a.b.c or http://b.c.d, this issue may occur with a certain probability. This probability depends on the number of nginx workers. If malicious requests are repeatedly called multiple times, once all workers have processed the malicious requests, this issue will consistently reproduce.

$ curl -s --resolve kystest.a.b:9080:10.41.0.12 -I -XOPTIONS -H 'Origin: http://a.b.c,http://b.c.d' http://kystest.a.b:9080
HTTP/1.1 200 OK
Date: Thu, 15 Aug 2024 05:20:53 GMT
Content-Type: text/plain; charset=utf-8
Transfer-Encoding: chunked
Connection: keep-alive
Keep-Alive: timeout=60
Vary: Accept-Encoding
X-Request-ID: 2417dd05-00cf-43fb-b7cc-cbef7176b044

$ for i in {0..99}; do curl -s --resolve kystest.a.b:9080:10.41.0.12 -I -XOPTIONS -H 'Origin: http://a.b.e' http://kystest.a.b:9080 | grep -q 'Access-Control' && echo ok || echo no ; done | sort | uniq -c
  29 no
  71 ok

# Call the malicious request multiple times
$ for i in {0..99}; do curl -s --resolve kystest.a.b:9080:10.41.0.12 -I -H 'Origin: http://a.b.c,http://b.c.d' -XOPTIONS http://kystest.a.b:9080 | grep -q 'Access-Control' && echo ok || echo no ; done | sort | uniq -c 
 100 no

# At this time, all normal requests will be affected.
for i in {0..99}; do curl -s --resolve kystest.a.b:9080:10.41.0.12 -I -XOPTIONS -H 'Origin: http://a.b.e' http://kystest.a.b:9080 | grep -q 'Access-Control' && echo ok || echo no ; done | sort | uniq -c
 100 no

@kys1230
Copy link
Author

kys1230 commented Aug 15, 2024

In order to ensure that allow_credential=true does not return '*', the code has been revised.

@kys1230
Copy link
Author

kys1230 commented Aug 26, 2024

Is there anything else I need to modify?

@shreemaan-abhishek
Copy link
Contributor

shreemaan-abhishek commented Sep 25, 2024

I'm not a cors expert but

When the values of the Origin for subsequent normal requests do not equal the values obtained by splitting the malicious request's Origin by ',', the cross-origin response headers will not be returned

where is this check performed?

when the request fails, what error does it give? Any error logs?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working size:XS This PR changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants