Skip to content

Commit

Permalink
Remove some usage of md5 from the system (#11491)
Browse files Browse the repository at this point in the history
* 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>
  • Loading branch information
markan committed Nov 4, 2021
1 parent 61bd417 commit 7e8228a
Show file tree
Hide file tree
Showing 7 changed files with 245 additions and 41 deletions.
3 changes: 3 additions & 0 deletions .changelog/_1253.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:security
agent: Use SHA256 instead of MD5 to generate persistence file names.
```
72 changes: 60 additions & 12 deletions agent/agent.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()),
Expand All @@ -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)
}
Expand All @@ -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{
Expand All @@ -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)
}
Expand All @@ -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{
Expand All @@ -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 {
Expand All @@ -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)
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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"
Expand All @@ -3003,13 +3022,26 @@ 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) {
return nil
// 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)
}
return fmt.Errorf("failed reading file %q: %s", file, err)
}

// Decode the state data
Expand All @@ -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
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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",
Expand Down
Loading

0 comments on commit 7e8228a

Please sign in to comment.