From cfd72af36ca8c8c6b62c563d6fa1f5ad0caef92f Mon Sep 17 00:00:00 2001 From: Freddy Date: Thu, 19 Nov 2020 10:14:48 -0700 Subject: [PATCH] Require operator:write to get Connect CA config (#9240) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A vulnerability was identified in Consul and Consul Enterprise (“Consul”) such that operators with `operator:read` ACL permissions are able to read the Consul Connect CA configuration when explicitly configured with the `/v1/connect/ca/configuration` endpoint, including the private key. This allows the user to effectively privilege escalate by enabling the ability to mint certificates for any Consul Connect services. This would potentially allow them to masquerade (receive/send traffic) as any service in the mesh. -- This PR increases the permissions required to read the Connect CA's private key when it was configured via the `/connect/ca/configuration` endpoint. They are now `operator:write`. --- .changelog/9240.txt | 3 + agent/consul/connect_ca_endpoint.go | 2 +- agent/consul/connect_ca_endpoint_test.go | 91 +++++++++++++++++++++++- website/pages/api-docs/connect/ca.mdx | 4 +- 4 files changed, 96 insertions(+), 4 deletions(-) create mode 100644 .changelog/9240.txt diff --git a/.changelog/9240.txt b/.changelog/9240.txt new file mode 100644 index 0000000000..5e665e6bcc --- /dev/null +++ b/.changelog/9240.txt @@ -0,0 +1,3 @@ +```release-note:security +Increase the permissions to read from the `/connect/ca/configuration` endpoint to `operator:write`. Previously Connect CA configuration, including the private key, set via this endpoint could be read back by an operator with `operator:read` privileges. [CVE-2020-28053](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-28053) +``` \ No newline at end of file diff --git a/agent/consul/connect_ca_endpoint.go b/agent/consul/connect_ca_endpoint.go index 2198c0ec65..e7ae0c7237 100644 --- a/agent/consul/connect_ca_endpoint.go +++ b/agent/consul/connect_ca_endpoint.go @@ -67,7 +67,7 @@ func (s *ConnectCA) ConfigurationGet( if err != nil { return err } - if rule != nil && rule.OperatorRead(nil) != acl.Allow { + if rule != nil && rule.OperatorWrite(nil) != acl.Allow { return acl.ErrPermissionDenied } diff --git a/agent/consul/connect_ca_endpoint_test.go b/agent/consul/connect_ca_endpoint_test.go index d55c7e4b67..4e2885e473 100644 --- a/agent/consul/connect_ca_endpoint_test.go +++ b/agent/consul/connect_ca_endpoint_test.go @@ -9,8 +9,7 @@ import ( "testing" "time" - "github.com/stretchr/testify/require" - + "github.com/hashicorp/consul/acl" "github.com/hashicorp/consul/agent/connect" ca "github.com/hashicorp/consul/agent/connect/ca" "github.com/hashicorp/consul/agent/structs" @@ -18,6 +17,7 @@ import ( "github.com/hashicorp/consul/testrpc" msgpackrpc "github.com/hashicorp/net-rpc-msgpackrpc" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func testParseCert(t *testing.T, pemValue string) *x509.Certificate { @@ -144,6 +144,93 @@ func TestConnectCAConfig_GetSet(t *testing.T) { } } +func TestConnectCAConfig_GetSet_ACLDeny(t *testing.T) { + t.Parallel() + + dir1, s1 := testServerWithConfig(t, func(c *Config) { + c.ACLDatacenter = "dc1" + c.ACLsEnabled = true + c.ACLMasterToken = TestDefaultMasterToken + c.ACLDefaultPolicy = "deny" + }) + defer os.RemoveAll(dir1) + defer s1.Shutdown() + + codec := rpcClient(t, s1) + defer codec.Close() + + testrpc.WaitForLeader(t, s1.RPC, "dc1") + + opReadToken, err := upsertTestTokenWithPolicyRules( + codec, TestDefaultMasterToken, "dc1", `operator = "read"`) + require.NoError(t, err) + + opWriteToken, err := upsertTestTokenWithPolicyRules( + codec, TestDefaultMasterToken, "dc1", `operator = "write"`) + require.NoError(t, err) + + // Update a config value + newConfig := &structs.CAConfiguration{ + Provider: "consul", + Config: map[string]interface{}{ + "PrivateKey": ` +-----BEGIN EC PRIVATE KEY----- +MHcCAQEEIMoTkpRggp3fqZzFKh82yS4LjtJI+XY+qX/7DefHFrtdoAoGCCqGSM49 +AwEHoUQDQgAEADPv1RHVNRfa2VKRAB16b6rZnEt7tuhaxCFpQXPj7M2omb0B9Fav +q5E0ivpNtv1QnFhxtPd7d5k4e+T7SkW1TQ== +-----END EC PRIVATE KEY-----`, + "RootCert": ` +-----BEGIN CERTIFICATE----- +MIICjDCCAjKgAwIBAgIIC5llxGV1gB8wCgYIKoZIzj0EAwIwFDESMBAGA1UEAxMJ +VGVzdCBDQSAyMB4XDTE5MDMyMjEzNTgyNloXDTI5MDMyMjEzNTgyNlowDjEMMAoG +A1UEAxMDd2ViMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEADPv1RHVNRfa2VKR +AB16b6rZnEt7tuhaxCFpQXPj7M2omb0B9Favq5E0ivpNtv1QnFhxtPd7d5k4e+T7 +SkW1TaOCAXIwggFuMA4GA1UdDwEB/wQEAwIDuDAdBgNVHSUEFjAUBggrBgEFBQcD +AgYIKwYBBQUHAwEwDAYDVR0TAQH/BAIwADBoBgNVHQ4EYQRfN2Q6MDc6ODc6M2E6 +NDA6MTk6NDc6YzM6NWE6YzA6YmE6NjI6ZGY6YWY6NGI6ZDQ6MDU6MjU6NzY6M2Q6 +NWE6OGQ6MTY6OGQ6Njc6NWU6MmU6YTA6MzQ6N2Q6ZGM6ZmYwagYDVR0jBGMwYYBf +ZDE6MTE6MTE6YWM6MmE6YmE6OTc6YjI6M2Y6YWM6N2I6YmQ6ZGE6YmU6YjE6OGE6 +ZmM6OWE6YmE6YjU6YmM6ODM6ZTc6NWU6NDE6NmY6ZjI6NzM6OTU6NTg6MGM6ZGIw +WQYDVR0RBFIwUIZOc3BpZmZlOi8vMTExMTExMTEtMjIyMi0zMzMzLTQ0NDQtNTU1 +NTU1NTU1NTU1LmNvbnN1bC9ucy9kZWZhdWx0L2RjL2RjMS9zdmMvd2ViMAoGCCqG +SM49BAMCA0gAMEUCIGC3TTvvjj76KMrguVyFf4tjOqaSCRie3nmHMRNNRav7AiEA +pY0heYeK9A6iOLrzqxSerkXXQyj5e9bE4VgUnxgPU6g= +-----END CERTIFICATE-----`, + }, + } + + args := &structs.CARequest{ + Datacenter: "dc1", + Config: newConfig, + WriteRequest: structs.WriteRequest{Token: TestDefaultMasterToken}, + } + var reply interface{} + require.NoError(t, msgpackrpc.CallWithCodec(codec, "ConnectCA.ConfigurationSet", args, &reply)) + + t.Run("deny get with operator:read", func(t *testing.T) { + args := &structs.DCSpecificRequest{ + Datacenter: "dc1", + QueryOptions: structs.QueryOptions{Token: opReadToken.SecretID}, + } + + var reply structs.CAConfiguration + err = msgpackrpc.CallWithCodec(codec, "ConnectCA.ConfigurationGet", args, &reply) + assert.True(t, acl.IsErrPermissionDenied(err)) + }) + + t.Run("allow get with operator:write", func(t *testing.T) { + args := &structs.DCSpecificRequest{ + Datacenter: "dc1", + QueryOptions: structs.QueryOptions{Token: opWriteToken.SecretID}, + } + + var reply structs.CAConfiguration + err = msgpackrpc.CallWithCodec(codec, "ConnectCA.ConfigurationGet", args, &reply) + assert.False(t, acl.IsErrPermissionDenied(err)) + assert.Equal(t, newConfig.Config, reply.Config) + }) +} + // This test case tests that the logic around forcing a rotation without cross // signing works when requested (and is denied when not requested). This occurs // if the current CA is not able to cross sign external CA certificates. diff --git a/website/pages/api-docs/connect/ca.mdx b/website/pages/api-docs/connect/ca.mdx index 02ba96fd66..434c45f5ae 100644 --- a/website/pages/api-docs/connect/ca.mdx +++ b/website/pages/api-docs/connect/ca.mdx @@ -121,7 +121,9 @@ The table below shows this endpoint's support for | Blocking Queries | Consistency Modes | Agent Caching | ACL Required | | ---------------- | ----------------- | ------------- | --------------- | -| `YES` | `all` | `none` | `operator:read` | +| `YES` | `all` | `none` | `operator:write` 1 | + +1 ACL required was operator:read prior to versions 1.8.6, 1.7.10, and 1.6.10. ### Sample Request