From bc530a6d403f54dcc25fb67b69c4e907444b5201 Mon Sep 17 00:00:00 2001
From: Tim Gross <tgross@hashicorp.com>
Date: Fri, 31 Mar 2023 11:40:21 -0400
Subject: [PATCH] acl: fix ACL bypass for anon requests that pass thru client
 HTTP

Requests without an ACL token that pass thru the client's HTTP API are treated
as though they come from the client itself. This allows bypass of ACLs on RPC
requests where ACL permissions are checked (like `Job.Register`). Invalid tokens
are correctly rejected.

Fix the bypass by only setting a client ID on the identity if we have a valid node secret.

Note that this changeset will break rate metrics for RPCs sent by clients
without a client secret such as `Node.GetClientAllocs`; these requests will be
recorded as anonymous.

Future work should:
* Ensure the node secret is sent with all client-driven RPCs except
  `Node.Register` which is TOFU.
* Create a new `acl.ACL` object from client requests so that we
  can enforce ACLs for all endpoints in a uniform way that's less error-prone.~
---
 .changelog/16775.txt | 3 +++
 nomad/acl.go         | 9 +++------
 nomad/acl_test.go    | 7 +++++++
 3 files changed, 13 insertions(+), 6 deletions(-)
 create mode 100644 .changelog/16775.txt

diff --git a/.changelog/16775.txt b/.changelog/16775.txt
new file mode 100644
index 000000000..87a376717
--- /dev/null
+++ b/.changelog/16775.txt
@@ -0,0 +1,3 @@
+```release-note:security
+acl: Fixed a bug where unauthenticated HTTP API requests through the client could bypass ACL policy checking [CVE-2023-1782](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2023-1782) [[GH-16775](https://github.com/hashicorp/nomad/issues/16775)]
+```
diff --git a/nomad/acl.go b/nomad/acl.go
index f2a7a5058..4748f4ba3 100644
--- a/nomad/acl.go
+++ b/nomad/acl.go
@@ -102,12 +102,9 @@ func (s *Server) Authenticate(ctx *RPCContext, args structs.RequestWithIdentity)
 
 	// At this point we either have an anonymous token or an invalid one.
 
-	// Previously-connected clients will have a NodeID set on the context, which
-	// is available for all yamux streams over the same yamux session (and TCP
-	// connection). This will be a large portion of the RPCs sent, but we can't
-	// fast-path this at the top of the method, because authenticated HTTP
-	// requests to the clients will come in over to the same session.
-	if ctx.NodeID != "" {
+	// TODO(tgross): remove this entirely in 1.6.0 and enforce that all RPCs
+	// driven by the clients have secret IDs set
+	if ctx.NodeID != "" && secretID != "" {
 		args.SetIdentity(&structs.AuthenticatedIdentity{ClientID: ctx.NodeID})
 		return nil
 	}
diff --git a/nomad/acl_test.go b/nomad/acl_test.go
index a3c4e3d64..a350bc2bd 100644
--- a/nomad/acl_test.go
+++ b/nomad/acl_test.go
@@ -217,6 +217,13 @@ func TestAuthenticate_mTLS(t *testing.T) {
 			expectClientID: node.ID,
 			expectIDKey:    fmt.Sprintf("client:%s", node.ID),
 		},
+		{
+			name:           "from client missing secret", // ex. Node.Register
+			tlsCfg:         clientTLSCfg,
+			expectAccessor: "anonymous",
+			expectTLSName:  "regionFoo.nomad",
+			expectIP:       follower.GetConfig().RPCAddr.IP.String(),
+		},
 		{
 			name:      "from failed workload", // ex. Variables.List
 			tlsCfg:    clientTLSCfg,