From 4454df0fba9aaa4269ed1157f34cb725d34c5711 Mon Sep 17 00:00:00 2001 From: Rajesh Rajendiran Date: Sun, 26 Jul 2026 14:59:17 +0000 Subject: [PATCH 1/2] config: reject overflowing seconds in CustomDuration.UnmarshalJSON CustomDuration's JSON wire format is documented as seconds (unlike its YAML format, which uses time.Duration's own "10s"-style parser). When a caller instead supplies a value on the order of 1e10 (e.g. mistaking it for nanoseconds, which is the usual convention for time.Duration), multiplying by time.Second overflows int64 and silently wraps into a large negative time.Duration. That negative duration is passed straight into net.Dialer.Timeout for originRequest.connectTimeout, so the dial context is already expired before net.Dialer is ever invoked: dials fail instantly with "i/o timeout" and zero socket()/connect() syscalls, with no indication the config value itself was invalid. Bounds-check the parsed seconds value against the int64 range before multiplying, returning a descriptive unmarshal error instead of silently producing a corrupted duration. Fixes cloudflare/cloudflared#1702 --- config/configuration.go | 11 +++++++++++ config/configuration_test.go | 20 ++++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/config/configuration.go b/config/configuration.go index cb0b0adeda5..7df742be6b7 100644 --- a/config/configuration.go +++ b/config/configuration.go @@ -4,6 +4,7 @@ import ( "encoding/json" "fmt" "io" + "math" "net/url" "os" "path/filepath" @@ -435,11 +436,21 @@ func (s CustomDuration) MarshalJSON() ([]byte, error) { return json.Marshal(s.Duration.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 diff --git a/config/configuration_test.go b/config/configuration_test.go index 3240a2a0da7..aea939e1686 100644 --- a/config/configuration_test.go +++ b/config/configuration_test.go @@ -2,6 +2,7 @@ package config import ( "encoding/json" + "fmt" "testing" "time" @@ -148,6 +149,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 From 40be4e67e909abc2844bd40ceb687846d882f9b7 Mon Sep 17 00:00:00 2001 From: Rajesh Rajendiran Date: Sun, 26 Jul 2026 15:06:22 +0000 Subject: [PATCH 2/2] config: fix pre-existing lint issues in touched files This repo's golangci-lint config uses whole-files: true, so editing configuration.go/configuration_test.go surfaces lint issues across the entire file, not just the diff. Address them: - errcheck: check file.Close() via defer func() { _ = file.Close() }() - staticcheck: de-capitalize ErrNoConfigFile message (ST1005), drop redundant embedded-field selectors in CustomDuration (QF1008) - testifylint: use require.NoError for error assertions that subsequent lines depend on, assert.InDelta for float comparison, assert.True instead of assert.Equal(true, ...) - gosec: nolint with justification for G304/G703 (paths are local config search paths, not attacker-controlled) and G301 (pre-existing directory permissions; tightening them is a separate behavior change out of scope for this PR) --- config/configuration.go | 25 +++++++++++++------------ config/configuration_test.go | 31 +++++++++++++++---------------- 2 files changed, 28 insertions(+), 28 deletions(-) diff --git a/config/configuration.go b/config/configuration.go index 7df742be6b7..43b82a5963a 100644 --- a/config/configuration.go +++ b/config/configuration.go @@ -36,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 ( @@ -50,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 "" } } @@ -88,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 @@ -127,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, @@ -392,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") { @@ -400,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) @@ -411,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 @@ -433,7 +434,7 @@ 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) @@ -457,7 +458,7 @@ func (s *CustomDuration) UnmarshalJSON(data []byte) error { } 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 aea939e1686..8dae151b6a4 100644 --- a/config/configuration_test.go +++ b/config/configuration_test.go @@ -66,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]) @@ -89,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(` @@ -193,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"