From ac90c6f008f6915584e9d26b127808d18f7ac588 Mon Sep 17 00:00:00 2001 From: Luiz Aoqui Date: Mon, 17 Jul 2023 20:35:28 -0400 Subject: [PATCH] acl: fix parsing of policies with blocks w/o label An ACL policy with a block without label generates unexpected results. For example, a policy such as this: ``` namespace { policy = "read" } ``` Is applied to a namespace called `policy` instead of the documented behaviour of applying it to the `default` namespace. This happens because of the way HCL1 decodes blocks. Since it doesn't know if a block is expected to have a label it applies the `key` tag to the content of the block and, in the example above, the first key is `policy`, so it sets that as the `namespace` block label. Since this happens internally in the HCL decoder it's not possible to detect the problem externally. Fixing the problem inside the decoder is challenging because the JSON and HCL parsers generate different ASTs that makes impossible to differentiate between a JSON tree from an invalid HCL tree within the decoder. The fix in this commit consists of manually parsing the policy after decoding to clear labels that were not set in the file. This allows the validation rules to consistently catch and return any errors, no matter if the policy is an invalid HCL or JSON. --- .changelog/17908.txt | 3 + acl/policy.go | 72 +++- acl/policy_test.go | 376 ++++++++++++++++++ .../content/docs/upgrade/upgrade-specific.mdx | 56 +++ 4 files changed, 506 insertions(+), 1 deletion(-) create mode 100644 .changelog/17908.txt diff --git a/.changelog/17908.txt b/.changelog/17908.txt new file mode 100644 index 000000000..b0cc93af3 --- /dev/null +++ b/.changelog/17908.txt @@ -0,0 +1,3 @@ +```release-note:security +acl: Fixed a bug where a namespace ACL policy without label was applied to an unexpected namespace. [CVE-2023-3072](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2023-3072) +``` diff --git a/acl/policy.go b/acl/policy.go index 554c9c462..744e403eb 100644 --- a/acl/policy.go +++ b/acl/policy.go @@ -4,10 +4,12 @@ package acl import ( + "errors" "fmt" "regexp" "github.com/hashicorp/hcl" + "github.com/hashicorp/hcl/hcl/ast" ) const ( @@ -477,5 +479,73 @@ func hclDecode(p *Policy, rules string) (err error) { } }() - return hcl.Decode(p, rules) + if err = hcl.Decode(p, rules); err != nil { + return err + } + + // Manually parse the policy to fix blocks without labels. + // + // Due to a bug in the way HCL decodes files, a block without a label may + // return an incorrect key value and make it impossible to determine if the + // key was set by the user or incorrectly set by the decoder. + // + // By manually parsing the file we are able to determine if the label is + // missing in the file and set them to an empty string so the policy + // validation can return the appropriate errors. + root, err := hcl.Parse(rules) + if err != nil { + return fmt.Errorf("failed to parse policy: %w", err) + } + + list, ok := root.Node.(*ast.ObjectList) + if !ok { + return errors.New("error parsing: root should be an object") + } + + nsList := list.Filter("namespace") + for i, nsObj := range nsList.Items { + // Fix missing namespace key. + if len(nsObj.Keys) == 0 { + p.Namespaces[i].Name = "" + } + + // Fix missing variable paths. + nsOT, ok := nsObj.Val.(*ast.ObjectType) + if !ok { + continue + } + varsList := nsOT.List.Filter("variables") + if varsList == nil || len(varsList.Items) == 0 { + continue + } + + varsObj, ok := varsList.Items[0].Val.(*ast.ObjectType) + if !ok { + continue + } + paths := varsObj.List.Filter("path") + for j, path := range paths.Items { + if len(path.Keys) == 0 { + p.Namespaces[i].Variables.Paths[j].PathSpec = "" + } + } + } + + npList := list.Filter("node_pool") + for i, npObj := range npList.Items { + // Fix missing node pool key. + if len(npObj.Keys) == 0 { + p.NodePools[i].Name = "" + } + } + + hvList := list.Filter("host_volume") + for i, hvObj := range hvList.Items { + // Fix missing host volume key. + if len(hvObj.Keys) == 0 { + p.HostVolumes[i].Name = "" + } + } + + return nil } diff --git a/acl/policy_test.go b/acl/policy_test.go index ec6086a79..261d89332 100644 --- a/acl/policy_test.go +++ b/acl/policy_test.go @@ -74,6 +74,18 @@ func TestParse(t *testing.T) { namespace "autoscaler" { policy = "scale" } + host_volume "production-tls-*" { + capabilities = ["mount-readonly"] + } + host_volume "staging-tls-*" { + policy = "write" + } + node_pool "prod" { + capabilities = ["read"] + } + node_pool "dev" { + policy = "write" + } agent { policy = "read" } @@ -175,6 +187,251 @@ func TestParse(t *testing.T) { }, }, }, + HostVolumes: []*HostVolumePolicy{ + { + Name: "production-tls-*", + Capabilities: []string{"mount-readonly"}, + }, + { + Name: "staging-tls-*", + Policy: "write", + Capabilities: []string{ + "mount-readonly", + "mount-readwrite", + }, + }, + }, + NodePools: []*NodePoolPolicy{ + { + Name: "prod", + Capabilities: []string{"read"}, + }, + { + Name: "dev", + Policy: "write", + Capabilities: []string{"delete", "read", "write"}, + }, + }, + Agent: &AgentPolicy{ + Policy: PolicyRead, + }, + Node: &NodePolicy{ + Policy: PolicyWrite, + }, + Operator: &OperatorPolicy{ + Policy: PolicyDeny, + }, + Quota: &QuotaPolicy{ + Policy: PolicyRead, + }, + Plugin: &PluginPolicy{ + Policy: PolicyRead, + }, + }, + }, + { + ` + { + "namespace": [ + { + "default": { + "policy": "read" + }, + }, + { + "other": { + "policy": "write" + }, + }, + { + "secret": { + "capabilities": [ + "deny", + "read-logs" + ] + } + }, + { + "apps": { + "variables": [ + { + "path": [ + { + "jobs/write-does-not-imply-read-or-delete": { + "capabilities": ["write"], + }, + }, + { + "project/read-implies-list": { + "capabilities": ["read"], + }, + }, + { + "project/explicit": { + "capabilities": ["read", "list", "destroy"], + }, + }, + ], + }, + ], + }, + }, + { + "autoscaler": { + "policy": "scale" + }, + }, + ], + "host_volume": [ + { + "production-tls-*": { + "capabilities": ["mount-readonly"] + } + }, + { + "staging-tls-*": { + "policy": "write" + } + } + ], + "node_pool": [ + { + "prod": { + "capabilities": ["read"] + } + }, + { + "dev": { + "policy": "write" + } + } + ], + "agent": { + "policy": "read" + }, + "node": { + "policy": "write" + }, + "operator": { + "policy": "deny" + }, + "quota": { + "policy": "read" + }, + "plugin": { + "policy": "read" + } + }`, + "", + &Policy{ + Namespaces: []*NamespacePolicy{ + { + Name: "default", + Policy: PolicyRead, + Capabilities: []string{ + NamespaceCapabilityListJobs, + NamespaceCapabilityParseJob, + NamespaceCapabilityReadJob, + NamespaceCapabilityCSIListVolume, + NamespaceCapabilityCSIReadVolume, + NamespaceCapabilityReadJobScaling, + NamespaceCapabilityListScalingPolicies, + NamespaceCapabilityReadScalingPolicy, + }, + }, + { + Name: "other", + Policy: PolicyWrite, + Capabilities: []string{ + NamespaceCapabilityListJobs, + NamespaceCapabilityParseJob, + NamespaceCapabilityReadJob, + NamespaceCapabilityCSIListVolume, + NamespaceCapabilityCSIReadVolume, + NamespaceCapabilityReadJobScaling, + NamespaceCapabilityListScalingPolicies, + NamespaceCapabilityReadScalingPolicy, + NamespaceCapabilityScaleJob, + NamespaceCapabilitySubmitJob, + NamespaceCapabilityDispatchJob, + NamespaceCapabilityReadLogs, + NamespaceCapabilityReadFS, + NamespaceCapabilityAllocExec, + NamespaceCapabilityAllocLifecycle, + NamespaceCapabilityCSIMountVolume, + NamespaceCapabilityCSIWriteVolume, + NamespaceCapabilitySubmitRecommendation, + }, + }, + { + Name: "secret", + Capabilities: []string{ + NamespaceCapabilityDeny, + NamespaceCapabilityReadLogs, + }, + }, + { + Name: "apps", + Variables: &VariablesPolicy{ + Paths: []*VariablesPathPolicy{ + { + PathSpec: "jobs/write-does-not-imply-read-or-delete", + Capabilities: []string{VariablesCapabilityWrite}, + }, + { + PathSpec: "project/read-implies-list", + Capabilities: []string{ + VariablesCapabilityRead, + VariablesCapabilityList, + }, + }, + { + PathSpec: "project/explicit", + Capabilities: []string{ + VariablesCapabilityRead, + VariablesCapabilityList, + VariablesCapabilityDestroy, + }, + }, + }, + }, + }, + { + Name: "autoscaler", + Policy: PolicyScale, + Capabilities: []string{ + NamespaceCapabilityListScalingPolicies, + NamespaceCapabilityReadScalingPolicy, + NamespaceCapabilityReadJobScaling, + NamespaceCapabilityScaleJob, + }, + }, + }, + HostVolumes: []*HostVolumePolicy{ + { + Name: "production-tls-*", + Capabilities: []string{"mount-readonly"}, + }, + { + Name: "staging-tls-*", + Policy: "write", + Capabilities: []string{ + "mount-readonly", + "mount-readwrite", + }, + }, + }, + NodePools: []*NodePoolPolicy{ + { + Name: "prod", + Capabilities: []string{"read"}, + }, + { + Name: "dev", + Policy: "write", + Capabilities: []string{"delete", "read", "write"}, + }, + }, Agent: &AgentPolicy{ Policy: PolicyRead, }, @@ -201,6 +458,30 @@ func TestParse(t *testing.T) { "Invalid namespace policy", nil, }, + { + ` + namespace { + policy = "read" + } + `, + "Invalid namespace name", + nil, + }, + { + ` + { + "namespace": [ + { + "": { + "policy": "read" + } + } + ] + } + `, + "Invalid namespace name", + nil, + }, { ` namespace "dev" { @@ -212,6 +493,48 @@ func TestParse(t *testing.T) { "Invalid variable policy: no variable paths in namespace dev", nil, }, + { + ` + namespace "dev" { + policy = "read" + + variables { + path {} + path "nomad/jobs/example" { + capabilities = ["read"] + } + } + } + `, + "Invalid missing variable path in namespace", + nil, + }, + { + ` + { + "namespace": [ + { + "dev": { + "policy": "read", + "variables": [ + { + "paths": [ + { + "": { + "capabilities": ["read"] + } + } + ] + ] + ] + } + } + ] + } + `, + "no variable paths in namespace dev", + nil, + }, { ` namespace "default" { @@ -221,6 +544,11 @@ func TestParse(t *testing.T) { "Invalid namespace capability", nil, }, + { + `namespace {}`, + "invalid acl policy", + nil, + }, { ` agent { @@ -416,6 +744,30 @@ func TestParse(t *testing.T) { "Invalid node pool capability", nil, }, + { + ` + node_pool { + policy = "read" + } + `, + "Invalid node pool name", + nil, + }, + { + ` + { + "node_pool": [ + { + "": { + "policy": "read" + } + } + ] + } + `, + "Invalid node pool name", + nil, + }, { ` host_volume "production-tls-*" { @@ -463,6 +815,30 @@ func TestParse(t *testing.T) { "Invalid host volume name", nil, }, + { + ` + host_volume { + policy = "read" + } + `, + "Invalid host volume name", + nil, + }, + { + ` + { + "host_volume": [ + { + "": { + "policy": "read" + } + } + ] + } + `, + "Invalid host volume name", + nil, + }, { ` plugin { diff --git a/website/content/docs/upgrade/upgrade-specific.mdx b/website/content/docs/upgrade/upgrade-specific.mdx index bf99c8bed..8fe6ae1a0 100644 --- a/website/content/docs/upgrade/upgrade-specific.mdx +++ b/website/content/docs/upgrade/upgrade-specific.mdx @@ -77,6 +77,33 @@ should be updated prior to upgrading to Nomad v1.6. See [#17780](https://github.com/hashicorp/nomad/issues/17780) for details. +#### Namespace ACL policies require a label + +Nomad 1.6.0 does not allow ACL policies for namespaces without a label. Prior +to this version, ACL policies for namespaces were allowed to be defined +without a label, and the documented behavior in this case was that the policy +would be applied to the `default` namespace. + +A bug in this logic caused the policy to be incorrectly applied to a different +namespace. For example, the policy below would be applied to a namespace called +`policy` instead of `default`. + +```hcl +namespace { + policy = "read" +} +``` + +To avoid further confusion and potential security incidents, this functionality +was removed and now all namespace policies are required to have a label. + +Tokens currently attached to an invalid policy will stop working after the +upgrade, so you should fix invalid policies to have an explicit namespace label +before upgrading Nomad. + +After the policies are fixed, the existing tokens with those policies will +continue to work and do not need to be regenerated. + #### Command `nomad tls cert create` flag `-cluster-region` deprecated Nomad 1.6.0 will deprecate the command `nomad tls cert create` flag `-cluster-region` @@ -96,6 +123,35 @@ tested and may include bugs around platform-specific integer sizes. Using 64-bit builds for small form-factor hosts such as the RaspberryPi is strongly recommended. +## Nomad 1.5.7, 1.4.11 + +#### Namespace ACL policies require a label + +Nomad 1.5.7 and 1.4.11 do not allow ACL policies for namespaces without a +label. Prior to these versions, ACL policies for namespaces were allowed to be +defined without a label, and the documented behavior in this case was that the +policy would be applied to the `default` namespace. + +A bug in this logic caused the policy to be incorrectly applied to a different +namespace. For example, the policy below would be applied to a namespace called +`policy` instead of `default`. + +```hcl +namespace { + policy = "read" +} +``` + +To avoid further confusion and potential security incidents, this functionality +was removed and now all namespace policies are required to have a label. + +Tokens currently attached to an invalid policy will stop working after the +upgrade, so you should fix invalid policies to have an explicit namespace label +before upgrading Nomad. + +After the policies are fixed, the existing tokens with those policies will +continue to work and do not need to be regenerated. + ## Nomad 1.5.5 Nomad 1.5.5 fixed a bug where allocations that are rescheduled for jobs