diff --git a/config/configuration.go b/config/configuration.go index cb0b0adeda5..43b82a5963a 100644 --- a/config/configuration.go +++ b/config/configuration.go @@ -4,6 +4,7 @@ import ( "encoding/json" "fmt" "io" + "math" "net/url" "os" "path/filepath" @@ -35,7 +36,7 @@ var ( defaultUserConfigDirs = []string{"~/.cloudflared", "~/.cloudflare-warp", "~/cloudflare-warp"} defaultNixConfigDirs = []string{"/etc/cloudflared", DefaultUnixConfigLocation} - ErrNoConfigFile = fmt.Errorf("Cannot determine default configuration path. No file %v in %v", DefaultConfigFiles, DefaultConfigSearchDirectories()) + ErrNoConfigFile = fmt.Errorf("cannot determine default configuration path. No file %v in %v", DefaultConfigFiles, DefaultConfigSearchDirectories()) ) const ( @@ -49,7 +50,8 @@ func DefaultConfigDirectory() string { path := os.Getenv("CFDPATH") if path == "" { path = filepath.Join(os.Getenv("ProgramFiles(x86)"), "cloudflared") - if _, err := os.Stat(path); os.IsNotExist(err) { // doesn't exist, so return an empty failure string + // doesn't exist, so return an empty failure string + if _, err := os.Stat(path); os.IsNotExist(err) { //nolint:gosec // path is derived from a fixed local install location, not attacker-controlled input return "" } } @@ -87,7 +89,7 @@ func DefaultConfigSearchDirectories() []string { // FileExists checks to see if a file exist at the provided path. func FileExists(path string) (bool, error) { - f, err := os.Open(path) + f, err := os.Open(path) //nolint:gosec // path is one of the local default config search paths, not attacker-controlled input if err != nil { if os.IsNotExist(err) { // ignore missing files @@ -126,19 +128,19 @@ func FindOrCreateConfigPath() string { if path == "" { // create the default directory if it doesn't exist path = DefaultConfigPath() - if err := os.MkdirAll(filepath.Dir(path), os.ModePerm); err != nil { + if err := os.MkdirAll(filepath.Dir(path), os.ModePerm); err != nil { //nolint:gosec // pre-existing default config dir permissions; tightening these is a behavior change out of scope here return "" } // write a new config file out - file, err := os.Create(path) + file, err := os.Create(path) //nolint:gosec // path is derived from the local default config directory, not attacker-controlled input if err != nil { return "" } - defer file.Close() + defer func() { _ = file.Close() }() logDir := DefaultLogDirectory() - _ = os.MkdirAll(logDir, os.ModePerm) // try and create it. Doesn't matter if it succeed or not, only byproduct will be no logs + _ = os.MkdirAll(logDir, os.ModePerm) //nolint:gosec // pre-existing default log dir permissions; tightening these is a behavior change out of scope here. Doesn't matter if it succeeds or not, only byproduct will be no logs c := Root{ LogDirectory: logDir, @@ -391,7 +393,7 @@ func ReadConfigFile(c *cli.Context, log *zerolog.Logger) (settings *configFileSe } log.Debug().Msgf("Loading configuration from %s", configFile) - file, err := os.Open(configFile) + file, err := os.Open(configFile) //nolint:gosec // configFile comes from the --config flag / local config search, not attacker-controlled input if err != nil { // If does not exist and config file was not specificly specified then return ErrNoConfigFile found. if os.IsNotExist(err) && !c.IsSet("config") { @@ -399,7 +401,7 @@ func ReadConfigFile(c *cli.Context, log *zerolog.Logger) (settings *configFileSe } return nil, "", err } - defer file.Close() + defer func() { _ = file.Close() }() if err := yaml.NewDecoder(file).Decode(&configuration); err != nil { if err == io.EOF { log.Error().Msgf("Configuration file %s was empty", configFile) @@ -410,7 +412,7 @@ func ReadConfigFile(c *cli.Context, log *zerolog.Logger) (settings *configFileSe configuration.sourceFile = configFile // Parse it again, with strict mode, to find warnings. - if file, err := os.Open(configFile); err == nil { + if file, err := os.Open(configFile); err == nil { //nolint:gosec // configFile comes from the --config flag / local config search, not attacker-controlled input decoder := yaml.NewDecoder(file) decoder.KnownFields(true) var unusedConfig configFileSettings @@ -432,21 +434,31 @@ type CustomDuration struct { } func (s CustomDuration) MarshalJSON() ([]byte, error) { - return json.Marshal(s.Duration.Seconds()) + return json.Marshal(s.Seconds()) } +// Bounds on the number of seconds that can be converted to a time.Duration (nanoseconds) +// without overflowing int64. +const ( + maxDurationSeconds = int64(math.MaxInt64) / int64(time.Second) + minDurationSeconds = int64(math.MinInt64) / int64(time.Second) +) + func (s *CustomDuration) UnmarshalJSON(data []byte) error { seconds, err := strconv.ParseInt(string(data), 10, 64) if err != nil { return err } + if seconds > maxDurationSeconds || seconds < minDurationSeconds { + return fmt.Errorf("%d seconds overflows time.Duration, must be between %d and %d seconds", seconds, minDurationSeconds, maxDurationSeconds) + } s.Duration = time.Duration(seconds * int64(time.Second)) return nil } func (s *CustomDuration) MarshalYAML() (interface{}, error) { - return s.Duration.String(), nil + return s.String(), nil } func (s *CustomDuration) UnmarshalYAML(unmarshal func(interface{}) error) error { diff --git a/config/configuration_test.go b/config/configuration_test.go index 3240a2a0da7..8dae151b6a4 100644 --- a/config/configuration_test.go +++ b/config/configuration_test.go @@ -2,6 +2,7 @@ package config import ( "encoding/json" + "fmt" "testing" "time" @@ -65,7 +66,7 @@ counters: ` var config configFileSettings err := yaml.Unmarshal([]byte(rawYAML), &config) - assert.NoError(t, err) + require.NoError(t, err) assert.Equal(t, "config-file-test", config.TunnelID) assert.Equal(t, firstIngress, config.Ingress[0]) @@ -88,31 +89,30 @@ counters: assert.Equal(t, ipRules, config.OriginRequest.IPRules) retries, err := config.Int("retries") - assert.NoError(t, err) + require.NoError(t, err) assert.Equal(t, 5, retries) gracePeriod, err := config.Duration("grace-period") - assert.NoError(t, err) + require.NoError(t, err) assert.Equal(t, time.Second*30, gracePeriod) percentage, err := config.Float64("percentage") - assert.NoError(t, err) - assert.Equal(t, 3.14, percentage) + require.NoError(t, err) + assert.InDelta(t, 3.14, percentage, 0.0001) hostname, err := config.String("hostname") - assert.NoError(t, err) + require.NoError(t, err) assert.Equal(t, "example.com", hostname) tags, err := config.StringSlice("tag") - assert.NoError(t, err) + require.NoError(t, err) assert.Equal(t, "test", tags[0]) assert.Equal(t, "central-1", tags[1]) counters, err := config.IntSlice("counters") - assert.NoError(t, err) + require.NoError(t, err) assert.Equal(t, 123, counters[0]) assert.Equal(t, 456, counters[1]) - } var rawJsonConfig = []byte(` @@ -148,6 +148,25 @@ var rawJsonConfig = []byte(` } `) +func TestCustomDurationUnmarshalJSONOverflow(t *testing.T) { + var d CustomDuration + + // A caller mistakenly supplying nanoseconds (as documented for time.Duration + // generally) instead of the seconds this type actually expects overflows + // int64 when multiplied by time.Second. This must be a clear error, not a + // silently wrapped negative duration that causes dials to fail instantly. + err := json.Unmarshal([]byte("10000000000"), &d) + require.Error(t, err) + + require.NoError(t, json.Unmarshal([]byte("10"), &d)) + assert.Equal(t, 10*time.Second, d.Duration) + + require.NoError(t, json.Unmarshal([]byte(fmt.Sprintf("%d", maxDurationSeconds)), &d)) + assert.Equal(t, time.Duration(maxDurationSeconds)*time.Second, d.Duration) + + require.Error(t, json.Unmarshal([]byte(fmt.Sprintf("%d", maxDurationSeconds+1)), &d)) +} + func TestMarshalUnmarshalOriginRequest(t *testing.T) { testCases := []struct { name string @@ -173,25 +192,25 @@ func assertConfig( var config OriginRequestConfig var config2 OriginRequestConfig - assert.NoError(t, json.Unmarshal(rawJsonConfig, &config)) + require.NoError(t, json.Unmarshal(rawJsonConfig, &config)) assert.Equal(t, time.Second*10, config.ConnectTimeout.Duration) assert.Equal(t, time.Second*30, config.TLSTimeout.Duration) assert.Equal(t, time.Second*30, config.TCPKeepAlive.Duration) - assert.Equal(t, true, *config.NoHappyEyeballs) + assert.True(t, *config.NoHappyEyeballs) assert.Equal(t, time.Second*60, config.KeepAliveTimeout.Duration) assert.Equal(t, 10, *config.KeepAliveConnections) assert.Equal(t, "app.tunnel.com", *config.HTTPHostHeader) assert.Equal(t, "app.tunnel.com", *config.OriginServerName) assert.Equal(t, "/etc/capool", *config.CAPool) - assert.Equal(t, true, *config.NoTLSVerify) - assert.Equal(t, true, *config.DisableChunkedEncoding) - assert.Equal(t, true, *config.BastionMode) + assert.True(t, *config.NoTLSVerify) + assert.True(t, *config.DisableChunkedEncoding) + assert.True(t, *config.BastionMode) assert.Equal(t, "127.0.0.3", *config.ProxyAddress) - assert.Equal(t, true, *config.NoTLSVerify) + assert.True(t, *config.NoTLSVerify) assert.Equal(t, uint(9000), *config.ProxyPort) assert.Equal(t, "socks", *config.ProxyType) - assert.Equal(t, true, *config.Http2Origin) + assert.True(t, *config.Http2Origin) privateV4 := "10.0.0.0/8" privateV6 := "fc00::/7"