[Security] Secvuln 8633 Consul configuration allowed repeated keys (#…

…21908) * upgrade hcl package and account for possiblity of duplicates existing already in the cache * upgrade to new tag * add defensive line to prevent potential forever loop * o mod tidy and changelog * Update acl/policy.go * fix raft reversion * go mod tidy * fix test * remove duplicate key in test * remove duplicates from test cases * clean up * go mod tidy * go mod tidy * pull in new hcl tag
hashicorp · Nov 14, 2024 · 32ce338 · 32ce338
1 parent a2e6923
commit 32ce338
Show file tree

Hide file tree

Showing 16 changed files with 81 additions and 27 deletions.
diff --git a/.changelog/21908.txt b/.changelog/21908.txt
@@ -0,0 +1,3 @@
+```release-note:security
+Resolved issue where hcl would allow duplicates of the same key in acl policy configuration.
+```
diff --git a/acl/acl.go b/acl/acl.go
@@ -22,6 +22,9 @@ type Config struct {
 	// WildcardName is the string that represents a request to authorize a wildcard permission
 	WildcardName string
 
+	//by default errors, but in certain instances we want to make sure to maintain backwards compatabilty
+	WarnOnDuplicateKey bool
+
 	// embedded enterprise configuration
 	EnterpriseConfig
 }

diff --git a/acl/policy.go b/acl/policy.go
@@ -310,8 +310,8 @@ func (pr *PolicyRules) Validate(conf *Config) error {
 	return nil
 }
 
-func parse(rules string, conf *Config, meta *EnterprisePolicyMeta) (*Policy, error) {
-	p, err := decodeRules(rules, conf, meta)
+func parse(rules string, warnOnDuplicateKey bool, conf *Config, meta *EnterprisePolicyMeta) (*Policy, error) {
+	p, err := decodeRules(rules, warnOnDuplicateKey, conf, meta)
 	if err != nil {
 		return nil, err
 	}
@@ -338,7 +338,11 @@ func NewPolicyFromSource(rules string, conf *Config, meta *EnterprisePolicyMeta)
 
 	var policy *Policy
 	var err error
-	policy, err = parse(rules, conf, meta)
+	warnOnDuplicateKey := false
+	if conf != nil {
+		warnOnDuplicateKey = conf.WarnOnDuplicateKey
+	}
+	policy, err = parse(rules, warnOnDuplicateKey, conf, meta)
 	return policy, err
 }
 

diff --git a/acl/policy_ce.go b/acl/policy_ce.go
@@ -7,8 +7,9 @@ package acl
 
 import (
 	"fmt"
-
+	"github.com/hashicorp/go-hclog"
 	"github.com/hashicorp/hcl"
+	"strings"
 )
 
 // EnterprisePolicyMeta stub
@@ -30,12 +31,28 @@ func (r *EnterprisePolicyRules) Validate(*Config) error {
 	return nil
 }
 
-func decodeRules(rules string, _ *Config, _ *EnterprisePolicyMeta) (*Policy, error) {
+func decodeRules(rules string, warnOnDuplicateKey bool, _ *Config, _ *EnterprisePolicyMeta) (*Policy, error) {
 	p := &Policy{}
 
-	if err := hcl.Decode(p, rules); err != nil {
+	err := hcl.DecodeErrorOnDuplicates(p, rules)
+
+	if errIsDuplicateKey(err) && warnOnDuplicateKey {
+		//because the snapshot saves the unparsed rules we have to assume some snapshots exist that shouldn't fail, but
+		// have duplicates
+		if err := hcl.Decode(p, rules); err != nil {
+			hclog.Default().Warn("Warning- Duplicate key in ACL Policy ignored", "errorMessage", err.Error())
+			return nil, fmt.Errorf("Failed to parse ACL rules: %v", err)
+		}
+	} else if err != nil {
 		return nil, fmt.Errorf("Failed to parse ACL rules: %v", err)
 	}
 
 	return p, nil
 }
+
+func errIsDuplicateKey(err error) bool {
+	if err == nil {
+		return false
+	}
+	return strings.Contains(err.Error(), "was already set. Each argument can only be defined once")
+}
diff --git a/acl/policy_test.go b/acl/policy_test.go
@@ -342,6 +342,12 @@ func TestPolicySourceParse(t *testing.T) {
 			RulesJSON: `{ "acl": "list" }`, // there is no list policy but this helps to exercise another check in isPolicyValid
 			Err:       "Invalid acl policy",
 		},
+		{
+			Name: "Bad Policy - Duplicate ACL Key",
+			Rules: `acl="read"
+					acl="write"`,
+			Err: "Failed to parse ACL rules: The argument \"acl\" at",
+		},
 		{
 			Name:      "Bad Policy - Agent",
 			Rules:     `agent "foo" { policy = "nope" }`,

diff --git a/agent/consul/acl_endpoint_test.go b/agent/consul/acl_endpoint_test.go
@@ -2169,6 +2169,22 @@ func TestACLEndpoint_PolicySet(t *testing.T) {
 		require.Error(t, err)
 	})
 
+	t.Run("Key Dup", func(t *testing.T) {
+		req := structs.ACLPolicySetRequest{
+			Datacenter: "dc1",
+			Policy: structs.ACLPolicy{
+				Description: "foobar",
+				Name:        "baz2",
+				Rules:       "service \"\" { policy = \"read\" policy = \"write\" }",
+			},
+			WriteRequest: structs.WriteRequest{Token: TestDefaultInitialManagementToken},
+		}
+		resp := structs.ACLPolicy{}
+
+		err := aclEp.PolicySet(&req, &resp)
+		require.Error(t, err)
+	})
+
 	t.Run("Update it", func(t *testing.T) {
 		req := structs.ACLPolicySetRequest{
 			Datacenter: "dc1",

diff --git a/agent/dns_test.go b/agent/dns_test.go
@@ -2367,7 +2367,7 @@ func TestDNS_trimUDPResponse_NoTrim(t *testing.T) {
 		},
 	}
 
-	cfg := loadRuntimeConfig(t, `node_name = "test" data_dir = "a" bind_addr = "127.0.0.1" node_name = "dummy" `)
+	cfg := loadRuntimeConfig(t, `node_name = "test" data_dir = "a" bind_addr = "127.0.0.1"  `)
 	if trimmed := trimUDPResponse(req, resp, cfg.DNSUDPAnswerLimit); trimmed {
 		t.Fatalf("Bad %#v", *resp)
 	}
@@ -2400,7 +2400,7 @@ func TestDNS_trimUDPResponse_NoTrim(t *testing.T) {
 }
 
 func TestDNS_trimUDPResponse_TrimLimit(t *testing.T) {
-	cfg := loadRuntimeConfig(t, `node_name = "test" data_dir = "a" bind_addr = "127.0.0.1" node_name = "dummy" `)
+	cfg := loadRuntimeConfig(t, `node_name = "test" data_dir = "a" bind_addr = "127.0.0.1" `)
 
 	req, resp, expected := &dns.Msg{}, &dns.Msg{}, &dns.Msg{}
 	for i := 0; i < cfg.DNSUDPAnswerLimit+1; i++ {
@@ -2439,7 +2439,7 @@ func TestDNS_trimUDPResponse_TrimLimit(t *testing.T) {
 }
 
 func TestDNS_trimUDPResponse_TrimLimitWithNS(t *testing.T) {
-	cfg := loadRuntimeConfig(t, `node_name = "test" data_dir = "a" bind_addr = "127.0.0.1" node_name = "dummy" `)
+	cfg := loadRuntimeConfig(t, `node_name = "test" data_dir = "a" bind_addr = "127.0.0.1" `)
 
 	req, resp, expected := &dns.Msg{}, &dns.Msg{}, &dns.Msg{}
 	for i := 0; i < cfg.DNSUDPAnswerLimit+1; i++ {
@@ -2486,7 +2486,7 @@ func TestDNS_trimUDPResponse_TrimLimitWithNS(t *testing.T) {
 }
 
 func TestDNS_trimTCPResponse_TrimLimitWithNS(t *testing.T) {
-	cfg := loadRuntimeConfig(t, `node_name = "test" data_dir = "a" bind_addr = "127.0.0.1" node_name = "dummy" `)
+	cfg := loadRuntimeConfig(t, `node_name = "test" data_dir = "a" bind_addr = "127.0.0.1" `)
 
 	req, resp, expected := &dns.Msg{}, &dns.Msg{}, &dns.Msg{}
 	for i := 0; i < 5000; i++ {
@@ -2542,7 +2542,7 @@ func loadRuntimeConfig(t *testing.T, hcl string) *config.RuntimeConfig {
 }
 
 func TestDNS_trimUDPResponse_TrimSize(t *testing.T) {
-	cfg := loadRuntimeConfig(t, `node_name = "test" data_dir = "a" bind_addr = "127.0.0.1" node_name = "dummy" `)
+	cfg := loadRuntimeConfig(t, `node_name = "test" data_dir = "a" bind_addr = "127.0.0.1"  `)
 
 	req, resp := &dns.Msg{}, &dns.Msg{}
 	for i := 0; i < 100; i++ {
@@ -2594,7 +2594,7 @@ func TestDNS_trimUDPResponse_TrimSize(t *testing.T) {
 }
 
 func TestDNS_trimUDPResponse_TrimSizeEDNS(t *testing.T) {
-	cfg := loadRuntimeConfig(t, `node_name = "test" data_dir = "a" bind_addr = "127.0.0.1" node_name = "dummy" `)
+	cfg := loadRuntimeConfig(t, `node_name = "test" data_dir = "a" bind_addr = "127.0.0.1" `)
 
 	req, resp := &dns.Msg{}, &dns.Msg{}
 
@@ -2672,7 +2672,7 @@ func TestDNS_trimUDPResponse_TrimSizeEDNS(t *testing.T) {
 }
 
 func TestDNS_trimUDPResponse_TrimSizeMaxSize(t *testing.T) {
-	cfg := loadRuntimeConfig(t, `node_name = "test" data_dir = "a" bind_addr = "127.0.0.1" node_name = "dummy" `)
+	cfg := loadRuntimeConfig(t, `node_name = "test" data_dir = "a" bind_addr = "127.0.0.1"             `)
 
 	resp := &dns.Msg{}
 

diff --git a/agent/routine-leak-checker/leak_test.go b/agent/routine-leak-checker/leak_test.go
@@ -64,9 +64,7 @@ func setupPrimaryServer(t *testing.T) *agent.TestAgent {
 
 	config := `
 	   server = true
-		datacenter = "primary"
-		primary_datacenter = "primary"
-		
+		datacenter = "primary"	
 		connect {
 			enabled = true
 		}

diff --git a/agent/structs/acl.go b/agent/structs/acl.go
@@ -800,6 +800,11 @@ func (policies ACLPolicies) resolveWithCache(cache *ACLCaches, entConf *acl.Conf
 			continue
 		}
 
+		//pulling from the cache, we don't want to break any rules that are already in the cache
+		if entConf == nil {
+			entConf = &acl.Config{}
+		}
+		entConf.WarnOnDuplicateKey = true
 		p, err := acl.NewPolicyFromSource(policy.Rules, entConf, policy.EnterprisePolicyMeta())
 		if err != nil {
 			return nil, fmt.Errorf("failed to parse %q: %v", policy.Name, err)

diff --git a/agent/structs/acl_test.go b/agent/structs/acl_test.go
@@ -403,7 +403,7 @@ func TestStructs_ACLPolicies_resolveWithCache(t *testing.T) {
 			ID:          "5d5653a1-2c2b-4b36-b083-fc9f1398eb7b",
 			Name:        "policy1",
 			Description: "policy1",
-			Rules:       `node_prefix "" { policy = "read" }`,
+			Rules:       `node_prefix "" { policy = "read", policy = "read", },`,
 			RaftIndex: RaftIndex{
 				CreateIndex: 1,
 				ModifyIndex: 2,
@@ -413,7 +413,7 @@ func TestStructs_ACLPolicies_resolveWithCache(t *testing.T) {
 			ID:          "b35541f0-a88a-48da-bc66-43553c60b628",
 			Name:        "policy2",
 			Description: "policy2",
-			Rules:       `agent_prefix "" { policy = "read" }`,
+			Rules:       `agent_prefix "" { policy = "read" } `,
 			RaftIndex: RaftIndex{
 				CreateIndex: 3,
 				ModifyIndex: 4,
@@ -433,7 +433,8 @@ func TestStructs_ACLPolicies_resolveWithCache(t *testing.T) {
 			ID:          "8bf38965-95e5-4e86-9be7-f6070cc0708b",
 			Name:        "policy4",
 			Description: "policy4",
-			Rules:       `service_prefix "" { policy = "read" }`,
+			//test should still pass even with the duplicate key since its resolving the cache
+			Rules: `service_prefix "" { policy = "read"  policy = "read" }`,
 			RaftIndex: RaftIndex{
 				CreateIndex: 7,
 				ModifyIndex: 8,

diff --git a/go.mod b/go.mod
@@ -68,7 +68,7 @@ require (
 	github.com/hashicorp/go-version v1.2.1
 	github.com/hashicorp/golang-lru v0.5.4
 	github.com/hashicorp/hcdiag v0.5.1
-	github.com/hashicorp/hcl v1.0.0
+	github.com/hashicorp/hcl v1.0.1-vault-7
 	github.com/hashicorp/hcl/v2 v2.14.1
 	github.com/hashicorp/hcp-scada-provider v0.2.4
 	github.com/hashicorp/hcp-sdk-go v0.80.0

diff --git a/go.sum b/go.sum
@@ -488,8 +488,9 @@ github.com/hashicorp/golang-lru/v2 v2.0.0 h1:Lf+9eD8m5pncvHAOCQj49GSN6aQI8XGfI5O
 github.com/hashicorp/golang-lru/v2 v2.0.0/go.mod h1:QeFd9opnmA6QUJc5vARoKUSoFhyfM2/ZepoAG6RGpeM=
 github.com/hashicorp/hcdiag v0.5.1 h1:KZcx9xzRfEOQ2OMbwPxVvHyXwLLRqYpSHxCEOtHfQ6w=
 github.com/hashicorp/hcdiag v0.5.1/go.mod h1:RMC2KkffN9uJ+5mFSaL67ZFVj4CDeetPF2d/53XpwXo=
-github.com/hashicorp/hcl v1.0.0 h1:0Anlzjpi4vEasTeNFn2mLJgTSwt0+6sfsiTG8qcWGx4=
 github.com/hashicorp/hcl v1.0.0/go.mod h1:E5yfLk+7swimpb2L/Alb/PJmXilQ/rhwaUYs4T20WEQ=
+github.com/hashicorp/hcl v1.0.1-vault-7 h1:ag5OxFVy3QYTFTJODRzTKVZ6xvdfLLCA1cy/Y6xGI0I=
+github.com/hashicorp/hcl v1.0.1-vault-7/go.mod h1:XYhtn6ijBSAj6n4YqAaf7RBPS4I06AItNorpy+MoQNM=
 github.com/hashicorp/hcl/v2 v2.14.1 h1:x0BpjfZ+CYdbiz+8yZTQ+gdLO7IXvOut7Da+XJayx34=
 github.com/hashicorp/hcl/v2 v2.14.1/go.mod h1:e4z5nxYlWNPdDSNYX+ph14EvWYMFm3eP0zIUqPc2jr0=
 github.com/hashicorp/hcp-scada-provider v0.2.4 h1:XvctVEd4VqWVlqN1VA4vIhJANstZrc4gd2oCfrFLWZc=

diff --git a/test-integ/go.mod b/test-integ/go.mod
@@ -64,7 +64,7 @@ require (
 	github.com/hashicorp/go-uuid v1.0.3 // indirect
 	github.com/hashicorp/go-version v1.2.1 // indirect
 	github.com/hashicorp/golang-lru v0.5.4 // indirect
-	github.com/hashicorp/hcl v1.0.0 // indirect
+	github.com/hashicorp/hcl v1.0.1-vault-7 // indirect
 	github.com/hashicorp/hcl/v2 v2.16.2 // indirect
 	github.com/hashicorp/memberlist v0.5.0 // indirect
 	github.com/hashicorp/serf v0.10.1 // indirect

diff --git a/test-integ/go.sum b/test-integ/go.sum
@@ -141,8 +141,8 @@ github.com/hashicorp/go-version v1.2.1/go.mod h1:fltr4n8CU8Ke44wwGCBoEymUuxUHl09
 github.com/hashicorp/golang-lru v0.5.0/go.mod h1:/m3WP610KZHVQ1SGc6re/UDhFvYD7pJ4Ao+sR/qLZy8=
 github.com/hashicorp/golang-lru v0.5.4 h1:YDjusn29QI/Das2iO9M0BHnIbxPeyuCHsjMW+lJfyTc=
 github.com/hashicorp/golang-lru v0.5.4/go.mod h1:iADmTwqILo4mZ8BN3D2Q6+9jd8WM5uGBxy+E8yxSoD4=
-github.com/hashicorp/hcl v1.0.0 h1:0Anlzjpi4vEasTeNFn2mLJgTSwt0+6sfsiTG8qcWGx4=
-github.com/hashicorp/hcl v1.0.0/go.mod h1:E5yfLk+7swimpb2L/Alb/PJmXilQ/rhwaUYs4T20WEQ=
+github.com/hashicorp/hcl v1.0.1-vault-7 h1:ag5OxFVy3QYTFTJODRzTKVZ6xvdfLLCA1cy/Y6xGI0I=
+github.com/hashicorp/hcl v1.0.1-vault-7/go.mod h1:XYhtn6ijBSAj6n4YqAaf7RBPS4I06AItNorpy+MoQNM=
 github.com/hashicorp/hcl/v2 v2.16.2 h1:mpkHZh/Tv+xet3sy3F9Ld4FyI2tUpWe9x3XtPx9f1a0=
 github.com/hashicorp/hcl/v2 v2.16.2/go.mod h1:JRmR89jycNkrrqnMmvPDMd56n1rQJ2Q6KocSLCMCXng=
 github.com/hashicorp/logutils v1.0.0/go.mod h1:QIAnNjmIWmVIIkWDTG1z5v++HQmx9WQRO+LraFDTW64=

diff --git a/test/integration/consul-container/go.mod b/test/integration/consul-container/go.mod
@@ -20,7 +20,7 @@ require (
 	github.com/hashicorp/go-multierror v1.1.1
 	github.com/hashicorp/go-uuid v1.0.3
 	github.com/hashicorp/go-version v1.2.1
-	github.com/hashicorp/hcl v1.0.0
+	github.com/hashicorp/hcl v1.0.1-vault-7
 	github.com/hashicorp/serf v0.10.1
 	github.com/itchyny/gojq v0.12.12
 	github.com/mitchellh/copystructure v1.2.0

diff --git a/test/integration/consul-container/go.sum b/test/integration/consul-container/go.sum
@@ -150,8 +150,8 @@ github.com/hashicorp/go-version v1.2.1/go.mod h1:fltr4n8CU8Ke44wwGCBoEymUuxUHl09
 github.com/hashicorp/golang-lru v0.5.0/go.mod h1:/m3WP610KZHVQ1SGc6re/UDhFvYD7pJ4Ao+sR/qLZy8=
 github.com/hashicorp/golang-lru v0.5.4 h1:YDjusn29QI/Das2iO9M0BHnIbxPeyuCHsjMW+lJfyTc=
 github.com/hashicorp/golang-lru v0.5.4/go.mod h1:iADmTwqILo4mZ8BN3D2Q6+9jd8WM5uGBxy+E8yxSoD4=
-github.com/hashicorp/hcl v1.0.0 h1:0Anlzjpi4vEasTeNFn2mLJgTSwt0+6sfsiTG8qcWGx4=
-github.com/hashicorp/hcl v1.0.0/go.mod h1:E5yfLk+7swimpb2L/Alb/PJmXilQ/rhwaUYs4T20WEQ=
+github.com/hashicorp/hcl v1.0.1-vault-7 h1:ag5OxFVy3QYTFTJODRzTKVZ6xvdfLLCA1cy/Y6xGI0I=
+github.com/hashicorp/hcl v1.0.1-vault-7/go.mod h1:XYhtn6ijBSAj6n4YqAaf7RBPS4I06AItNorpy+MoQNM=
 github.com/hashicorp/logutils v1.0.0/go.mod h1:QIAnNjmIWmVIIkWDTG1z5v++HQmx9WQRO+LraFDTW64=
 github.com/hashicorp/mdns v1.0.4/go.mod h1:mtBihi+LeNXGtG8L9dX59gAEa12BDtBQSp4v/YAJqrc=
 github.com/hashicorp/memberlist v0.5.0 h1:EtYPN8DpAURiapus508I4n9CzHs2W+8NZGbmmR/prTM=