From 0fd03941c23c696df14e89f5283d4d09622edfac Mon Sep 17 00:00:00 2001 From: Cristian Maglie Date: Sat, 1 Aug 2020 21:51:58 +0200 Subject: [PATCH 1/6] Added tests for path traversal in Resources --- arduino/resources/helpers_test.go | 48 +++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/arduino/resources/helpers_test.go b/arduino/resources/helpers_test.go index 21445d25aab..61a877c1bbb 100644 --- a/arduino/resources/helpers_test.go +++ b/arduino/resources/helpers_test.go @@ -36,6 +36,54 @@ func (h *EchoHandler) ServeHTTP(writer http.ResponseWriter, request *http.Reques request.Write(writer) } +func TestResourcesSanityChecks(t *testing.T) { + tmp, err := paths.MkTempDir("", "") + require.NoError(t, err) + defer tmp.RemoveAll() + + { + testArchiveFileNames := []string{ + "test.txt", + "/test.txt", + "somepath/to/test.txt", + "/../test.txt", + "some/../test.txt", + "../test.txt", + } + for _, testArchiveFileName := range testArchiveFileNames { + r := &DownloadResource{ + ArchiveFileName: testArchiveFileName, + CachePath: "cache", + } + archivePath, err := r.ArchivePath(tmp) + require.NoError(t, err) + expectedArchivePath := tmp.Join("cache", "test.txt") + require.Equal(t, expectedArchivePath.String(), archivePath.String()) + } + } + + { + r := &DownloadResource{ + ArchiveFileName: "/test.txt", + CachePath: "cache", + } + archivePath, err := r.ArchivePath(tmp) + require.NoError(t, err) + expectedArchivePath := tmp.Join("cache", "test.txt") + require.Equal(t, expectedArchivePath.String(), archivePath.String()) + } + + { + r := &DownloadResource{ + ArchiveFileName: "..", + CachePath: "cache", + } + archivePath, err := r.ArchivePath(tmp) + require.Error(t, err) + require.Nil(t, archivePath) + } +} + func TestDownloadApplyUserAgentHeaderUsingConfig(t *testing.T) { goldUserAgentValue := fmt.Sprintf("arduino-cli/0.0.0-test.preview (amd64; linux; go1.12.4) Commit:deadbeef/Build:2019-06-12 11:11:11.111") goldUserAgentString := "User-Agent: " + goldUserAgentValue From d552883af2f057779dd7361fff7c4b6e9b665b4a Mon Sep 17 00:00:00 2001 From: Cristian Maglie Date: Fri, 24 Jul 2020 17:37:42 +0200 Subject: [PATCH 2/6] Do not allow paths in "archiveFileName" property in package_index.json --- arduino/resources/helpers.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/arduino/resources/helpers.go b/arduino/resources/helpers.go index c735ff36559..63c6f4bac26 100644 --- a/arduino/resources/helpers.go +++ b/arduino/resources/helpers.go @@ -20,6 +20,7 @@ import ( "os" "github.com/arduino/go-paths-helper" + "github.com/pkg/errors" "go.bug.st/downloader/v2" ) @@ -30,7 +31,14 @@ func (r *DownloadResource) ArchivePath(downloadDir *paths.Path) (*paths.Path, er if err := staging.MkdirAll(); err != nil { return nil, err } - return staging.Join(r.ArchiveFileName), nil + + // Filter out paths from file name + archiveFileName := paths.New(r.ArchiveFileName).Base() + archivePath := staging.Join(archiveFileName).Clean() + if archivePath.IsDir() { + return nil, errors.Errorf("invalid filename or exinsting directory: %s", archivePath) + } + return archivePath, nil } // IsCached returns true if the specified DownloadResource has already been downloaded From 7f64261cb0d1de0df7be5f2193012b79e33bac6f Mon Sep 17 00:00:00 2001 From: Cristian Maglie Date: Sat, 1 Aug 2020 22:01:21 +0200 Subject: [PATCH 3/6] Resource: increase code coverage --- arduino/resources/helpers.go | 10 +++++----- arduino/resources/helpers_test.go | 23 +++++++++++++++++++++++ 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/arduino/resources/helpers.go b/arduino/resources/helpers.go index 63c6f4bac26..418c91c60ee 100644 --- a/arduino/resources/helpers.go +++ b/arduino/resources/helpers.go @@ -52,6 +52,11 @@ func (r *DownloadResource) IsCached(downloadDir *paths.Path) (bool, error) { // Download a DownloadResource. func (r *DownloadResource) Download(downloadDir *paths.Path, config *downloader.Config) (*downloader.Downloader, error) { + path, err := r.ArchivePath(downloadDir) + if err != nil { + return nil, fmt.Errorf("getting archive path: %s", err) + } + cached, err := r.TestLocalArchiveIntegrity(downloadDir) if err != nil { return nil, fmt.Errorf("testing local archive integrity: %s", err) @@ -61,11 +66,6 @@ func (r *DownloadResource) Download(downloadDir *paths.Path, config *downloader. return nil, nil } - path, err := r.ArchivePath(downloadDir) - if err != nil { - return nil, fmt.Errorf("getting archive path: %s", err) - } - if stats, err := path.Stat(); os.IsNotExist(err) { // normal download } else if err == nil && stats.Size() > r.Size { diff --git a/arduino/resources/helpers_test.go b/arduino/resources/helpers_test.go index 61a877c1bbb..0935803fe74 100644 --- a/arduino/resources/helpers_test.go +++ b/arduino/resources/helpers_test.go @@ -84,6 +84,29 @@ func TestResourcesSanityChecks(t *testing.T) { } } +func TestResourceErrorHandling(t *testing.T) { + tmp, err := paths.MkTempDir("", "") + require.NoError(t, err) + defer tmp.RemoveAll() + + r := &DownloadResource{ + ArchiveFileName: "..", + CachePath: "cache", + } + + c, err := r.IsCached(tmp) + require.Error(t, err) + require.False(t, c) + + d, err := r.Download(tmp, nil) + require.Error(t, err) + require.Nil(t, d) + + e, err := r.TestLocalArchiveIntegrity(tmp) + require.Error(t, err) + require.False(t, e) +} + func TestDownloadApplyUserAgentHeaderUsingConfig(t *testing.T) { goldUserAgentValue := fmt.Sprintf("arduino-cli/0.0.0-test.preview (amd64; linux; go1.12.4) Commit:deadbeef/Build:2019-06-12 11:11:11.111") goldUserAgentString := "User-Agent: " + goldUserAgentValue From 4b3d731d22dd47fbba7b19b14451793139fef1cd Mon Sep 17 00:00:00 2001 From: Cristian Maglie Date: Sat, 1 Aug 2020 22:22:55 +0200 Subject: [PATCH 4/6] Resource: increase code coverage --- arduino/resources/resources_test.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/arduino/resources/resources_test.go b/arduino/resources/resources_test.go index 326f34c11f3..f458171fe8b 100644 --- a/arduino/resources/resources_test.go +++ b/arduino/resources/resources_test.go @@ -80,14 +80,26 @@ func TestDownloadAndChecksums(t *testing.T) { r.Checksum = "BOH:12312312312313123123123123123123" _, err = r.TestLocalArchiveChecksum(tmp) require.Error(t, err) + _, err = r.TestLocalArchiveIntegrity(tmp) + require.Error(t, err) + _, err = r.Download(tmp, nil) + require.Error(t, err) r.Checksum = "MD5 667cf48afcc12c38c8c1637947a04224" _, err = r.TestLocalArchiveChecksum(tmp) require.Error(t, err) + _, err = r.TestLocalArchiveIntegrity(tmp) + require.Error(t, err) + _, err = r.Download(tmp, nil) + require.Error(t, err) r.Checksum = "MD5:zmxcbzxmncbzxmnbczxmnbczxmnbcnnb" _, err = r.TestLocalArchiveChecksum(tmp) require.Error(t, err) + _, err = r.TestLocalArchiveIntegrity(tmp) + require.Error(t, err) + _, err = r.Download(tmp, nil) + require.Error(t, err) r.Checksum = "SHA-1:c007e47637cc6ad6176e7d94aeffc232ee34c1c1" res, err := r.TestLocalArchiveChecksum(tmp) From 258eacb00af5dcf14dbcd92af0d981120a72c15e Mon Sep 17 00:00:00 2001 From: Cristian Maglie Date: Sun, 2 Aug 2020 16:13:45 +0200 Subject: [PATCH 5/6] Use paths.SafeNew when dealing with external sources --- arduino/resources/helpers.go | 6 +++++- arduino/resources/helpers_test.go | 33 +++++++++++++++++++++++-------- go.mod | 2 +- go.sum | 2 ++ 4 files changed, 33 insertions(+), 10 deletions(-) diff --git a/arduino/resources/helpers.go b/arduino/resources/helpers.go index 418c91c60ee..795e41499df 100644 --- a/arduino/resources/helpers.go +++ b/arduino/resources/helpers.go @@ -33,7 +33,11 @@ func (r *DownloadResource) ArchivePath(downloadDir *paths.Path) (*paths.Path, er } // Filter out paths from file name - archiveFileName := paths.New(r.ArchiveFileName).Base() + archiveFile, err := paths.SafeNew(r.ArchiveFileName) + if err != nil { + return nil, errors.Errorf("invalid filename: %s", r.ArchiveFileName) + } + archiveFileName := archiveFile.Base() archivePath := staging.Join(archiveFileName).Clean() if archivePath.IsDir() { return nil, errors.Errorf("invalid filename or exinsting directory: %s", archivePath) diff --git a/arduino/resources/helpers_test.go b/arduino/resources/helpers_test.go index 0935803fe74..5d6c3987439 100644 --- a/arduino/resources/helpers_test.go +++ b/arduino/resources/helpers_test.go @@ -46,9 +46,11 @@ func TestResourcesSanityChecks(t *testing.T) { "test.txt", "/test.txt", "somepath/to/test.txt", - "/../test.txt", - "some/../test.txt", + "/somepath/to/test.txt", + "path/to/../test.txt", + "/path/to/../test.txt", "../test.txt", + "/../test.txt", } for _, testArchiveFileName := range testArchiveFileNames { r := &DownloadResource{ @@ -74,13 +76,28 @@ func TestResourcesSanityChecks(t *testing.T) { } { - r := &DownloadResource{ - ArchiveFileName: "..", - CachePath: "cache", + testArchiveFileNames := []string{ + "/", + ".", + "/.", + "..", + "/..", + "path/..", + "/path/..", + "path/path/..", + "/path/path/..", + ".." + string([]byte{0xC0, 0xAF}) + "test.txt", + "/.." + string([]byte{0xC0, 0xAF}) + "test.txt", + } + for _, testArchiveFileName := range testArchiveFileNames { + r := &DownloadResource{ + ArchiveFileName: testArchiveFileName, + CachePath: "cache", + } + archivePath, err := r.ArchivePath(tmp) + require.Nil(t, archivePath) + require.Error(t, err) } - archivePath, err := r.ArchivePath(tmp) - require.Error(t, err) - require.Nil(t, archivePath) } } diff --git a/go.mod b/go.mod index a07ab1e2cfd..7cd566949cf 100644 --- a/go.mod +++ b/go.mod @@ -9,7 +9,7 @@ require ( bou.ke/monkey v1.0.1 github.com/GeertJohan/go.rice v1.0.0 github.com/arduino/board-discovery v0.0.0-20180823133458-1ba29327fb0c - github.com/arduino/go-paths-helper v1.2.0 + github.com/arduino/go-paths-helper v1.2.1-0.20200802112116-33dcc69b14ba github.com/arduino/go-properties-orderedmap v1.3.0 github.com/arduino/go-timeutils v0.0.0-20171220113728-d1dd9e313b1b github.com/arduino/go-win32-utils v0.0.0-20180330194947-ed041402e83b diff --git a/go.sum b/go.sum index 30933947a33..87f959e7399 100644 --- a/go.sum +++ b/go.sum @@ -18,6 +18,8 @@ github.com/arduino/go-paths-helper v1.0.1 h1:utYXLM2RfFlc9qp/MJTIYp3t6ux/xM6mWje github.com/arduino/go-paths-helper v1.0.1/go.mod h1:HpxtKph+g238EJHq4geEPv9p+gl3v5YYu35Yb+w31Ck= github.com/arduino/go-paths-helper v1.2.0 h1:qDW93PR5IZUN/jzO4rCtexiwF8P4OIcOmcSgAYLZfY4= github.com/arduino/go-paths-helper v1.2.0/go.mod h1:HpxtKph+g238EJHq4geEPv9p+gl3v5YYu35Yb+w31Ck= +github.com/arduino/go-paths-helper v1.2.1-0.20200802112116-33dcc69b14ba h1:rQtLTpIICgc8ad2UG/A7X1F4TpKGoazBxhKR+crsf4k= +github.com/arduino/go-paths-helper v1.2.1-0.20200802112116-33dcc69b14ba/go.mod h1:HpxtKph+g238EJHq4geEPv9p+gl3v5YYu35Yb+w31Ck= github.com/arduino/go-properties-orderedmap v1.3.0 h1:4No/vQopB36e7WUIk6H6TxiSEJPiMrVOCZylYmua39o= github.com/arduino/go-properties-orderedmap v1.3.0/go.mod h1:DKjD2VXY/NZmlingh4lSFMEYCVubfeArCsGPGDwb2yk= github.com/arduino/go-timeutils v0.0.0-20171220113728-d1dd9e313b1b h1:9hDi4F2st6dbLC3y4i02zFT5quS4X6iioWifGlVwfy4= From aea3cc5eab3e969bb2424944099703e693888585 Mon Sep 17 00:00:00 2001 From: Cristian Maglie Date: Sun, 2 Aug 2020 16:17:02 +0200 Subject: [PATCH 6/6] Fixed error message --- arduino/resources/helpers.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arduino/resources/helpers.go b/arduino/resources/helpers.go index 795e41499df..e6251bd0192 100644 --- a/arduino/resources/helpers.go +++ b/arduino/resources/helpers.go @@ -40,7 +40,7 @@ func (r *DownloadResource) ArchivePath(downloadDir *paths.Path) (*paths.Path, er archiveFileName := archiveFile.Base() archivePath := staging.Join(archiveFileName).Clean() if archivePath.IsDir() { - return nil, errors.Errorf("invalid filename or exinsting directory: %s", archivePath) + return nil, errors.Errorf("archive filename points to an existing directory: %s", archivePath) } return archivePath, nil }