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

Cors plugin doesn't validate "flat strings" appropriately #3832

Closed
djdmbrwsk opened this issue Oct 5, 2018 · 0 comments
Closed

Cors plugin doesn't validate "flat strings" appropriately #3832

djdmbrwsk opened this issue Oct 5, 2018 · 0 comments
Assignees
Labels

Comments

@djdmbrwsk
Copy link

Summary

The Cors plugin states you can provide "flat strings or PCRE regexes" for the config.origins value. However, flat string origins are not validated safely.

From what I can tell flat strings are evaluated with re_find() (see here). Since there are no line start/end (^, $) symbols surrounding the flat string, as long as an origin starts with the same characters it will be validated.

Steps To Reproduce

  1. Setup an clean Kong environment. I'm using Docker with kong:0.13.0-alpine and postgres:10.1-alpine, but I believe this bug exists in all versions of the plugin (see re_find() link above).
  2. Setup the Cors plugin like this:
curl -X POST \
  http://localhost:8001/plugins \
  -H 'Content-Type: application/json' \
  -d '{
    "name": "cors",
    "config": {
      "origins": [
        "http://my-site.com",
        "http://my-other-site.com"
      ]
    }
  }'
  1. Ping the Proxy with your legitimate origin and confirm you do get the Access-Control-Allow-Origin response header ✅
curl -X OPTIONS \
  http://localhost:8000 \
  -H 'Origin: http://my-site.com' 
  1. Ping the Proxy with a clearly illegitimate origin and confirm you do not get the Access-Control-Allow-Origin response header ✅
curl -X OPTIONS \
  http://localhost:8000 \
  -H 'Origin: http://bad-guys.com' 
  1. Ping the Proxy with a tricky illegitimate origin and confirm you do not get the Access-Control-Allow-Origin response header 😞
curl -X OPTIONS \
  http://localhost:8000 \
  -H 'Origin: http://my-site.com.bad-guys.com' 
@subnetmarco subnetmarco self-assigned this Oct 19, 2018
thibaultcha pushed a commit that referenced this issue Dec 1, 2018
@thibaultcha:

Prior to this patch, configuring the plugin with, e.g.:

    "config": {
      "origins": [
        "http://my-site.com",
        "http://my-other-site.com"
      ]
    }

Would make the following request Origin validate comparison and
injecting the ACAO response header:

    curl -X OPTIONS \
      http://localhost:8000 \
      -H 'Origin: http://my-site.com.bad-guys.com'

This patch's main goal is to anchor the comparison regex in order to
ensure we match the full configured origin.

Additionally, we now ensure that protocol, host, and port are all taken
in consideration when comparing origins, as per RFC 3654 on "Comparing
Origins".

Fix #3832
From #3872

Signed-off-by: Thibault Charbonnier <[email protected]>
thibaultcha pushed a commit that referenced this issue Dec 3, 2018
@thibaultcha:

Prior to this patch, configuring the plugin with, e.g.:

    "config": {
      "origins": [
        "http://my-site.com",
        "http://my-other-site.com"
      ]
    }

Would make the following request Origin validate comparison and
injecting the ACAO response header:

    curl -X OPTIONS \
      http://localhost:8000 \
      -H 'Origin: http://my-site.com.bad-guys.com'

This patch's main goal is to anchor the comparison regex in order to
ensure we match the full configured origin.

Additionally, we now ensure that protocol, host, and port are all taken
in consideration when comparing origins, as per RFC 3654 on "Comparing
Origins".

Fix #3832
From #3872

Signed-off-by: Thibault Charbonnier <[email protected]>
thibaultcha pushed a commit that referenced this issue Dec 3, 2018
@thibaultcha:

Prior to this patch, configuring the plugin with, e.g.:

    "config": {
      "origins": [
        "http://my-site.com",
        "http://my-other-site.com"
      ]
    }

Would make the following request Origin validate comparison and
injecting the ACAO response header:

    curl -X OPTIONS \
      http://localhost:8000 \
      -H 'Origin: http://my-site.com.bad-guys.com'

This patch's main goal is to anchor the comparison regex in order to
ensure we match the full configured origin.

Additionally, we now ensure that protocol, host, and port are all taken
in consideration when comparing origins, as per RFC 3654 on "Comparing
Origins".

Fix #3832
From #3872

Signed-off-by: Thibault Charbonnier <[email protected]>
hishamhm added a commit that referenced this issue Jan 29, 2019
This reverts a regression introduced in #3872, in which regexes were being
forcibly anchored and matched against normalized domains, leading to a
breaking change with regard to the 0.x behavior.

In 0.x, regexes such as `(.*[.])?foo\.test` would accept subdomain entries
(but were subject to bug #3832 as it would also accept `foo.test.evil.test`);
in 1.0.2, the latter is not accepted, but the regular uses failed as well,
because regexes were translated to `^(.*[.])?foo\.test$` but were then matched
against the normalized domains in a way that always included the port.

With this commit, both configured origins and the input header value are
normalized so that default ports are not an issue for both regex and non-regex
cases. We verify if each configured origin is a regex or not, then:

* for non-regex entries, we do plain equality matching against the normalized
  domain
* For regex entries,
  * if the regex contains `:`, we do an anchored match against the normalized
    domain
  * otherwise, we do an anchored match against the host component only
    (to account for the 0.x behavior where ports were not considered)

Matching domains with regexes must be done with care (for starters,
note that dots must be escaped), so we recommend using plain-text full-domain
matching whenever possible.

This change in behavior is arguably a breaking change, but it is a compromise
between a workable behavior and backwards compatibility. Good domain-matching
regexes such as `(.*[.])?foo\.test` will remain matching against the host
component as intended (without being subject to #3832). Naive regexes such as
`.foo.test` will stop "working", but these were vulnerable to #3832 anyway. In
particular, thorough regexes such as
`^https?://(.*[.])?foo\\.test(:(80|90))?$` that performed their own anchoring
remain working as well.
thibaultcha pushed a commit that referenced this issue Jan 30, 2019
This reverts a regression introduced in #3872, in which regexes were
being forcibly anchored and matched against normalized domains, leading
to a breaking change with regard to the 0.x behavior.

In 0.x, regexes such as `(.*[.])?foo\.test` would accept subdomain
entries (but were subject to bug #3832 as it would also accept
`foo.test.evil.test`); in 1.0.2, the latter is not accepted, but the
regular uses failed as well, because regexes were translated to
`^(.*[.])?foo\.test$` but were then matched against the normalized
domains in a way that always included the port.

With this commit, both configured origins and the input header value are
normalized so that default ports are not an issue for both regex and
non-regex cases. We verify if each configured origin is a regex or not,
then:

* for non-regex entries, we do plain equality matching against the normalized
  domain
* For regex entries,
  * if the regex contains `:`, we do an anchored match against the normalized
    domain
  * otherwise, we do an anchored match against the host component only
    (to account for the 0.x behavior where ports were not considered)

Matching domains with regexes must be done with care (for starters, note
that dots must be escaped), so we recommend using plain-text full-domain
matching whenever possible.

This change in behavior is arguably a breaking change, but it is a
compromise between a workable behavior and backwards compatibility. Good
domain-matching regexes such as `(.*[.])?foo\.test` will remain matching
against the host component as intended (without being subject to #3832).
Naive regexes such as `.foo.test` will stop "working", but these were
vulnerable to #3832 anyway. In particular, thorough regexes such as
`^https?://(.*[.])?foo\\.test(:(80|90))?$` that performed their own
anchoring remain working as well.

From #4261

Signed-off-by: Thibault Charbonnier <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants