diff --git a/.changelog/14577.txt b/.changelog/14577.txt new file mode 100644 index 000000000..022cdf011 --- /dev/null +++ b/.changelog/14577.txt @@ -0,0 +1,3 @@ +```release-note:security +auto-config: Added input validation for auto-config JWT authorization checks. Prior to this change, it was possible for malicious actors to construct requests which incorrectly pass custom JWT claim validation for the `AutoConfig.InitialConfiguration` endpoint. Now, only a subset of characters are allowed for the input before evaluating the bexpr. +``` \ No newline at end of file diff --git a/agent/consul/auto_config_endpoint.go b/agent/consul/auto_config_endpoint.go index 088c9a3e0..32b2b6a53 100644 --- a/agent/consul/auto_config_endpoint.go +++ b/agent/consul/auto_config_endpoint.go @@ -5,6 +5,7 @@ import ( "crypto/x509" "encoding/base64" "fmt" + "regexp" "github.com/hashicorp/consul/acl" @@ -12,6 +13,7 @@ import ( "github.com/hashicorp/consul/agent/connect" "github.com/hashicorp/consul/agent/consul/authmethod/ssoauth" + "github.com/hashicorp/consul/agent/dns" "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/lib/template" "github.com/hashicorp/consul/proto/pbautoconf" @@ -51,6 +53,11 @@ type jwtAuthorizer struct { claimAssertions []string } +// Invalidate any quote or whitespace characters that could cause an escape with bexpr. +// This includes an extra single-quote character not specified in the grammar for safety in case it is later added. +// https://github.com/hashicorp/go-bexpr/blob/v0.1.11/grammar/grammar.peg#L188-L191 +var invalidSegmentName = regexp.MustCompile("[`'\"\\s]+") + func (a *jwtAuthorizer) Authorize(req *pbautoconf.AutoConfigRequest) (AutoConfigOptions, error) { // perform basic JWT Authorization identity, err := a.validator.ValidateLogin(context.Background(), req.JWT) @@ -59,6 +66,21 @@ func (a *jwtAuthorizer) Authorize(req *pbautoconf.AutoConfigRequest) (AutoConfig return AutoConfigOptions{}, acl.PermissionDenied("Failed JWT authorization: %v", err) } + // Ensure provided data cannot escape the RHS of a bexpr for security. + // This is not the cleanest way to prevent this behavior. Ideally, the bexpr would allow us to + // inject a variable on the RHS for comparison as well, but it would be a complex change to implement + // that would likely break backwards-compatibility in certain circumstances. + if dns.InvalidNameRe.MatchString(req.Node) { + return AutoConfigOptions{}, fmt.Errorf("Invalid request field. %v = `%v`", "node", req.Node) + } + if invalidSegmentName.MatchString(req.Segment) { + return AutoConfigOptions{}, fmt.Errorf("Invalid request field. %v = `%v`", "segment", req.Segment) + } + if req.Partition != "" && !dns.IsValidLabel(req.Partition) { + return AutoConfigOptions{}, fmt.Errorf("Invalid request field. %v = `%v`", "partition", req.Partition) + } + + // Ensure that every value in this mapping is safe to interpolate before using it. varMap := map[string]string{ "node": req.Node, "segment": req.Segment, diff --git a/agent/consul/auto_config_endpoint_test.go b/agent/consul/auto_config_endpoint_test.go index 43df5fdab..782e3cdb4 100644 --- a/agent/consul/auto_config_endpoint_test.go +++ b/agent/consul/auto_config_endpoint_test.go @@ -92,9 +92,9 @@ func signJWTWithStandardClaims(t *testing.T, privKey string, claims interface{}) // TestAutoConfigInitialConfiguration is really an integration test of all the moving parts of the AutoConfig.InitialConfiguration RPC. // Full testing of the individual parts will not be done in this test: // -// * Any implementations of the AutoConfigAuthorizer interface (although these test do use the jwtAuthorizer) -// * Each of the individual config generation functions. These can be unit tested separately and should NOT -// require running test servers +// - Any implementations of the AutoConfigAuthorizer interface (although these test do use the jwtAuthorizer) +// - Each of the individual config generation functions. These can be unit tested separately and should NOT +// require running test servers func TestAutoConfigInitialConfiguration(t *testing.T) { if testing.Short() { t.Skip("too slow for testing.Short") @@ -236,6 +236,29 @@ func TestAutoConfigInitialConfiguration(t *testing.T) { }, err: "Permission denied: Failed JWT authorization: no known key successfully validated the token signature", }, + "bad-req-node": { + request: &pbautoconf.AutoConfigRequest{ + Node: "bad node", + JWT: signJWTWithStandardClaims(t, priv, map[string]interface{}{"consul_node_name": "test-node"}), + }, + err: "Invalid request field. node =", + }, + "bad-req-segment": { + request: &pbautoconf.AutoConfigRequest{ + Node: "test-node", + Segment: "bad segment", + JWT: signJWTWithStandardClaims(t, priv, map[string]interface{}{"consul_node_name": "test-node"}), + }, + err: "Invalid request field. segment =", + }, + "bad-req-partition": { + request: &pbautoconf.AutoConfigRequest{ + Node: "test-node", + Partition: "bad partition", + JWT: signJWTWithStandardClaims(t, priv, map[string]interface{}{"consul_node_name": "test-node"}), + }, + err: "Invalid request field. partition =", + }, "claim-assertion-failed": { request: &pbautoconf.AutoConfigRequest{ Node: "test-node", @@ -850,3 +873,39 @@ func TestAutoConfig_updateJoinAddressesInConfig(t *testing.T) { backend.AssertExpectations(t) } + +func TestAutoConfig_invalidSegmentName(t *testing.T) { + invalid := []string{ + "\n", + "\r", + "\t", + "`", + `'`, + `"`, + ` `, + `a b`, + `a'b`, + `a or b`, + `a and b`, + `segment name`, + `segment"name`, + `"segment"name`, + `"segment" name`, + `segment'name'`, + } + valid := []string{ + ``, + `a`, + `a.b`, + `a.b.c`, + `a-b-c`, + `segment.name`, + } + + for _, s := range invalid { + require.True(t, invalidSegmentName.MatchString(s), "incorrect match: %v", s) + } + for _, s := range valid { + require.False(t, invalidSegmentName.MatchString(s), "incorrect match: %v", s) + } +}