From 15f9d54deaa78c42106f79d618d46533114cf928 Mon Sep 17 00:00:00 2001 From: Luiz Aoqui Date: Tue, 1 Feb 2022 18:54:53 -0500 Subject: [PATCH] api: prevent excessice CPU load on job parse Add new namespace ACL requirement for the /v1/jobs/parse endpoint and return early if HCLv2 parsing fails. The endpoint now requires the new `parse-job` ACL capability or `submit-job`. --- .changelog/12038.txt | 3 + acl/policy.go | 4 +- acl/policy_test.go | 3 + command/agent/agent_endpoint.go | 52 ++++-------- command/agent/http.go | 26 ++++++ command/agent/http_test.go | 58 ++++++++++++++ command/agent/job_endpoint.go | 21 ++++- command/agent/job_endpoint_test.go | 123 +++++++++++++++++++++++++++++ jobspec2/parse.go | 6 ++ jobspec2/parse_test.go | 43 ++++++++++ 10 files changed, 301 insertions(+), 38 deletions(-) create mode 100644 .changelog/12038.txt diff --git a/.changelog/12038.txt b/.changelog/12038.txt new file mode 100644 index 000000000..d9fee4620 --- /dev/null +++ b/.changelog/12038.txt @@ -0,0 +1,3 @@ +```release-note:security +Add ACL requirement and HCL validation to the job parse API endpoint to prevent excessive CPU usage. [CVE-2022-24685](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2022-24685) +``` diff --git a/acl/policy.go b/acl/policy.go index 07fa36457..95df4a280 100644 --- a/acl/policy.go +++ b/acl/policy.go @@ -26,6 +26,7 @@ const ( NamespaceCapabilityDeny = "deny" NamespaceCapabilityListJobs = "list-jobs" + NamespaceCapabilityParseJob = "parse-job" NamespaceCapabilityReadJob = "read-job" NamespaceCapabilitySubmitJob = "submit-job" NamespaceCapabilityDispatchJob = "dispatch-job" @@ -146,7 +147,7 @@ func (p *PluginPolicy) isValid() bool { // isNamespaceCapabilityValid ensures the given capability is valid for a namespace policy func isNamespaceCapabilityValid(cap string) bool { switch cap { - case NamespaceCapabilityDeny, NamespaceCapabilityListJobs, NamespaceCapabilityReadJob, + case NamespaceCapabilityDeny, NamespaceCapabilityParseJob, NamespaceCapabilityListJobs, NamespaceCapabilityReadJob, NamespaceCapabilitySubmitJob, NamespaceCapabilityDispatchJob, NamespaceCapabilityReadLogs, NamespaceCapabilityReadFS, NamespaceCapabilityAllocLifecycle, NamespaceCapabilityAllocExec, NamespaceCapabilityAllocNodeExec, @@ -166,6 +167,7 @@ func isNamespaceCapabilityValid(cap string) bool { func expandNamespacePolicy(policy string) []string { read := []string{ NamespaceCapabilityListJobs, + NamespaceCapabilityParseJob, NamespaceCapabilityReadJob, NamespaceCapabilityCSIListVolume, NamespaceCapabilityCSIReadVolume, diff --git a/acl/policy_test.go b/acl/policy_test.go index 60e4615ea..9060147d0 100644 --- a/acl/policy_test.go +++ b/acl/policy_test.go @@ -29,6 +29,7 @@ func TestParse(t *testing.T) { Policy: PolicyRead, Capabilities: []string{ NamespaceCapabilityListJobs, + NamespaceCapabilityParseJob, NamespaceCapabilityReadJob, NamespaceCapabilityCSIListVolume, NamespaceCapabilityCSIReadVolume, @@ -78,6 +79,7 @@ func TestParse(t *testing.T) { Policy: PolicyRead, Capabilities: []string{ NamespaceCapabilityListJobs, + NamespaceCapabilityParseJob, NamespaceCapabilityReadJob, NamespaceCapabilityCSIListVolume, NamespaceCapabilityCSIReadVolume, @@ -91,6 +93,7 @@ func TestParse(t *testing.T) { Policy: PolicyWrite, Capabilities: []string{ NamespaceCapabilityListJobs, + NamespaceCapabilityParseJob, NamespaceCapabilityReadJob, NamespaceCapabilityCSIListVolume, NamespaceCapabilityCSIReadVolume, diff --git a/command/agent/agent_endpoint.go b/command/agent/agent_endpoint.go index 448b158f7..4c4072802 100644 --- a/command/agent/agent_endpoint.go +++ b/command/agent/agent_endpoint.go @@ -16,7 +16,6 @@ import ( "github.com/docker/docker/pkg/ioutils" log "github.com/hashicorp/go-hclog" "github.com/hashicorp/go-msgpack/codec" - "github.com/hashicorp/nomad/acl" "github.com/hashicorp/nomad/api" cstructs "github.com/hashicorp/nomad/client/structs" "github.com/hashicorp/nomad/command/agent/host" @@ -62,24 +61,7 @@ func (s *HTTPServer) AgentSelfRequest(resp http.ResponseWriter, req *http.Reques return nil, CodedError(405, ErrInvalidMethod) } - var secret string - s.parseToken(req, &secret) - - var aclObj *acl.ACL - var err error - - // Get the member as a server - var member serf.Member - if srv := s.agent.Server(); srv != nil { - member = srv.LocalMember() - aclObj, err = srv.ResolveToken(secret) - } else { - // Not a Server, so use the Client for token resolution. Note - // this gets forwarded to a server with AllowStale = true if - // the local ACL cache TTL has expired (30s by default) - aclObj, err = s.agent.Client().ResolveToken(secret) - } - + aclObj, err := s.ResolveToken(req) if err != nil { return nil, err } @@ -89,6 +71,12 @@ func (s *HTTPServer) AgentSelfRequest(resp http.ResponseWriter, req *http.Reques return nil, structs.ErrPermissionDenied } + // Get the member as a server + var member serf.Member + if srv := s.agent.Server(); srv != nil { + member = srv.LocalMember() + } + self := agentSelf{ Member: nomadMember(member), Stats: s.agent.Stats(), @@ -671,27 +659,19 @@ func (s *HTTPServer) AgentHostRequest(resp http.ResponseWriter, req *http.Reques return nil, CodedError(405, ErrInvalidMethod) } - var secret string - s.parseToken(req, &secret) - - // Check agent read permissions - var aclObj *acl.ACL - var enableDebug bool - var err error - if srv := s.agent.Server(); srv != nil { - aclObj, err = srv.ResolveToken(secret) - enableDebug = srv.GetConfig().EnableDebug - } else { - // Not a Server, so use the Client for token resolution. Note - // this gets forwarded to a server with AllowStale = true if - // the local ACL cache TTL has expired (30s by default) - aclObj, err = s.agent.Client().ResolveToken(secret) - enableDebug = s.agent.Client().GetConfig().EnableDebug - } + aclObj, err := s.ResolveToken(req) if err != nil { return nil, err } + // Check agent read permissions + var enableDebug bool + if srv := s.agent.Server(); srv != nil { + enableDebug = srv.GetConfig().EnableDebug + } else { + enableDebug = s.agent.Client().GetConfig().EnableDebug + } + if (aclObj != nil && !aclObj.AllowAgentRead()) || (aclObj == nil && !enableDebug) { return nil, structs.ErrPermissionDenied diff --git a/command/agent/http.go b/command/agent/http.go index a3c082661..c054d5096 100644 --- a/command/agent/http.go +++ b/command/agent/http.go @@ -23,6 +23,7 @@ import ( multierror "github.com/hashicorp/go-multierror" "github.com/rs/cors" + "github.com/hashicorp/nomad/acl" "github.com/hashicorp/nomad/helper/noxssrw" "github.com/hashicorp/nomad/helper/tlsutil" "github.com/hashicorp/nomad/nomad/structs" @@ -270,6 +271,31 @@ func (s *HTTPServer) Shutdown() { } } +// ResolveToken extracts the ACL token secret ID from the request and +// translates it into an ACL object. Returns nil if ACLs are disabled. +func (s *HTTPServer) ResolveToken(req *http.Request) (*acl.ACL, error) { + var secret string + s.parseToken(req, &secret) + + var aclObj *acl.ACL + var err error + + if srv := s.agent.Server(); srv != nil { + aclObj, err = srv.ResolveToken(secret) + } else { + // Not a Server, so use the Client for token resolution. Note + // this gets forwarded to a server with AllowStale = true if + // the local ACL cache TTL has expired (30s by default) + aclObj, err = s.agent.Client().ResolveToken(secret) + } + + if err != nil { + return nil, fmt.Errorf("failed to resolve ACL token: %v", err) + } + + return aclObj, nil +} + // registerHandlers is used to attach our handlers to the mux func (s HTTPServer) registerHandlers(enableDebug bool) { s.mux.HandleFunc("/v1/jobs", s.wrap(s.JobsRequest)) diff --git a/command/agent/http_test.go b/command/agent/http_test.go index 596c3a58c..dd1521387 100644 --- a/command/agent/http_test.go +++ b/command/agent/http_test.go @@ -23,6 +23,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/hashicorp/nomad/acl" "github.com/hashicorp/nomad/api" "github.com/hashicorp/nomad/helper" "github.com/hashicorp/nomad/helper/testlog" @@ -1315,6 +1316,57 @@ func TestHTTPServer_Limits_OK(t *testing.T) { } } +func TestHTTPServer_ResolveToken(t *testing.T) { + t.Parallel() + + // Setup two servers, one with ACL enabled and another with ACL disabled. + noACLServer := makeHTTPServer(t, func(c *Config) { + c.ACL = &ACLConfig{Enabled: false} + }) + defer noACLServer.Shutdown() + + ACLServer := makeHTTPServer(t, func(c *Config) { + c.ACL = &ACLConfig{Enabled: true} + }) + defer ACLServer.Shutdown() + + // Register sample token. + state := ACLServer.Agent.server.State() + token := mock.CreatePolicyAndToken(t, state, 1000, "node", mock.NodePolicy(acl.PolicyWrite)) + + // Tests cases. + t.Run("acl disabled", func(t *testing.T) { + req := &http.Request{Body: http.NoBody} + got, err := noACLServer.Server.ResolveToken(req) + require.NoError(t, err) + require.Nil(t, got) + }) + + t.Run("token not found", func(t *testing.T) { + req := &http.Request{ + Body: http.NoBody, + Header: make(map[string][]string), + } + setToken(req, mock.ACLToken()) + got, err := ACLServer.Server.ResolveToken(req) + require.Nil(t, got) + require.Error(t, err) + require.Contains(t, err.Error(), "ACL token not found") + }) + + t.Run("set token", func(t *testing.T) { + req := &http.Request{ + Body: http.NoBody, + Header: make(map[string][]string), + } + setToken(req, token) + got, err := ACLServer.Server.ResolveToken(req) + require.NoError(t, err) + require.NotNil(t, got) + require.True(t, got.AllowNodeWrite()) + }) +} + func Test_IsAPIClientError(t *testing.T) { trueCases := []int{400, 403, 404, 499} for _, c := range trueCases { @@ -1410,6 +1462,12 @@ func setToken(req *http.Request, token *structs.ACLToken) { req.Header.Set("X-Nomad-Token", token.SecretID) } +func setNamespace(req *http.Request, ns string) { + q := req.URL.Query() + q.Add("namespace", ns) + req.URL.RawQuery = q.Encode() +} + func encodeReq(obj interface{}) io.ReadCloser { buf := bytes.NewBuffer(nil) enc := json.NewEncoder(buf) diff --git a/command/agent/job_endpoint.go b/command/agent/job_endpoint.go index b3a4b6bd2..6576cefbe 100644 --- a/command/agent/job_endpoint.go +++ b/command/agent/job_endpoint.go @@ -7,6 +7,7 @@ import ( "strings" "github.com/golang/snappy" + "github.com/hashicorp/nomad/acl" api "github.com/hashicorp/nomad/api" "github.com/hashicorp/nomad/helper" "github.com/hashicorp/nomad/jobspec" @@ -703,6 +704,25 @@ func (s *HTTPServer) JobsParseRequest(resp http.ResponseWriter, req *http.Reques return nil, CodedError(405, ErrInvalidMethod) } + var namespace string + parseNamespace(req, &namespace) + + aclObj, err := s.ResolveToken(req) + if err != nil { + return nil, err + } + + // Check job parse permissions + if aclObj != nil { + hasParseJob := aclObj.AllowNsOp(namespace, acl.NamespaceCapabilityParseJob) + hasSubmitJob := aclObj.AllowNsOp(namespace, acl.NamespaceCapabilitySubmitJob) + + allowed := hasParseJob || hasSubmitJob + if !allowed { + return nil, structs.ErrPermissionDenied + } + } + args := &api.JobsParseRequest{} if err := decodeBody(req, &args); err != nil { return nil, CodedError(400, err.Error()) @@ -712,7 +732,6 @@ func (s *HTTPServer) JobsParseRequest(resp http.ResponseWriter, req *http.Reques } var jobStruct *api.Job - var err error if args.HCLv1 { jobStruct, err = jobspec.Parse(strings.NewReader(args.JobHCL)) } else { diff --git a/command/agent/job_endpoint_test.go b/command/agent/job_endpoint_test.go index d0c8a8690..ff4d615a6 100644 --- a/command/agent/job_endpoint_test.go +++ b/command/agent/job_endpoint_test.go @@ -12,6 +12,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/hashicorp/nomad/acl" api "github.com/hashicorp/nomad/api" "github.com/hashicorp/nomad/helper" "github.com/hashicorp/nomad/nomad/mock" @@ -407,6 +408,128 @@ func TestHTTP_JobsParse(t *testing.T) { } }) } + +func TestHTTP_JobsParse_ACL(t *testing.T) { + t.Parallel() + + httpACLTest(t, nil, func(s *TestAgent) { + state := s.Agent.server.State() + + // ACL tokens used in tests. + nodeToken := mock.CreatePolicyAndToken( + t, state, 1000, "node", + mock.NodePolicy(acl.PolicyWrite), + ) + parseJobDevToken := mock.CreatePolicyAndToken( + t, state, 1002, "parse-job-dev", + mock.NamespacePolicy("dev", "", []string{"parse-job"}), + ) + readNsDevToken := mock.CreatePolicyAndToken( + t, state, 1004, "read-dev", + mock.NamespacePolicy("dev", "read", nil), + ) + parseJobDefaultToken := mock.CreatePolicyAndToken( + t, state, 1006, "parse-job-default", + mock.NamespacePolicy("default", "", []string{"parse-job"}), + ) + submitJobDefaultToken := mock.CreatePolicyAndToken( + t, state, 1008, "submit-job-default", + mock.NamespacePolicy("default", "", []string{"submit-job"}), + ) + readNsDefaultToken := mock.CreatePolicyAndToken( + t, state, 1010, "read-default", + mock.NamespacePolicy("default", "read", nil), + ) + + testCases := []struct { + name string + token *structs.ACLToken + namespace string + expectError bool + }{ + { + name: "missing ACL token", + token: nil, + expectError: true, + }, + { + name: "wrong permissions", + token: nodeToken, + expectError: true, + }, + { + name: "wrong namespace", + token: readNsDevToken, + expectError: true, + }, + { + name: "wrong namespace capability", + token: parseJobDevToken, + expectError: true, + }, + { + name: "default namespace read", + token: readNsDefaultToken, + expectError: false, + }, + { + name: "non-default namespace read", + token: readNsDevToken, + namespace: "dev", + expectError: false, + }, + { + name: "default namespace parse-job capability", + token: parseJobDefaultToken, + expectError: false, + }, + { + name: "default namespace submit-job capability", + token: submitJobDefaultToken, + expectError: false, + }, + { + name: "non-default namespace capability", + token: parseJobDevToken, + namespace: "dev", + expectError: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + buf := encodeReq(api.JobsParseRequest{JobHCL: mock.HCL()}) + req, err := http.NewRequest("POST", "/v1/jobs/parse", buf) + require.NoError(t, err) + + if tc.namespace != "" { + setNamespace(req, tc.namespace) + } + + if tc.token != nil { + setToken(req, tc.token) + } + + respW := httptest.NewRecorder() + obj, err := s.Server.JobsParseRequest(respW, req) + + if tc.expectError { + require.Error(t, err) + require.Equal(t, structs.ErrPermissionDenied.Error(), err.Error()) + } else { + require.NoError(t, err) + require.NotNil(t, obj) + + job := obj.(*api.Job) + expected := mock.Job() + require.Equal(t, expected.Name, *job.Name) + require.ElementsMatch(t, expected.Datacenters, job.Datacenters) + } + }) + } + }) +} + func TestHTTP_JobQuery(t *testing.T) { t.Parallel() httpTest(t, nil, func(s *TestAgent) { diff --git a/jobspec2/parse.go b/jobspec2/parse.go index 1a5599fac..1febab617 100644 --- a/jobspec2/parse.go +++ b/jobspec2/parse.go @@ -96,6 +96,12 @@ func decode(c *jobConfig) error { diags = append(diags, ds...) } + // Return early if the input job or variable files are not valid. + // Decoding and evaluating invalid files may result in unexpected results. + if diags.HasErrors() { + return diags + } + diags = append(diags, c.decodeBody(file.Body)...) if diags.HasErrors() { diff --git a/jobspec2/parse_test.go b/jobspec2/parse_test.go index 7457b3b02..2cb0496a9 100644 --- a/jobspec2/parse_test.go +++ b/jobspec2/parse_test.go @@ -374,6 +374,49 @@ job "example" { require.Equal(t, "3", out.TaskGroups[2].Tasks[0].Meta["VERSION"]) } +func TestParse_InvalidHCL(t *testing.T) { + t.Run("invalid body", func(t *testing.T) { + hcl := `invalid{hcl` + + _, err := ParseWithConfig(&ParseConfig{ + Path: "input.hcl", + Body: []byte(hcl), + ArgVars: []string{}, + AllowFS: true, + }) + require.Error(t, err) + }) + + t.Run("invalid vars file", func(t *testing.T) { + tmp, err := ioutil.TempFile("", "nomad-jobspec2-") + require.NoError(t, err) + defer os.Remove(tmp.Name()) + + vars := `invalid{hcl` + _, err = tmp.Write([]byte(vars)) + require.NoError(t, err) + + hcl := ` +variables { + region_var = "default" +} +job "example" { + datacenters = [for s in ["dc1", "dc2"] : upper(s)] + region = var.region_var +} +` + + _, err = ParseWithConfig(&ParseConfig{ + Path: "input.hcl", + Body: []byte(hcl), + VarFiles: []string{tmp.Name()}, + ArgVars: []string{}, + AllowFS: true, + }) + require.Error(t, err) + }) +} + func TestParse_InvalidScalingSyntax(t *testing.T) { cases := []struct { name string