[skip changelog] Do not allow extra paths in "archiveFileName" property in package_index.json

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,18 @@ 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
+	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("archive filename points to an existing directory: %s", archivePath)
+	}
+	return archivePath, nil
 }
 
 // IsCached returns true if the specified DownloadResource has already been downloaded
@@ -44,6 +56,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)
@@ -53,11 +70,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 {

arduino/resources/helpers_test.go

@@ -36,6 +36,94 @@ 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",
+			"/somepath/to/test.txt",
+			"path/to/../test.txt",
+			"/path/to/../test.txt",
+			"../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())
+	}
+
+	{
+		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)
+		}
+	}
+}
+
+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

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)

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

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=