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