Bug 1844990: server/api: Disable unsafe ciphers for CVE-2016-2183 #2420

darkmuggle · 2021-02-18T19:40:09Z

With [1], SHA and 3DES Ciphers are considered unsafe.

http/2 requires these inseucre Ciphers which means that fixing the CVE requires disableing http/2

[1] https://access.redhat.com/security/cve/cve-2016-2183

This change will not conflict with the TLS profile work coming later.

openshift-ci-robot · 2021-02-18T19:40:16Z

@darkmuggle: This pull request references Bugzilla bug 1844990, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.8.0) matches configured target release for branch (4.8.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Details

In response to this:

Bug 1844990: server/api: Disable unsafe ciphers for CVE-2016-2183

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.

darkmuggle · 2021-02-22T14:09:44Z

/retest

yuqi-zhang

I tested this with the steps in the BZ and I see:
SWEET32 (CVE-2016-2183, CVE-2016-6329) not vulnerable (OK)

so I think the functionality is good. Code also generally LGTM, but I am uncertain about some of the specifics. e.g. does disabling HTTP/2 affect anything?

darkmuggle · 2021-02-23T01:18:27Z

so I think the functionality is good. Code also generally LGTM, but I am uncertain about some of the specifics. e.g. does disabling HTTP/2 affect anything?

Disabling http/2 is not really a big deal - the features like multiplexing, prioritization controls, compression, etc -- we're not using. More importantly, http/2 by definition is insecure for the SWEET32 ciphers; there is simply no use http/2 with TLS 1.2 (required with Azure). The only way to use http/2 is to force TLS 1.3 and that can't happen. If the MCS was doing things like streaming media or trying to handle prefetching or HTTP pipelining, then it would be a different story.

sinnykumari · 2021-02-23T09:48:27Z

pkg/server/api.go

inFirst value will always be False as we are not setting it True when it was not found in first.

Good catch -- fixed up.

sinnykumari · 2021-02-23T09:50:43Z

pkg/server/api.go

Since we are matching with Suffix here, it won't match ciphers listed in blacklist that ends with CBC_SHA256, CBC_SHA384 . Are these safe?

SHA256, 384, and 512 are considered secure....for now.

sinnykumari · 2021-02-23T09:52:08Z

pkg/server/api.go

Really like the useful comments you have added throughout the PR ❤️

darkmuggle · 2021-02-23T16:38:17Z

/retest

yuqi-zhang · 2021-02-23T16:48:36Z

pkg/server/api.go

I think you meant inFirst here

My local toolchain must be really wonky -- make verify and make test -- all worked.

With [1], SHA and 3DES are considered unsafe. [1] https://access.redhat.com/security/cve/cve-2016-2183 Signed-off-by: Ben Howard <[email protected]>

sinnykumari · 2021-02-24T09:10:35Z

/lgtm

@darkmuggle As you have done good digging, can you create a followup doc PR into https://github.com/openshift/machine-config-operator/blob/master/docs/MachineConfigServer.md with details and reason of MCO sticking to http and TLS 1.2 version

openshift-ci-robot · 2021-02-24T09:10:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: darkmuggle, sinnykumari, yuqi-zhang

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [darkmuggle,sinnykumari,yuqi-zhang]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2021-02-24T09:11:04Z

/retest