-
Notifications
You must be signed in to change notification settings - Fork 39.2k
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
apiserver: fix cve for CORS #112809
apiserver: fix cve for CORS #112809
Conversation
then should we imply each regexp has a |
I think this is effectively adding "^" at the beginning and "$" at the end: Instead of using
I think an alternative is to use |
f946f10
to
373e9df
Compare
the flag is documented as a list of regular expressions, I think that is on users to define those options on the regexp |
// Dispatch to the next handler | ||
handler.ServeHTTP(w, req) | ||
}) | ||
} | ||
|
||
func isAllowed(origin string, allowedOriginPatternsREs []*regexp.Regexp) bool { | ||
for _, re := range allowedOriginPatternsREs { | ||
if leftRightIndex := re.FindStringIndex(origin); len(leftRightIndex) == 2 && leftRightIndex[1] == len(origin) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure this does not do the right thing when the origin header contains the scheme and/or the port
The :port portion is optional (according to the RFC)
This reminds me a lot of openshift/origin#16204
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @liggitt for the pointer
I made some changes assuming the following:
- we want to proactively prevent this CVE even though the regex defined by the cluster operator does not fully match the host name in origin.
- the kube-apiserver can't change the cluster operator defined regex on the fly, there is no way for us to verify the validity of the changed regex. is it as simple as appending a
(:|$)
to the regexp string?
Alternatively we could:
- No changes in CORS implementation, we can ask the cluster operator to fix their regex to match the entirety of the origin header (how about the case when the origin header has multiple origins?)
- Change the CORS filter to check if the regex matches the entirety of the origin header and report to log or metric that there could be a potential CVE there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we want to proactively prevent this CVE even though the regex defined by the cluster operator does not fully match the host name in origin.
There's multiple categories I can see existing clusters being in:
Unsafe options, for example:
- literal domain name not realizing it is a regex (e.g.
foo.bar.com
, which can matchfooxbar.com
) - regex for a single origin not realizing they are doing a partial match (e.g.
foo\.com
, which can matchfoo.companybar.com
) - regex for multiple origins not realizing they are doing a partial match (e.g.
(foo\.com|bar\.com)
, which can matchfoo.companybar.com
) - regex intending to do partial matches not realizing they are also doing partial suffix matches (e.g.
.*\.foo\.com
, which can matchx.foo.companybar.com
)
Safe options, for example:
- regex which forces matching the whole host (e.g.
//foo\.com(:|$)
) - regex which forces matching the whole origin and scheme (e.g.
https://foo\.com(:|$)
) - regex which pins to the start/end (e.g.
^https://foo\.com(:|$)
)
We don't want to change anything that would make already safe options stop matching what they already matched. Blindly prepending //
and appending (:|$)
to the regex would break those. That only worked in the openshift PR I linked because that was taking literal hostnames as inputs, not regexes of full URLs.
Distinguishing intent when someone has unescaped .
characters in their config is also tricky. Probably it's a bug and they mean't to match a literal .
, so should have written /.
, but we can't be 100% sure.
I think trying to auto-fix things in the unsafe category to be safe is going to be really hard to do without any possibility of breaking someone's intended behavior. Would it make more sense to detect when the provided regex doesn't do anything that would pin to the start (missing ^
or //
) or end (missing :
or $
) and fail validation of the flag with a message indicating how they can modify the flag value? That would have the benefit of prompting the operator to make their config safe in a way they would also probably want to apply to older versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@liggitt please review the changes when you have some time:
- fail validation if the regular expressions in
cors-allowed-origins
does not pin to start/end - run the regular expression on each origin from the header (the
origin
header may contain multiple origins)
/triage accepted |
824c72a
to
f3b3f91
Compare
/test pull-kubernetes-conformance-kind-ga-only-parallel |
} | ||
for _, re := range allowedOriginPatternsREs { | ||
if re.MatchString(origin) { | ||
return true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it accurate that matching one origin in a multi-origin request means we should allow the whole chain? I'm not familiar enough with how multi-origin requests work to know if this is right or wrong, but it seems like this would open the door to an allowed origin of https://good.example.com
permitting a multi-origin chain of https://good.example.com https://bad.example.com
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@liggitt After diffing a bit, this is what the RFC says about multiple origins in the Origin
header
In some cases, a number of origins contribute to causing the user
agents to issue an HTTP request. In those cases, the user agent MAY
list all the origins in the Origin header field. For example, if the
HTTP request was initially issued by one origin but then later
redirected by another origin, the user agent MAY inform the server
that two origins were involved in causing the user agent to issue the
request.
So theoretically it is possible to have multiple origins in the Origin
header, then I looked at the CORS spec here:
https://www.w3.org/TR/2020/SPSD-cors-20200602/#access-control-allow-origin-response-header
Access-Control-Allow-Origin = "Access-Control-Allow-Origin" ":" origin-list-or-null | "*"
In practice the origin-list-or-null production is more constrained. Rather than allowing a space-separated list of origins, it is either a single origin or the string "null".
If the response includes zero or more than one Access-Control-Allow-Origin header values, return fail and terminate this algorithm
I can think of the following options if the Origin
header contains multiple origins and CORS is enabled:
- A: do nothing:
- if the regexp matches, the CORS filter will include the matched origin in the
Access-Control-Allow-Origin
response header, not all the origins. (I am going to write a test to observe this behavior) - if the regexp does not match - did the user expect a certain origin from the
Origin
header to match the regular expression (today we usereq.Header.Get("Origin")
and that returns the first origin.
- if the regexp matches, the CORS filter will include the matched origin in the
- B: if the
Origin
header has multiple origins we don't send CORS response headers (we can send a Warning to the client) - C: if the
Origin
header has multiple origins, we run the regular expression(s) against each origin and send the first matching origin viaAccess-Control-Allow-Origin
depending on how we want to approach it, I will open a separate PR, this PR seems orthogonal to this issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a test in #115242 to understand the current behavior with multiple origins in Origin
header(s)
if len(regexp) == 0 { | ||
continue | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this correct, or is this a bug in their config that produces a regex that matches anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, or it could be due to --cors-allowed-origins=""
, we should return a validation error if that happens
staging/src/k8s.io/apiserver/pkg/server/options/server_run_options.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiserver/pkg/server/options/server_run_options.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiserver/pkg/server/options/server_run_options.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiserver/pkg/server/options/server_run_options.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiserver/pkg/server/options/server_run_options.go
Outdated
Show resolved
Hide resolved
8d30f80
to
d11924d
Compare
/lgtm |
LGTM label has been added. Git tree hash: 236e386d33be922f62a0641df636f7855c3f4f00
|
Thanks, improved validation on the command-line flag lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: liggitt, tkashem The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kubernetes#112809 tightened what regular expressions are allowed and now requires that they start matching with a double dash.
When using
I think the square brackets can be ignored, the actual parameter value is kube-apiserver then fails:
Replacing the single slash with double slashes fixes it for me. But I'm wondering whether the check is too strict? |
kubernetes#112809 tightened what regular expressions are allowed and now requires that they start matching with a double dash.
kubernetes#112809 tightened what regular expressions are allowed and now requires that they start matching with a double dash.
kubernetes#112809 tightened what regular expressions are allowed and now requires that they start matching with a double dash.
What type of PR is this?
/kind bug
What this PR does / why we need it:
This makes an attempt to fix CVE-2022-1996
https://huntr.dev/bounties/be837427-415c-4d8c-808b-62ce20aa84f1/
--cors-allowed-origins
server run option matches the start and end of thehost
inside of anorigin
header. If not, then fail with an error so that the cluster operator can modify the regular expression accordingly.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: