From bd5616991b9310b95262c2dfea9a6590187c05ee Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Tue, 7 Feb 2017 23:15:24 +0000 Subject: [PATCH] time: bound file reads and validate LoadLocation argument Fixes #18985 Change-Id: I956117f47d1d2b453b4786c7b78c1c944defeca0 Reviewed-on: https://go-review.googlesource.com/36551 Reviewed-by: Ian Lance Taylor --- src/time/export_test.go | 2 ++ src/time/sys_plan9.go | 4 ++++ src/time/sys_unix.go | 4 ++++ src/time/sys_windows.go | 4 ++++ src/time/time_test.go | 13 +++++++++++++ src/time/zoneinfo.go | 21 +++++++++++++++++++++ src/time/zoneinfo_read.go | 11 +++++++++++ src/time/zoneinfo_test.go | 24 ++++++++++++++++++++++-- 8 files changed, 81 insertions(+), 2 deletions(-) diff --git a/src/time/export_test.go b/src/time/export_test.go index 26584b5454..4c08ab13af 100644 --- a/src/time/export_test.go +++ b/src/time/export_test.go @@ -32,4 +32,6 @@ var ( ParseTimeZone = parseTimeZone SetMono = (*Time).setMono GetMono = (*Time).mono + ErrLocation = errLocation + ReadFile = readFile ) diff --git a/src/time/sys_plan9.go b/src/time/sys_plan9.go index 11365a791f..9086a6e835 100644 --- a/src/time/sys_plan9.go +++ b/src/time/sys_plan9.go @@ -19,6 +19,7 @@ func interrupt() { // readFile reads and returns the content of the named file. // It is a trivial implementation of ioutil.ReadFile, reimplemented // here to avoid depending on io/ioutil or os. +// It returns an error if name exceeds maxFileSize bytes. func readFile(name string) ([]byte, error) { f, err := syscall.Open(name, syscall.O_RDONLY) if err != nil { @@ -38,6 +39,9 @@ func readFile(name string) ([]byte, error) { if n == 0 || err != nil { break } + if len(ret) > maxFileSize { + return nil, fileSizeError(name) + } } return ret, err } diff --git a/src/time/sys_unix.go b/src/time/sys_unix.go index 91d54c9ffd..d4db8f9afd 100644 --- a/src/time/sys_unix.go +++ b/src/time/sys_unix.go @@ -19,6 +19,7 @@ func interrupt() { // readFile reads and returns the content of the named file. // It is a trivial implementation of ioutil.ReadFile, reimplemented // here to avoid depending on io/ioutil or os. +// It returns an error if name exceeds maxFileSize bytes. func readFile(name string) ([]byte, error) { f, err := syscall.Open(name, syscall.O_RDONLY, 0) if err != nil { @@ -38,6 +39,9 @@ func readFile(name string) ([]byte, error) { if n == 0 || err != nil { break } + if len(ret) > maxFileSize { + return nil, fileSizeError(name) + } } return ret, err } diff --git a/src/time/sys_windows.go b/src/time/sys_windows.go index a4a068f784..9e38165393 100644 --- a/src/time/sys_windows.go +++ b/src/time/sys_windows.go @@ -16,6 +16,7 @@ func interrupt() { // readFile reads and returns the content of the named file. // It is a trivial implementation of ioutil.ReadFile, reimplemented // here to avoid depending on io/ioutil or os. +// It returns an error if name exceeds maxFileSize bytes. func readFile(name string) ([]byte, error) { f, err := syscall.Open(name, syscall.O_RDONLY, 0) if err != nil { @@ -35,6 +36,9 @@ func readFile(name string) ([]byte, error) { if n == 0 || err != nil { break } + if len(ret) > maxFileSize { + return nil, fileSizeError(name) + } } return ret, err } diff --git a/src/time/time_test.go b/src/time/time_test.go index 2922560f09..90e2abf03e 100644 --- a/src/time/time_test.go +++ b/src/time/time_test.go @@ -11,7 +11,9 @@ import ( "fmt" "math/big" "math/rand" + "os" "runtime" + "strings" "testing" "testing/quick" . "time" @@ -1254,3 +1256,14 @@ func TestZeroMonthString(t *testing.T) { t.Errorf("zero month = %q; want %q", got, want) } } + +func TestReadFileLimit(t *testing.T) { + const zero = "/dev/zero" + if _, err := os.Stat(zero); err != nil { + t.Skip("skipping test without a /dev/zero") + } + _, err := ReadFile(zero) + if err == nil || !strings.Contains(err.Error(), "is too large") { + t.Errorf("readFile(%q) error = %v; want error containing 'is too large'", zero, err) + } +} diff --git a/src/time/zoneinfo.go b/src/time/zoneinfo.go index 7cde142239..dfe857f436 100644 --- a/src/time/zoneinfo.go +++ b/src/time/zoneinfo.go @@ -5,6 +5,7 @@ package time import ( + "errors" "sync" "syscall" ) @@ -256,6 +257,8 @@ func (l *Location) lookupName(name string, unix int64) (offset int, isDST bool, // NOTE(rsc): Eventually we will need to accept the POSIX TZ environment // syntax too, but I don't feel like implementing it today. +var errLocation = errors.New("time: invalid location name") + var zoneinfo *string var zoneinfoOnce sync.Once @@ -280,6 +283,11 @@ func LoadLocation(name string) (*Location, error) { if name == "Local" { return Local, nil } + if containsDotDot(name) || name[0] == '/' || name[0] == '\\' { + // No valid IANA Time Zone name contains a single dot, + // much less dot dot. Likewise, none begin with a slash. + return nil, errLocation + } zoneinfoOnce.Do(func() { env, _ := syscall.Getenv("ZONEINFO") zoneinfo = &env @@ -292,3 +300,16 @@ func LoadLocation(name string) (*Location, error) { } return loadLocation(name) } + +// containsDotDot reports whether s contains "..". +func containsDotDot(s string) bool { + if len(s) < 2 { + return false + } + for i := 0; i < len(s)-1; i++ { + if s[i] == '.' && s[i+1] == '.' { + return true + } + } + return false +} diff --git a/src/time/zoneinfo_read.go b/src/time/zoneinfo_read.go index 1b3356e48c..b0cd9da923 100644 --- a/src/time/zoneinfo_read.go +++ b/src/time/zoneinfo_read.go @@ -11,6 +11,17 @@ package time import "errors" +// maxFileSize is the max permitted size of files read by readFile. +// As reference, the zoneinfo.zip distributed by Go is ~350 KB, +// so 10MB is overkill. +const maxFileSize = 10 << 20 + +type fileSizeError string + +func (f fileSizeError) Error() string { + return "time: file " + string(f) + " is too large" +} + // Copies of io.Seek* constants to avoid importing "io": const ( seekStart = 0 diff --git a/src/time/zoneinfo_test.go b/src/time/zoneinfo_test.go index e388e998d0..b25733c1f6 100644 --- a/src/time/zoneinfo_test.go +++ b/src/time/zoneinfo_test.go @@ -20,8 +20,8 @@ func init() { func TestEnvVarUsage(t *testing.T) { time.ResetZoneinfoForTesting() - testZoneinfo := "foo.zip" - env := "ZONEINFO" + const testZoneinfo = "foo.zip" + const env = "ZONEINFO" defer os.Setenv(env, os.Getenv(env)) os.Setenv(env, testZoneinfo) @@ -35,6 +35,26 @@ func TestEnvVarUsage(t *testing.T) { } } +func TestLoadLocationValidatesNames(t *testing.T) { + time.ResetZoneinfoForTesting() + const env = "ZONEINFO" + defer os.Setenv(env, os.Getenv(env)) + os.Setenv(env, "") + + bad := []string{ + "/usr/foo/Foo", + "\\UNC\foo", + "..", + "a..", + } + for _, v := range bad { + _, err := time.LoadLocation(v) + if err != time.ErrLocation { + t.Errorf("LoadLocation(%q) error = %v; want ErrLocation", v, err) + } + } +} + func TestVersion3(t *testing.T) { time.ForceZipFileForTesting(true) defer time.ForceZipFileForTesting(false) -- 2.48.1