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

SWEET32 vulnerability on metrics-server exposed port #310

Closed
w32-blaster opened this issue Sep 5, 2019 · 29 comments
Closed

SWEET32 vulnerability on metrics-server exposed port #310

w32-blaster opened this issue Sep 5, 2019 · 29 comments
Labels
lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed.

Comments

@w32-blaster
Copy link

Our internal vulnerability scanner found that metrics-server is open to the SWEET32 vulnerability.
We have double checked with different tools.
Here is the report from one of the tools:

 Testing for SWEET32 (Birthday Attacks on 64-bit Block Ciphers)

 SWEET32 (CVE-2016-2183, CVE-2016-6329)    VULNERABLE, uses 64 bit block ciphers
@jasinner
Copy link

There was a previous request to disable 3DES ciphers in Golang by default, [0]. However the request was rejected because Internet Explorer on Windows XP still uses those ciphers. I think that's the better place to address this issue.

If you read Red Hat's CVE page for CVE-2016-2183, [1], you'll see that a mitigation for this attack is to prefer stronger ciphers over 3DES. Golang is doing that as there are 17 stronger ciphers offered before 3DES, [2].

Cipher suite negotiation happens during a TLS handshake, and the weaker ciphers will only be chosen if the client doesn't support the stronger ones offered first. Also, CVE-2016-2183 allows an attacker to break the encryption of long lived connections. I think it's unlikely it could be used to break encryption of a HTTPS session, which are normally short lived. In the POC, [3] they mention that it took a single HTTPS session with 785 GB of traffic to break the encryption.

Regards,
Jason Shepherd
Red Hat Product Security

[0] golang/go#21144
[1] https://access.redhat.com/security/cve/cve-2016-2183
[2] https://github.com/golang/go/blob/release-branch.go1.10/src/crypto/tls/cipher_suites.go#L78
[3] https://sweet32.info/

@serathius
Copy link
Contributor

Is this problem actionable by metrics-server or should be left to Golang community?

@w32-blaster
Copy link
Author

Assuming we can explicitly disable ciphers if this would be supported by the metrics-server implementation. That’s what nginx has as well.

@serathius
Copy link
Contributor

Such changes should be done throughout all apiservers in kubernetes.
I think better place for proposing this change would be in https://github.com/kubernetes/apiserver/issues

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 4, 2020
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Mar 5, 2020
@LykinsN
Copy link

LykinsN commented Apr 1, 2020

What's the current status of this ask? We're encountering the same issues as the OP and it's mandated that we 'remediate' this, such that there are no impacted ports being reported back to our vulnerability scanner. At this point, I've implemented a subset of allowed ciphers for kubelet, kube-apiserver, as well having upgraded OpenSSL to 1.1.1d. However, I'm still seeing the vulnerabilities get reported back, both on port 4443 for metrics-server as we ll as port 10250 for kubelet.

I haven't seen any movement on it in either the Golang repository or in kube-apiserver, so I wasn't sure if this item was still being considered at all.

@serathius
Copy link
Contributor

My understanding is that it needs to be first fixed in https://github.com/kubernetes/apiserver
PRs are welcome

@fejta-bot
Copy link

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

@k8s-ci-robot
Copy link
Contributor

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@robbiezhang
Copy link

/reopen

@k8s-ci-robot
Copy link
Contributor

@robbiezhang: You can't reopen an issue/PR unless you authored it or you are a collaborator.

In response to this:

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@robbiezhang
Copy link

Can we reopen this issue?
The generic apiserver support to override tls cipher suite.
https://github.com/kubernetes/apiserver/blob/e9efb24530b35b891051ceb7723873845f5e852e/pkg/server/config.go#L260

apiserver and kubelet support to pass in custom tls cipher suites
kubernetes/kubernetes#48859

@robbiezhang
Copy link

@serathius
Copy link
Contributor

/reopen

@k8s-ci-robot
Copy link
Contributor

@serathius: Reopened this issue.

In response to this:

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot reopened this Aug 20, 2020
@serathius
Copy link
Contributor

@robbiezhang Thanks for pointing this out. Are you interested in proposing a list of ciphers and sending a PR that configures them?

@robbiezhang
Copy link

@serathius, I tried the following tls-cipher-suite, it should mitigate the issue.
--tls-cipher-suites=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384

But it's much more restrict than the default. Do you have any concern to use them?

@serathius
Copy link
Contributor

serathius commented Aug 21, 2020

I think it would make sense to follow some recommendations, so not only we address current concerns but have some guidance for future.

In one of my older project I used Mozzilla recommendations for TLS configuration. Looks like they have ready configurations for golang https://ssl-config.mozilla.org/#server=go&version=1.12&config=intermediate&hsts=false&guideline=5.6

From what I see they suggest different configuration based on golang version. This means that it would apply differently to depending on Metrics Server version.

  • For golang 1.14 they don't suggest any changes. I interpret it as we could upgrade golang 1.14 and don't need to override TLS config.
  • For golang 1.12 they suggest list of ciphers below. This would affect Metrics Server version 0.3.*.

For golang 1.12 they suggest configuring:

            tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,
            tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,
            tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,
            tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,
            tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305,
            tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305

Actions that I would suggest:

  • Upgrade golang on master branch
  • Update manifests for 0.3.8 release to enforce those ciphers

Other option would be to release 0.3.9 with updated golang, but we didn't plan to make new releases on this branch and it would require much more work.

What do you think? Does it make sense?
@s-urbaniak for visibility

@robbiezhang
Copy link

The Mozila recommendations are the same as the CIS recommendations. My list doesn't contain the CHACHA policy 1305 because they are not FIPS 140-2 compliant. I think we can follow CIS recommendations.

@fejta-bot
Copy link

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

@k8s-ci-robot
Copy link
Contributor

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@nrvnrvn
Copy link

nrvnrvn commented Dec 9, 2020

@serathius as far as I can tell the issue was closed due to inactivity but it is still not fixed, right?

@serathius
Copy link
Contributor

Metrics Server v0.4.x uses newer version of apiserver which allows overriding suites using --tls-cipher-suites flag.

Default MS configuration is still vulnerable to SWEET32, but now you can fix that by providing command line flag as described by @robbiezhang in #310 (comment)

I think we should still secure the default configuration, but I would like to get some opinion from security/apiserver people if this shouldn't be fixed by apiserver library itself.
@logicalhan @mikedanese

@serathius serathius reopened this Dec 9, 2020
@mikedanese
Copy link

Having 3DES enabled can trip automated vulnerability scanners, but it is not a vulnerability in itself and PCI compliance does not require that it be disabled. 3DES is not used with any client that supports anything better and all modern clients (e.g. all Kubernetes core clients) will negotiate the use of modern ciphers. Having 3DES enabled on a server does not compromise the security of clients using stronger cipher suites.

Upgrade golang on master branch

Please please please keep go up to date :).

Update manifests for 0.3.8 release to enforce those ciphers

In general, we trust the central expertise of the go crypto maintainers and don't deviate from the go stdlib defaults when it comes to TLS. Users can opt in to a smaller set of supported ciphers using --tls-cipher-suites. Keep in mind that disabling 3DES does not improve the first-order security of the system—it just breaks clients that cannot do better. That might lead to helpful second-order effects (e.g. they update their clients), but the benefit is indirect.

If we do make changes, we should strive for some consistency. If we modify defaults, we should modify them in "k8s.io/apiserver" (or better "crypto/tls").

@mikedanese
Copy link

FWIW, I'll also add:

I would expect disabling 3DES suites here would be exceedingly unlikely to break anyone given that virtually all requests to metrics-server come from kube-apiserver and are secured with AES-128-GCM. That reduces the risk, but also reduces the benefit.

@nrvnrvn
Copy link

nrvnrvn commented Jan 10, 2021

I tend to agree that deviating from the go stdlib defaults may be more critical. Especially given that there is a handy option to opt in and explicitly set cipher suites for virtually any Kubernetes component for those cases where disabling weak ciphers is a hard requirement.

An upstream issue worth following:
golang/go#41476

@fejta-bot
Copy link

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-contributor-experience at kubernetes/community.
/close

@k8s-ci-robot
Copy link
Contributor

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-contributor-experience at kubernetes/community.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed.
Projects
None yet
Development

No branches or pull requests

9 participants