From 90040f8bffb311e6cd8599273e95b607175e311f Mon Sep 17 00:00:00 2001 From: Matt Keeler Date: Mon, 4 Mar 2019 14:52:45 +0000 Subject: [PATCH] Fixes for CVE-2019-8336 Fix error in detecting raft replication errors. Detect redacted token secrets and prevent attempting to insert. Add a Redacted field to the TokenBatchRead and TokenRead RPC endpoints This will indicate whether token secrets have been redacted. Ensure any token with a redacted secret in secondary datacenters is removed. Test that redacted tokens cannot be replicated. --- agent/consul/acl_endpoint.go | 4 + agent/consul/acl_replication.go | 9 +- agent/consul/acl_replication_test.go | 146 +++++++++++++++++++++++++++ agent/consul/leader.go | 15 +++ agent/consul/server_test.go | 9 +- agent/structs/acl.go | 6 +- 6 files changed, 181 insertions(+), 8 deletions(-) diff --git a/agent/consul/acl_endpoint.go b/agent/consul/acl_endpoint.go index 138ff845ee..ed152a0faf 100644 --- a/agent/consul/acl_endpoint.go +++ b/agent/consul/acl_endpoint.go @@ -213,6 +213,9 @@ func (a *ACL) TokenRead(args *structs.ACLTokenGetRequest, reply *structs.ACLToke index, token, err = state.ACLTokenGetByAccessor(ws, args.TokenID) if token != nil { a.srv.filterACLWithAuthorizer(rule, &token) + if !rule.ACLWrite() { + reply.Redacted = true + } } } else { index, token, err = state.ACLTokenGetBySecret(ws, args.TokenID) @@ -589,6 +592,7 @@ func (a *ACL) TokenBatchRead(args *structs.ACLTokenBatchGetRequest, reply *struc a.srv.filterACLWithAuthorizer(rule, &tokens) reply.Index, reply.Tokens = index, tokens + reply.Redacted = !rule.ACLWrite() return nil }) } diff --git a/agent/consul/acl_replication.go b/agent/consul/acl_replication.go index 88cc37c78d..d691895b67 100644 --- a/agent/consul/acl_replication.go +++ b/agent/consul/acl_replication.go @@ -113,7 +113,7 @@ func (s *Server) updateLocalACLPolicies(policies structs.ACLPolicies, ctx contex if err != nil { return false, fmt.Errorf("Failed to apply policy upserts: %v", err) } - if respErr, ok := resp.(error); ok && err != nil { + if respErr, ok := resp.(error); ok && respErr != nil { return false, fmt.Errorf("Failed to apply policy upsert: %v", respErr) } s.logger.Printf("[DEBUG] acl: policy replication - upserted 1 batch with %d policies of size %d", batchEnd-batchStart, batchSize) @@ -283,6 +283,9 @@ func (s *Server) updateLocalACLTokens(tokens structs.ACLTokens, ctx context.Cont batchSize := 0 batchEnd := batchStart for ; batchEnd < len(tokens) && batchSize < aclBatchUpsertSize; batchEnd += 1 { + if tokens[batchEnd].SecretID == redactedToken { + return false, fmt.Errorf("Detected redacted token secrets: stopping token update round - verify that the replication token in use has acl:write permissions.") + } batchSize += tokens[batchEnd].EstimateSize() } @@ -295,7 +298,7 @@ func (s *Server) updateLocalACLTokens(tokens structs.ACLTokens, ctx context.Cont if err != nil { return false, fmt.Errorf("Failed to apply token upserts: %v", err) } - if respErr, ok := resp.(error); ok && err != nil { + if respErr, ok := resp.(error); ok && respErr != nil { return false, fmt.Errorf("Failed to apply token upserts: %v", respErr) } @@ -491,6 +494,8 @@ func (s *Server) replicateACLTokens(lastRemoteIndex uint64, ctx context.Context) tokens, err = s.fetchACLTokensBatch(res.LocalUpserts) if err != nil { return 0, false, fmt.Errorf("failed to retrieve ACL token updates: %v", err) + } else if tokens.Redacted { + return 0, false, fmt.Errorf("failed to retrieve unredacted tokens - replication token in use does not grant acl:write") } s.logger.Printf("[DEBUG] acl: token replication - downloaded %d tokens", len(tokens.Tokens)) diff --git a/agent/consul/acl_replication_test.go b/agent/consul/acl_replication_test.go index 4bebacbdcf..6d9147e415 100644 --- a/agent/consul/acl_replication_test.go +++ b/agent/consul/acl_replication_test.go @@ -4,6 +4,7 @@ import ( "fmt" "os" "testing" + "time" "github.com/hashicorp/consul/acl" "github.com/hashicorp/consul/agent/structs" @@ -563,3 +564,148 @@ func TestACLReplication_Policies(t *testing.T) { checkSame(r) }) } + +func TestACLReplication_TokensRedacted(t *testing.T) { + t.Parallel() + dir1, s1 := testServerWithConfig(t, func(c *Config) { + c.ACLDatacenter = "dc1" + c.ACLsEnabled = true + c.ACLMasterToken = "root" + }) + defer os.RemoveAll(dir1) + defer s1.Shutdown() + testrpc.WaitForLeader(t, s1.RPC, "dc1") + client := rpcClient(t, s1) + defer client.Close() + + // Create the ACL Write Policy + policyArg := structs.ACLPolicySetRequest{ + Datacenter: "dc1", + Policy: structs.ACLPolicy{ + Name: "token-replication-redacted", + Description: "token-replication-redacted", + Rules: `acl = "write"`, + }, + WriteRequest: structs.WriteRequest{Token: "root"}, + } + var policy structs.ACLPolicy + require.NoError(t, s1.RPC("ACL.PolicySet", &policyArg, &policy)) + + // Create the dc2 replication token + tokenArg := structs.ACLTokenSetRequest{ + Datacenter: "dc1", + ACLToken: structs.ACLToken{ + Description: "dc2-replication", + Policies: []structs.ACLTokenPolicyLink{ + structs.ACLTokenPolicyLink{ + ID: policy.ID, + }, + }, + Local: false, + }, + WriteRequest: structs.WriteRequest{Token: "root"}, + } + + var token structs.ACLToken + require.NoError(t, s1.RPC("ACL.TokenSet", &tokenArg, &token)) + + dir2, s2 := testServerWithConfig(t, func(c *Config) { + c.Datacenter = "dc2" + c.ACLDatacenter = "dc1" + c.ACLsEnabled = true + c.ACLTokenReplication = true + c.ACLReplicationRate = 100 + c.ACLReplicationBurst = 100 + c.ACLReplicationApplyLimit = 1000000 + }) + s2.tokens.UpdateReplicationToken(token.SecretID, tokenStore.TokenSourceConfig) + testrpc.WaitForLeader(t, s2.RPC, "dc2") + defer os.RemoveAll(dir2) + defer s2.Shutdown() + + // Try to join. + joinWAN(t, s2, s1) + testrpc.WaitForLeader(t, s2.RPC, "dc2") + testrpc.WaitForLeader(t, s2.RPC, "dc1") + waitForNewACLs(t, s2) + + // ensures replication is working ok + retry.Run(t, func(r *retry.R) { + var tokenResp structs.ACLTokenResponse + req := structs.ACLTokenGetRequest{ + Datacenter: "dc2", + TokenID: "root", + TokenIDType: structs.ACLTokenSecret, + QueryOptions: structs.QueryOptions{Token: "root"}, + } + err := s2.RPC("ACL.TokenRead", &req, &tokenResp) + require.NoError(r, err) + require.Equal(r, "root", tokenResp.Token.SecretID) + + var status structs.ACLReplicationStatus + statusReq := structs.DCSpecificRequest{ + Datacenter: "dc2", + } + require.NoError(r, s2.RPC("ACL.ReplicationStatus", &statusReq, &status)) + // ensures that tokens are not being synced + require.True(r, status.ReplicatedTokenIndex > 0, "ReplicatedTokenIndex not greater than 0") + + }) + + // modify the replication policy to change to only granting read privileges + policyArg = structs.ACLPolicySetRequest{ + Datacenter: "dc1", + Policy: structs.ACLPolicy{ + ID: policy.ID, + Name: "token-replication-redacted", + Description: "token-replication-redacted", + Rules: `acl = "read"`, + }, + WriteRequest: structs.WriteRequest{Token: "root"}, + } + require.NoError(t, s1.RPC("ACL.PolicySet", &policyArg, &policy)) + + // Create the another token so that replication will attempt to read it. + tokenArg = structs.ACLTokenSetRequest{ + Datacenter: "dc1", + ACLToken: structs.ACLToken{ + Description: "management", + Policies: []structs.ACLTokenPolicyLink{ + structs.ACLTokenPolicyLink{ + ID: structs.ACLPolicyGlobalManagementID, + }, + }, + Local: false, + }, + WriteRequest: structs.WriteRequest{Token: "root"}, + } + var token2 structs.ACLToken + + // record the time right before we are touching the token + minErrorTime := time.Now() + require.NoError(t, s1.RPC("ACL.TokenSet", &tokenArg, &token2)) + + retry.Run(t, func(r *retry.R) { + var tokenResp structs.ACLTokenResponse + req := structs.ACLTokenGetRequest{ + Datacenter: "dc2", + TokenID: redactedToken, + TokenIDType: structs.ACLTokenSecret, + QueryOptions: structs.QueryOptions{Token: redactedToken}, + } + err := s2.RPC("ACL.TokenRead", &req, &tokenResp) + // its not an error for the secret to not be found. + require.NoError(r, err) + require.Nil(r, tokenResp.Token) + + var status structs.ACLReplicationStatus + statusReq := structs.DCSpecificRequest{ + Datacenter: "dc2", + } + require.NoError(r, s2.RPC("ACL.ReplicationStatus", &statusReq, &status)) + // ensures that tokens are not being synced + require.True(r, status.ReplicatedTokenIndex < token2.CreateIndex, "ReplicatedTokenIndex is not less than the token2s create index") + // ensures that token replication is erroring + require.True(r, status.LastError.After(minErrorTime), "Replication LastError not after the minErrorTime") + }) +} diff --git a/agent/consul/leader.go b/agent/consul/leader.go index fe26a95708..a44a0116f6 100644 --- a/agent/consul/leader.go +++ b/agent/consul/leader.go @@ -410,6 +410,21 @@ func (s *Server) initializeACLs(upgrade bool) error { // leader. s.acls.cache.Purge() + // Remove any token affected by CVE-2019-8336 + if !s.InACLDatacenter() { + _, token, err := s.fsm.State().ACLTokenGetBySecret(nil, redactedToken) + if err == nil && token != nil { + req := structs.ACLTokenBatchDeleteRequest{ + TokenIDs: []string{token.AccessorID}, + } + + _, err := s.raftApply(structs.ACLTokenDeleteRequestType, &req) + if err != nil { + return fmt.Errorf("failed to remove token with a redacted secret: %v", err) + } + } + } + if s.InACLDatacenter() { if s.UseLegacyACLs() && !upgrade { s.logger.Printf("[INFO] acl: initializing legacy acls") diff --git a/agent/consul/server_test.go b/agent/consul/server_test.go index 3c00a4d85b..a338fa8341 100644 --- a/agent/consul/server_test.go +++ b/agent/consul/server_test.go @@ -239,13 +239,12 @@ func TestServer_JoinLAN(t *testing.T) { } func TestServer_LANReap(t *testing.T) { - t.Parallel() dir1, s1 := testServerWithConfig(t, func(c *Config) { c.Datacenter = "dc1" c.Bootstrap = true c.SerfFloodInterval = 100 * time.Millisecond c.SerfLANConfig.ReconnectTimeout = 250 * time.Millisecond - c.SerfLANConfig.ReapInterval = 500 * time.Millisecond + c.SerfLANConfig.ReapInterval = 300 * time.Millisecond }) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -255,7 +254,7 @@ func TestServer_LANReap(t *testing.T) { c.Bootstrap = false c.SerfFloodInterval = 100 * time.Millisecond c.SerfLANConfig.ReconnectTimeout = 250 * time.Millisecond - c.SerfLANConfig.ReapInterval = 500 * time.Millisecond + c.SerfLANConfig.ReapInterval = 300 * time.Millisecond }) defer os.RemoveAll(dir2) @@ -264,7 +263,7 @@ func TestServer_LANReap(t *testing.T) { c.Bootstrap = false c.SerfFloodInterval = 100 * time.Millisecond c.SerfLANConfig.ReconnectTimeout = 250 * time.Millisecond - c.SerfLANConfig.ReapInterval = 500 * time.Millisecond + c.SerfLANConfig.ReapInterval = 300 * time.Millisecond }) defer os.RemoveAll(dir3) defer s3.Shutdown() @@ -273,6 +272,8 @@ func TestServer_LANReap(t *testing.T) { joinLAN(t, s2, s1) joinLAN(t, s3, s1) + testrpc.WaitForLeader(t, s1.RPC, "dc1") + testrpc.WaitForLeader(t, s2.RPC, "dc1") testrpc.WaitForLeader(t, s3.RPC, "dc1") retry.Run(t, func(r *retry.R) { diff --git a/agent/structs/acl.go b/agent/structs/acl.go index d1a6fc250f..5e052b414f 100644 --- a/agent/structs/acl.go +++ b/agent/structs/acl.go @@ -621,13 +621,15 @@ type ACLTokenBootstrapRequest struct { // ACLTokenResponse returns a single Token + metadata type ACLTokenResponse struct { - Token *ACLToken + Token *ACLToken + Redacted bool // whether the token's secret was redacted QueryMeta } // ACLTokenBatchResponse returns multiple Tokens associated with the same metadata type ACLTokenBatchResponse struct { - Tokens []*ACLToken + Tokens []*ACLToken + Redacted bool // whether the token secrets were redacted. QueryMeta }