config: reject overflowing seconds in CustomDuration.UnmarshalJSON

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,15 +393,15 @@ 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") {
 			err = ErrNoConfigFile
 		}
 		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 {

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"