Remove some usage of md5 from the system (#11491)

* Remove some usage of md5 from the system

OSS side of https://github.com/hashicorp/consul-enterprise/pull/1253

This is a potential security issue because an attacker could conceivably manipulate inputs to cause persistence files to collide, effectively deleting the persistence file for one of the colliding elements.

Signed-off-by: Mark Anderson <manderson@hashicorp.com>

.changelog/_1253.txt

@@ -0,0 +1,3 @@
+```release-note:security
+agent: Use SHA256 instead of MD5 to generate persistence file names.
+```

agent/agent.go

@@ -1775,10 +1775,14 @@ type persistedService struct {
 	LocallyRegisteredAsSidecar bool `json:",omitempty"`
 }
 
+func (a *Agent) makeServiceFilePath(svcID structs.ServiceID) string {
+	return filepath.Join(a.config.DataDir, servicesDir, svcID.StringHashSHA256())
+}
+
 // persistService saves a service definition to a JSON file in the data dir
 func (a *Agent) persistService(service *structs.NodeService, source configSource) error {
 	svcID := service.CompoundServiceID()
-	svcPath := filepath.Join(a.config.DataDir, servicesDir, svcID.StringHash())
+	svcPath := a.makeServiceFilePath(svcID)
 
 	wrapped := persistedService{
 		Token:                      a.State.ServiceToken(service.CompoundServiceID()),
@@ -1796,7 +1800,7 @@ func (a *Agent) persistService(service *structs.NodeService, source configSource
 
 // purgeService removes a persisted service definition file from the data dir
 func (a *Agent) purgeService(serviceID structs.ServiceID) error {
-	svcPath := filepath.Join(a.config.DataDir, servicesDir, serviceID.StringHash())
+	svcPath := a.makeServiceFilePath(serviceID)
 	if _, err := os.Stat(svcPath); err == nil {
 		return os.Remove(svcPath)
 	}
@@ -1806,7 +1810,7 @@ func (a *Agent) purgeService(serviceID structs.ServiceID) error {
 // persistCheck saves a check definition to the local agent's state directory
 func (a *Agent) persistCheck(check *structs.HealthCheck, chkType *structs.CheckType, source configSource) error {
 	cid := check.CompoundCheckID()
-	checkPath := filepath.Join(a.config.DataDir, checksDir, cid.StringHash())
+	checkPath := filepath.Join(a.config.DataDir, checksDir, cid.StringHashSHA256())
 
 	// Create the persisted check
 	wrapped := persistedCheck{
@@ -1826,7 +1830,7 @@ func (a *Agent) persistCheck(check *structs.HealthCheck, chkType *structs.CheckT
 
 // purgeCheck removes a persisted check definition file from the data dir
 func (a *Agent) purgeCheck(checkID structs.CheckID) error {
-	checkPath := filepath.Join(a.config.DataDir, checksDir, checkID.StringHash())
+	checkPath := filepath.Join(a.config.DataDir, checksDir, checkID.StringHashSHA256())
 	if _, err := os.Stat(checkPath); err == nil {
 		return os.Remove(checkPath)
 	}
@@ -1842,6 +1846,10 @@ type persistedServiceConfig struct {
 	structs.EnterpriseMeta
 }
 
+func (a *Agent) makeServiceConfigFilePath(serviceID structs.ServiceID) string {
+	return filepath.Join(a.config.DataDir, serviceConfigDir, serviceID.StringHashSHA256())
+}
+
 func (a *Agent) persistServiceConfig(serviceID structs.ServiceID, defaults *structs.ServiceConfigResponse) error {
 	// Create the persisted config.
 	wrapped := persistedServiceConfig{
@@ -1856,7 +1864,7 @@ func (a *Agent) persistServiceConfig(serviceID structs.ServiceID, defaults *stru
 	}
 
 	dir := filepath.Join(a.config.DataDir, serviceConfigDir)
-	configPath := filepath.Join(dir, serviceID.StringHash())
+	configPath := a.makeServiceConfigFilePath(serviceID)
 
 	// Create the config dir if it doesn't exist
 	if err := os.MkdirAll(dir, 0700); err != nil {
@@ -1867,7 +1875,7 @@ func (a *Agent) persistServiceConfig(serviceID structs.ServiceID, defaults *stru
 }
 
 func (a *Agent) purgeServiceConfig(serviceID structs.ServiceID) error {
-	configPath := filepath.Join(a.config.DataDir, serviceConfigDir, serviceID.StringHash())
+	configPath := a.makeServiceConfigFilePath(serviceID)
 	if _, err := os.Stat(configPath); err == nil {
 		return os.Remove(configPath)
 	}
@@ -1914,7 +1922,18 @@ func (a *Agent) readPersistedServiceConfigs() (map[structs.ServiceID]*structs.Se
 			)
 			continue
 		}
-		out[structs.NewServiceID(p.ServiceID, &p.EnterpriseMeta)] = p.Defaults
+
+		serviceID := structs.NewServiceID(p.ServiceID, &p.EnterpriseMeta)
+
+		// Rename files that used the old md5 hash to the new sha256 name; only needed when upgrading from 1.10 and before.
+		newPath := a.makeServiceConfigFilePath(serviceID)
+		if file != newPath {
+			if err := os.Rename(file, newPath); err != nil {
+				a.logger.Error("Failed renaming service config file from %s to %s", file, newPath, err)
+			}
+		}
+
+		out[serviceID] = p.Defaults
 	}
 
 	return out, nil
@@ -2983,7 +3002,7 @@ func (a *Agent) persistCheckState(check *checks.CheckTTL, status, output string)
 	}
 
 	// Write the state to the file
-	file := filepath.Join(dir, check.CheckID.StringHash())
+	file := filepath.Join(dir, check.CheckID.StringHashSHA256())
 
 	// Create temp file in same dir, to make more likely atomic
 	tempFile := file + ".tmp"
@@ -3003,14 +3022,27 @@ func (a *Agent) persistCheckState(check *checks.CheckTTL, status, output string)
 func (a *Agent) loadCheckState(check *structs.HealthCheck) error {
 	cid := check.CompoundCheckID()
 	// Try to read the persisted state for this check
-	file := filepath.Join(a.config.DataDir, checkStateDir, cid.StringHash())
+	file := filepath.Join(a.config.DataDir, checkStateDir, cid.StringHashSHA256())
 	buf, err := ioutil.ReadFile(file)
+	if err != nil {
+		if os.IsNotExist(err) {
+			// try the md5 based name. This can be removed once we no longer support upgrades from versions that use MD5 hashing
+			oldFile := filepath.Join(a.config.DataDir, checkStateDir, cid.StringHashMD5())
+			buf, err = ioutil.ReadFile(oldFile)
 			if err != nil {
 				if os.IsNotExist(err) {
 					return nil
-		}
+				} else {
 					return fmt.Errorf("failed reading file %q: %s", file, err)
 				}
+			}
+			if err := os.Rename(oldFile, file); err != nil {
+				a.logger.Error("Failed renaming service file from %s to %s", oldFile, file, err)
+			}
+		} else {
+			return fmt.Errorf("failed reading file %q: %s", file, err)
+		}
+	}
 
 	// Decode the state data
 	var p persistedCheckState
@@ -3033,7 +3065,7 @@ func (a *Agent) loadCheckState(check *structs.HealthCheck) error {
 
 // purgeCheckState is used to purge the state of a check from the data dir
 func (a *Agent) purgeCheckState(checkID structs.CheckID) error {
-	file := filepath.Join(a.config.DataDir, checkStateDir, checkID.StringHash())
+	file := filepath.Join(a.config.DataDir, checkStateDir, checkID.StringHashSHA256())
 	err := os.Remove(file)
 	if os.IsNotExist(err) {
 		return nil
@@ -3232,6 +3264,14 @@ func (a *Agent) loadServices(conf *config.RuntimeConfig, snap map[structs.CheckI
 			}
 		}
 
+		// Rename files that used the old md5 hash to the new sha256 name; only needed when upgrading from 1.10 and before.
+		newPath := a.makeServiceFilePath(p.Service.CompoundServiceID())
+		if file != newPath {
+			if err := os.Rename(file, newPath); err != nil {
+				a.logger.Error("Failed renaming service file from %s to %s", file, newPath, err)
+			}
+		}
+
 		// Restore LocallyRegisteredAsSidecar, see persistedService.LocallyRegisteredAsSidecar
 		p.Service.LocallyRegisteredAsSidecar = p.LocallyRegisteredAsSidecar
 
@@ -3362,6 +3402,14 @@ func (a *Agent) loadChecks(conf *config.RuntimeConfig, snap map[structs.CheckID]
 		}
 		checkID := p.Check.CompoundCheckID()
 
+		// Rename files that used the old md5 hash to the new sha256 name; only needed when upgrading from 1.10 and before.
+		newPath := filepath.Join(a.config.DataDir, checksDir, checkID.StringHashSHA256())
+		if file != newPath {
+			if err := os.Rename(file, newPath); err != nil {
+				a.logger.Error("Failed renaming service file from %s to %s", file, newPath, err)
+			}
+		}
+
 		source, ok := ConfigSourceFromName(p.Source)
 		if !ok {
 			a.logger.Warn("check exists with invalid source, purging",

agent/agent_test.go

@@ -3,6 +3,7 @@ package agent
 import (
 	"bytes"
 	"context"
+	"crypto/md5"
 	"crypto/tls"
 	"crypto/x509"
 	"encoding/base64"
@@ -2254,7 +2255,7 @@ func testAgent_PersistService(t *testing.T, extraHCL string) {
 		Port:    8000,
 	}
 
-	file := filepath.Join(a.Config.DataDir, servicesDir, stringHash(svc.ID))
+	file := filepath.Join(a.Config.DataDir, servicesDir, structs.NewServiceID(svc.ID, nil).StringHashSHA256())
 
 	// Check is not persisted unless requested
 	if err := a.addServiceFromSource(svc, nil, false, "", ConfigSourceLocal); err != nil {
@@ -2365,7 +2366,7 @@ func testAgent_persistedService_compat(t *testing.T, extraHCL string) {
 	}
 
 	// Write the content to the file
-	file := filepath.Join(a.Config.DataDir, servicesDir, stringHash(svc.ID))
+	file := filepath.Join(a.Config.DataDir, servicesDir, structs.NewServiceID(svc.ID, nil).StringHashSHA256())
 	if err := os.MkdirAll(filepath.Dir(file), 0700); err != nil {
 		t.Fatalf("err: %s", err)
 	}
@@ -2383,6 +2384,85 @@ func testAgent_persistedService_compat(t *testing.T, extraHCL string) {
 	require.Equal(t, svc, result)
 }
 
+func TestAgent_persistedService_compat_hash(t *testing.T) {
+	t.Run("normal", func(t *testing.T) {
+		t.Parallel()
+		testAgent_persistedService_compat_hash(t, "enable_central_service_config = false")
+	})
+	t.Run("service manager", func(t *testing.T) {
+		t.Parallel()
+		testAgent_persistedService_compat_hash(t, "enable_central_service_config = true")
+	})
+
+}
+
+func testAgent_persistedService_compat_hash(t *testing.T, extraHCL string) {
+	t.Helper()
+
+	// Tests backwards compatibility of persisted services from pre-0.5.1
+	a := NewTestAgent(t, extraHCL)
+	defer a.Shutdown()
+
+	svc := &structs.NodeService{
+		ID:              "redis",
+		Service:         "redis",
+		Tags:            []string{"foo"},
+		Port:            8000,
+		TaggedAddresses: map[string]structs.ServiceAddress{},
+		Weights:         &structs.Weights{Passing: 1, Warning: 1},
+		EnterpriseMeta:  *structs.DefaultEnterpriseMetaInDefaultPartition(),
+	}
+
+	// Encode the NodeService directly. This is what previous versions
+	// would serialize to the file (without the wrapper)
+	encoded, err := json.Marshal(svc)
+	if err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	// Write the content to the file using the old md5 based path
+	file := filepath.Join(a.Config.DataDir, servicesDir, stringHashMD5(svc.ID))
+	if err := os.MkdirAll(filepath.Dir(file), 0700); err != nil {
+		t.Fatalf("err: %s", err)
+	}
+	if err := ioutil.WriteFile(file, encoded, 0600); err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	wrapped := persistedServiceConfig{
+		ServiceID:      "redis",
+		Defaults:       &structs.ServiceConfigResponse{},
+		EnterpriseMeta: *structs.DefaultEnterpriseMetaInDefaultPartition(),
+	}
+
+	encodedConfig, err := json.Marshal(wrapped)
+	if err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	configFile := filepath.Join(a.Config.DataDir, serviceConfigDir, stringHashMD5(svc.ID))
+	if err := os.MkdirAll(filepath.Dir(configFile), 0700); err != nil {
+		t.Fatalf("err: %s", err)
+	}
+	if err := ioutil.WriteFile(configFile, encodedConfig, 0600); err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	// Load the services
+	if err := a.loadServices(a.Config, nil); err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	// Ensure the service was restored
+	result := requireServiceExists(t, a, "redis")
+	require.Equal(t, svc, result)
+}
+
+// Exists for backwards compatibility testing
+func stringHashMD5(s string) string {
+	return fmt.Sprintf("%x", md5.Sum([]byte(s)))
+}
+
 func TestAgent_PurgeService(t *testing.T) {
 	if testing.Short() {
 		t.Skip("too slow for testing.Short")
@@ -2411,7 +2491,7 @@ func testAgent_PurgeService(t *testing.T, extraHCL string) {
 		Port:    8000,
 	}
 
-	file := filepath.Join(a.Config.DataDir, servicesDir, stringHash(svc.ID))
+	file := filepath.Join(a.Config.DataDir, servicesDir, structs.NewServiceID(svc.ID, nil).StringHashSHA256())
 	if err := a.addServiceFromSource(svc, nil, true, "", ConfigSourceLocal); err != nil {
 		t.Fatalf("err: %v", err)
 	}
@@ -2491,7 +2571,7 @@ func testAgent_PurgeServiceOnDuplicate(t *testing.T, extraHCL string) {
 	defer a2.Shutdown()
 
 	sid := svc1.CompoundServiceID()
-	file := filepath.Join(a.Config.DataDir, servicesDir, sid.StringHash())
+	file := filepath.Join(a.Config.DataDir, servicesDir, sid.StringHashSHA256())
 	_, err := os.Stat(file)
 	require.Error(t, err, "should have removed persisted service")
 	result := requireServiceExists(t, a, "redis")
@@ -2525,7 +2605,7 @@ func TestAgent_PersistCheck(t *testing.T) {
 	}
 
 	cid := check.CompoundCheckID()
-	file := filepath.Join(a.Config.DataDir, checksDir, cid.StringHash())
+	file := filepath.Join(a.Config.DataDir, checksDir, cid.StringHashSHA256())
 
 	// Not persisted if not requested
 	require.NoError(t, a.AddCheck(check, chkType, false, "", ConfigSourceLocal))
@@ -2671,7 +2751,7 @@ func TestAgent_PurgeCheckOnDuplicate(t *testing.T) {
 	defer a2.Shutdown()
 
 	cid := check1.CompoundCheckID()
-	file := filepath.Join(a.DataDir, checksDir, cid.StringHash())
+	file := filepath.Join(a.DataDir, checksDir, cid.StringHashSHA256())
 	if _, err := os.Stat(file); err == nil {
 		t.Fatalf("should have removed persisted check")
 	}
@@ -3452,6 +3532,69 @@ func TestAgent_checkStateSnapshot(t *testing.T) {
 	}
 }
 
+func TestAgent_checkStateSnapshot_backcompat(t *testing.T) {
+	if testing.Short() {
+		t.Skip("too slow for testing.Short")
+	}
+
+	t.Parallel()
+	a := NewTestAgent(t, "")
+	defer a.Shutdown()
+
+	// First register a service
+	svc := &structs.NodeService{
+		ID:      "redis",
+		Service: "redis",
+		Tags:    []string{"foo"},
+		Port:    8000,
+	}
+	if err := a.addServiceFromSource(svc, nil, false, "", ConfigSourceLocal); err != nil {
+		t.Fatalf("err: %v", err)
+	}
+
+	// Register a check
+	check1 := &structs.HealthCheck{
+		Node:        a.Config.NodeName,
+		CheckID:     "service:redis",
+		Name:        "redischeck",
+		Status:      api.HealthPassing,
+		ServiceID:   "redis",
+		ServiceName: "redis",
+	}
+	if err := a.AddCheck(check1, nil, true, "", ConfigSourceLocal); err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	// Snapshot the state
+	snap := a.snapshotCheckState()
+
+	// Unload all of the checks
+	if err := a.unloadChecks(); err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	// Mutate the path to look like the old md5 checksum
+	dir := filepath.Join(a.config.DataDir, checksDir)
+	new_path := filepath.Join(dir, check1.CompoundCheckID().StringHashSHA256())
+	old_path := filepath.Join(dir, check1.CompoundCheckID().StringHashMD5())
+	if err := os.Rename(new_path, old_path); err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	// Reload the checks and restore the snapshot.
+	if err := a.loadChecks(a.Config, snap); err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	// Search for the check
+	out := requireCheckExists(t, a, check1.CheckID)
+
+	// Make sure state was restored
+	if out.Status != api.HealthPassing {
+		t.Fatalf("should have restored check state")
+	}
+}
+
 func TestAgent_loadChecks_checkFails(t *testing.T) {
 	if testing.Short() {
 		t.Skip("too slow for testing.Short")
@@ -3514,7 +3657,7 @@ func TestAgent_persistCheckState(t *testing.T) {
 	}
 
 	// Check the persisted file exists and has the content
-	file := filepath.Join(a.Config.DataDir, checkStateDir, cid.StringHash())
+	file := filepath.Join(a.Config.DataDir, checkStateDir, cid.StringHashSHA256())
 	buf, err := ioutil.ReadFile(file)
 	if err != nil {
 		t.Fatalf("err: %s", err)
@@ -3582,7 +3725,7 @@ func TestAgent_loadCheckState(t *testing.T) {
 	}
 
 	// Should have purged the state
-	file := filepath.Join(a.Config.DataDir, checksDir, stringHash("check1"))
+	file := filepath.Join(a.Config.DataDir, checksDir, structs.NewCheckID("check1", nil).StringHashSHA256())
 	if _, err := os.Stat(file); !os.IsNotExist(err) {
 		t.Fatalf("should have purged state")
 	}
@@ -3639,7 +3782,7 @@ func TestAgent_purgeCheckState(t *testing.T) {
 	}
 
 	// Removed the file
-	file := filepath.Join(a.Config.DataDir, checkStateDir, cid.StringHash())
+	file := filepath.Join(a.Config.DataDir, checkStateDir, cid.StringHashSHA256())
 	if _, err := os.Stat(file); !os.IsNotExist(err) {
 		t.Fatalf("should have removed file")
 	}

agent/service_manager_test.go

@@ -396,8 +396,8 @@ func TestServiceManager_PersistService_API(t *testing.T) {
 	svc := newNodeService()
 	svcID := svc.CompoundServiceID()
 
-	svcFile := filepath.Join(a.Config.DataDir, servicesDir, svcID.StringHash())
+	svcFile := filepath.Join(a.Config.DataDir, servicesDir, svcID.StringHashSHA256())
-	configFile := filepath.Join(a.Config.DataDir, serviceConfigDir, svcID.StringHash())
+	configFile := filepath.Join(a.Config.DataDir, serviceConfigDir, svcID.StringHashSHA256())
 
 	// Service is not persisted unless requested, but we always persist service configs.
 	err = a.AddService(AddServiceRequest{Service: svc, Source: ConfigSourceRemote})
@@ -643,8 +643,8 @@ func TestServiceManager_PersistService_ConfigFiles(t *testing.T) {
 		require.Equal(r, expectState, current)
 	})
 
-	svcFile := filepath.Join(a.Config.DataDir, servicesDir, stringHash(svcID))
+	svcFile := filepath.Join(a.Config.DataDir, servicesDir, stringHashSHA256(svcID))
-	configFile := filepath.Join(a.Config.DataDir, serviceConfigDir, stringHash(svcID))
+	configFile := filepath.Join(a.Config.DataDir, serviceConfigDir, stringHashSHA256(svcID))
 
 	// Service is never persisted, but we always persist service configs.
 	requireFileIsAbsent(t, svcFile)

agent/structs/structs.go

@@ -3,6 +3,7 @@ package structs
 import (
 	"bytes"
 	"crypto/md5"
+	"crypto/sha256"
 	"encoding/json"
 	"fmt"
 	"math/rand"
@@ -1876,15 +1877,25 @@ func NewCheckID(id types.CheckID, entMeta *EnterpriseMeta) CheckID {
 	return cid
 }
 
-// StringHash is used mainly to populate part of the filename of a check
+// StringHashMD5 is used mainly to populate part of the filename of a check
-// definition persisted on the local agent
+// definition persisted on the local agent (deprecated in favor of StringHashSHA256)
-func (cid CheckID) StringHash() string {
+// Kept around for backwards compatibility
+func (cid CheckID) StringHashMD5() string {
 	hasher := md5.New()
 	hasher.Write([]byte(cid.ID))
 	cid.EnterpriseMeta.addToHash(hasher, true)
 	return fmt.Sprintf("%x", hasher.Sum(nil))
 }
 
+// StringHashSHA256 is used mainly to populate part of the filename of a check
+// definition persisted on the local agent
+func (cid CheckID) StringHashSHA256() string {
+	hasher := sha256.New()
+	hasher.Write([]byte(cid.ID))
+	cid.EnterpriseMeta.addToHash(hasher, true)
+	return fmt.Sprintf("%x", hasher.Sum(nil))
+}
+
 type ServiceID struct {
 	ID string
 	EnterpriseMeta
@@ -1906,10 +1917,10 @@ func (sid ServiceID) Matches(other ServiceID) bool {
 	return sid.ID == other.ID && sid.EnterpriseMeta.Matches(&other.EnterpriseMeta)
 }
 
-// StringHash is used mainly to populate part of the filename of a service
+// StringHashSHA256 is used mainly to populate part of the filename of a service
 // definition persisted on the local agent
-func (sid ServiceID) StringHash() string {
+func (sid ServiceID) StringHashSHA256() string {
-	hasher := md5.New()
+	hasher := sha256.New()
 	hasher.Write([]byte(sid.ID))
 	sid.EnterpriseMeta.addToHash(hasher, true)
 	return fmt.Sprintf("%x", hasher.Sum(nil))

agent/util.go

@@ -1,7 +1,7 @@
 package agent
 
 import (
-	"crypto/md5"
+	"crypto/sha256"
 	"fmt"
 	"os"
 	"os/exec"
@@ -14,14 +14,13 @@ import (
 	"github.com/hashicorp/consul/types"
 )
 
-// stringHash returns a simple md5sum for a string.
+func stringHashSHA256(s string) string {
-func stringHash(s string) string {
+	return fmt.Sprintf("%x", sha256.Sum256([]byte(s)))
-	return fmt.Sprintf("%x", md5.Sum([]byte(s)))
 }
 
 // checkIDHash returns a simple md5sum for a types.CheckID.
 func checkIDHash(checkID types.CheckID) string {
-	return stringHash(string(checkID))
+	return stringHashSHA256(string(checkID))
 }
 
 // setFilePermissions handles configuring ownership and permissions

agent/util_test.go

@@ -14,13 +14,13 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-func TestStringHash(t *testing.T) {
+func TestStringHashSHA256(t *testing.T) {
 	t.Parallel()
-	in := "hello world"
+	in := "hello world\n"
-	expected := "5eb63bbbe01eeed093cb22bb8f5acdc3"
+	expected := "a948904f2f0f479b8f8197694b30184b0d2ed1c1cd2a1ec0fb85d299a192a447"
 
-	if out := stringHash(in); out != expected {
+	if out := stringHashSHA256(in); out != expected {
-		t.Fatalf("bad: %s", out)
+		t.Fatalf("bad: %s expected %s", out, expected)
 	}
 }